From 631ef003e72d3580cb4431deff324a9afbbe3b85 Mon Sep 17 00:00:00 2001 From: Max Freedom Pollard <272618364+MaxFreedomPollard@users.noreply.github.com> Date: Mon, 7 Sep 2026 16:15:07 -0400 Subject: [PATCH] Avoid signed overflow in RGBLINK's `+`, `-`, and `*` (#2060) RGBASM computes these with unsigned arithmetic and casts back, since signed overflow is UB in C++, but RGBLINK's RPN evaluator used `int32_t` directly, so `src/link/patch.cpp` tripped UBSan on lines 117, 121, and 124. Share the three operators through `opmath.cpp` so both evaluators stay in step. --- include/opmath.hpp | 3 +++ src/asm/rpn.cpp | 7 +++---- src/link/patch.cpp | 6 +++--- src/opmath.cpp | 17 +++++++++++++++++ test/link/patch-wraparound.asm | 9 +++++++++ test/link/patch-wraparound.out | 0 test/link/patch-wraparound.out.bin | Bin 0 -> 12 bytes 7 files changed, 35 insertions(+), 7 deletions(-) create mode 100644 test/link/patch-wraparound.asm create mode 100644 test/link/patch-wraparound.out create mode 100644 test/link/patch-wraparound.out.bin diff --git a/include/opmath.hpp b/include/opmath.hpp index c4dac167..682769a5 100644 --- a/include/opmath.hpp +++ b/include/opmath.hpp @@ -5,6 +5,9 @@ #include +int32_t op_add(int32_t augend, int32_t addend); +int32_t op_sub(int32_t minuend, int32_t subtrahend); +int32_t op_mul(int32_t multiplicand, int32_t multiplier); int32_t op_divide(int32_t dividend, int32_t divisor); int32_t op_modulo(int32_t dividend, int32_t divisor); int32_t op_exponent(int32_t base, uint32_t power); diff --git a/src/asm/rpn.cpp b/src/asm/rpn.cpp index e0c29f3e..301c8e35 100644 --- a/src/asm/rpn.cpp +++ b/src/asm/rpn.cpp @@ -297,7 +297,6 @@ void Expression::makeBinaryOp(RPNCommand op, Expression &&src1, Expression const if (src1.isKnown() && src2.isKnown()) { // If both expressions are known, just compute the value int32_t lval = src1.value(), rval = src2.value(); - uint32_t ulval = static_cast(lval), urval = static_cast(rval); switch (op) { case RPN_LOGOR: @@ -325,10 +324,10 @@ void Expression::makeBinaryOp(RPNCommand op, Expression &&src1, Expression const data = lval != rval; break; case RPN_ADD: - data = static_cast(ulval + urval); + data = op_add(lval, rval); break; case RPN_SUB: - data = static_cast(ulval - urval); + data = op_sub(lval, rval); break; case RPN_XOR: data = lval ^ rval; @@ -370,7 +369,7 @@ void Expression::makeBinaryOp(RPNCommand op, Expression &&src1, Expression const data = op_shift_right_unsigned(lval, rval); break; case RPN_MUL: - data = static_cast(ulval * urval); + data = op_mul(lval, rval); break; case RPN_DIV: if (rval == 0) { diff --git a/src/link/patch.cpp b/src/link/patch.cpp index 4efca6a3..75729c9d 100644 --- a/src/link/patch.cpp +++ b/src/link/patch.cpp @@ -114,14 +114,14 @@ static int32_t computeRPNExpr(Patch const &patch, std::vector const &fil int32_t value; switch (command) { case RPN_ADD: - value = popRPN(patch) + popRPN(patch); + value = op_add(popRPN(patch), popRPN(patch)); break; case RPN_SUB: value = popRPN(patch); - value = popRPN(patch) - value; + value = op_sub(popRPN(patch), value); break; case RPN_MUL: - value = popRPN(patch) * popRPN(patch); + value = op_mul(popRPN(patch), popRPN(patch)); break; case RPN_DIV: value = popRPN(patch); diff --git a/src/opmath.cpp b/src/opmath.cpp index 44916a24..b3cfa8e2 100644 --- a/src/opmath.cpp +++ b/src/opmath.cpp @@ -8,6 +8,23 @@ #include "helpers.hpp" // assume, clz, ctz +// Signed overflow is UB, so these compute with unsigned arithmetic, which wraps around. +// Casting back is OK because the types implement two's complement behavior. + +int32_t op_add(int32_t augend, int32_t addend) { + return static_cast(static_cast(augend) + static_cast(addend)); +} + +int32_t op_sub(int32_t minuend, int32_t subtrahend) { + return static_cast(static_cast(minuend) - static_cast(subtrahend)); +} + +int32_t op_mul(int32_t multiplicand, int32_t multiplier) { + return static_cast( + static_cast(multiplicand) * static_cast(multiplier) + ); +} + int32_t op_divide(int32_t dividend, int32_t divisor) { assume(divisor != 0); // Division by 0 is UB assume(dividend != INT32_MIN || divisor != -1); // INT32_MIN / -1 is UB diff --git a/test/link/patch-wraparound.asm b/test/link/patch-wraparound.asm new file mode 100644 index 00000000..cfb75414 --- /dev/null +++ b/test/link/patch-wraparound.asm @@ -0,0 +1,9 @@ +def fzero equs "startof(\"test\")" +section "test", rom0 +; Signed overflow is undefined behavior, so these must wrap around instead +dl $7fff_ffff + ({fzero} + 1) +dl $8000_0000 - ({fzero} + 1) +dl $7fff_ffff * ({fzero} + 3) + +; XXX: We rely on this landing at address $0000, which isn't *guaranteed*... +assert startof("test") == 0 diff --git a/test/link/patch-wraparound.out b/test/link/patch-wraparound.out new file mode 100644 index 00000000..e69de29b diff --git a/test/link/patch-wraparound.out.bin b/test/link/patch-wraparound.out.bin new file mode 100644 index 0000000000000000000000000000000000000000..ebf24f6a09d80298f2710d56006a9b0b638287a9 GIT binary patch literal 12 ScmZQzU}*UN|9|~oAO-*>Uq literal 0 HcmV?d00001