Skip to content

Avoid signed overflow in RGBLINK's +, -, and * - #2060

Merged
Rangi42 merged 1 commit into
gbdev:masterfrom
MaxFreedomPollard:link-rpn-unsigned-arithmetic
Sep 7, 2026
Merged

Rangi42 merged 1 commit into
gbdev:masterfrom
MaxFreedomPollard:link-rpn-unsigned-arithmetic

Conversation

@MaxFreedomPollard

Copy link
Copy Markdown
Contributor

RGBLINK's RPN evaluator computed RPN_ADD, RPN_SUB, and RPN_MUL directly on int32_t, at src/link/patch.cpp lines 117, 121, and 124. Signed overflow is UB in C++, so any link-time expression whose result leaves the 32-bit signed range was undefined. RGBASM already handles this: Expression::makeBinaryOp in src/asm/rpn.cpp cast both operands to uint32_t for exactly those three operators and cast the result back. RGBLINK never got the same treatment.

The develop preset turns on UBSan, so it shows up right away:

$ cat over.asm
def fzero equs "startof(\"test\")"
section "test", rom0
dl $7fff_ffff + ({fzero} + 1)
$ ./rgbasm -o over.o over.asm && ./rgblink -o over.gb over.o
src/link/patch.cpp:117:26: runtime error: signed integer overflow: 1 + 2147483647 cannot be represented in type 'int32_t' (aka 'int')

I moved the three operators into opmath.cpp as op_add, op_sub, and op_mul, next to op_neg and the others that are already there to avoid C++'s behavior, and called them from both RGBASM and RGBLINK, so the two evaluators cannot drift apart again. RPN_ADD and RPN_MUL still pop both operands in one expression, which stays safe because both operations are commutative.

Values are unchanged on every platform RGBDS supports, so no existing expected output moved. The new test/link/patch-wraparound test covers all three operators: its .out.bin pins the wrapped results $80000000, $7FFFFFFF, and $7FFFFFFD, and its empty .out fails without the fix, because the three UBSan reports land on stderr.

Built with cmake -B build --preset develop and cmake --build build -j 4 on macOS 26 with Apple clang 17, then ran test/run-tests.sh --only-internal: 864 RGBASM, 115 RGBLINK, 441 RGBFIX, and 245 RGBGFX tests pass. I did not run the external-project tests.

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.
@Rangi42 Rangi42 added bug Unexpected behavior / crashes; to be fixed ASAP! rgblink This affects RGBLINK labels Sep 7, 2026
@Rangi42 Rangi42 added this to the 1.0.4 milestone Sep 7, 2026
@Rangi42
Rangi42 self-requested a review September 7, 2026 19:55

@Rangi42 Rangi42 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed that the UBSan error exists and that this fixes it. The change looks good and matches our pre-existing code style. Nice catch, thank you!

@Rangi42
Rangi42 merged commit 631ef00 into gbdev:master Sep 7, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Unexpected behavior / crashes; to be fixed ASAP! rgblink This affects RGBLINK

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants