mirror of
https://github.com/gbdev/rgbds.git
synced 2026-09-15 18:27:06 +00:00
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.
This commit is contained in:
@@ -5,6 +5,9 @@
|
||||
|
||||
#include <stdint.h>
|
||||
|
||||
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);
|
||||
|
||||
+3
-4
@@ -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<uint32_t>(lval), urval = static_cast<uint32_t>(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<int32_t>(ulval + urval);
|
||||
data = op_add(lval, rval);
|
||||
break;
|
||||
case RPN_SUB:
|
||||
data = static_cast<int32_t>(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<int32_t>(ulval * urval);
|
||||
data = op_mul(lval, rval);
|
||||
break;
|
||||
case RPN_DIV:
|
||||
if (rval == 0) {
|
||||
|
||||
+3
-3
@@ -114,14 +114,14 @@ static int32_t computeRPNExpr(Patch const &patch, std::vector<Symbol> 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);
|
||||
|
||||
@@ -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<int32_t>(static_cast<uint32_t>(augend) + static_cast<uint32_t>(addend));
|
||||
}
|
||||
|
||||
int32_t op_sub(int32_t minuend, int32_t subtrahend) {
|
||||
return static_cast<int32_t>(static_cast<uint32_t>(minuend) - static_cast<uint32_t>(subtrahend));
|
||||
}
|
||||
|
||||
int32_t op_mul(int32_t multiplicand, int32_t multiplier) {
|
||||
return static_cast<int32_t>(
|
||||
static_cast<uint32_t>(multiplicand) * static_cast<uint32_t>(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
|
||||
|
||||
@@ -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
|
||||
Binary file not shown.
Reference in New Issue
Block a user