Skip to content

Commit

Permalink
Avoid negation of signed integers in C that may hold INT*_MIN.
Browse files Browse the repository at this point in the history
Reported by minoki.
Recent C compilers 'take advantage' of the undefined behavior.
This completely changes the meaning of expressions like (k == -k).

(cherry picked from commit 8a5e398)

This patch changes all possibly dangerous -x operations on integers to
the corresponding two's complement. Also, it removes all related UBSAN
suppressions, since they are fixed.

Also, this patch limits the `bit.tohex()` result by 254 characters.

There is no testcase for `strscan_oct()`, `strscan_dec()` or/and
`STRSCAN_U32` format since first the unary minus is parsed first and
only after the number itself is parsed during parsing C syntax. So the
error is raised in `cp_expr_prefix()` instead. For parsing the exponent
header, there is no testcase, since the power is limited by
`STRSCAN_MAXEXP`.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#9924
Relates to tarantool/tarantool#8473

Reviewed-by: Maxim Kokryashkin <[email protected]>
Reviewed-by: Sergey Bronnikov <[email protected]>
Signed-off-by: Sergey Kaplun <[email protected]>
  • Loading branch information
Mike Pall authored and Buristan committed Jul 4, 2024
1 parent 4eaacf8 commit 78f4de4
Show file tree
Hide file tree
Showing 16 changed files with 164 additions and 55 deletions.
2 changes: 1 addition & 1 deletion src/lib_base.c
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ LJLIB_ASM(tonumber) LJLIB_REC(.)
while (lj_char_isspace((unsigned char)(*ep))) ep++;
if (*ep == '\0') {
if (LJ_DUALNUM && LJ_LIKELY(ul < 0x80000000u+neg)) {
if (neg) ul = -ul;
if (neg) ul = ~ul+1u;
setintV(L->base-1-LJ_FR2, (int32_t)ul);
} else {
lua_Number n = (lua_Number)ul;
Expand Down
3 changes: 2 additions & 1 deletion src/lib_bit.c
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,8 @@ LJLIB_CF(bit_tohex) LJLIB_REC(.)
#endif
SBuf *sb = lj_buf_tmp_(L);
SFormat sf = (STRFMT_UINT|STRFMT_T_HEX);
if (n < 0) { n = -n; sf |= STRFMT_F_UPPER; }
if (n < 0) { n = (int32_t)(~(uint32_t)n+1u); sf |= STRFMT_F_UPPER; }
if ((uint32_t)n > 254) n = 254;
sf |= ((SFormat)((n+1)&255) << STRFMT_SH_PREC);
#if LJ_HASFFI
if (n < 16) b &= ((uint64_t)1 << 4*n)-1;
Expand Down
2 changes: 1 addition & 1 deletion src/lj_asm_mips.h
Original file line number Diff line number Diff line change
Expand Up @@ -1843,7 +1843,7 @@ static void asm_arithov(ASMState *as, IRIns *ir)
lj_assertA(!irt_is64(ir->t), "bad usage");
if (irref_isk(ir->op2)) {
int k = IR(ir->op2)->i;
if (ir->o == IR_SUBOV) k = -k;
if (ir->o == IR_SUBOV) k = (int)(~(unsigned int)k+1u);
if (checki16(k)) { /* (dest < left) == (k >= 0 ? 1 : 0) */
left = ra_alloc1(as, ir->op1, RSET_GPR);
asm_guard(as, k >= 0 ? MIPSI_BNE : MIPSI_BEQ, RID_TMP, RID_ZERO);
Expand Down
7 changes: 1 addition & 6 deletions src/lj_carith.c
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,6 @@ static int carith_ptr(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
}

/* 64 bit integer arithmetic. */
#if LUAJIT_USE_UBSAN
/* See https://github.com/LuaJIT/LuaJIT/issues/928. */
static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
__attribute__((no_sanitize("signed-integer-overflow")));
#endif
static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
{
if (ctype_isnum(ca->ct[0]->info) && ca->ct[0]->size <= 8 &&
Expand Down Expand Up @@ -216,7 +211,7 @@ static int carith_int64(lua_State *L, CTState *cts, CDArith *ca, MMS mm)
else
*up = lj_carith_powu64(u0, u1);
break;
case MM_unm: *up = (uint64_t)-(int64_t)u0; break;
case MM_unm: *up = ~u0+1u; break;
default:
lj_assertL(0, "bad metamethod %d", mm);
break;
Expand Down
2 changes: 1 addition & 1 deletion src/lj_cparse.c
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ static void cp_expr_prefix(CPState *cp, CPValue *k)
} else if (cp_opt(cp, '+')) {
cp_expr_unary(cp, k); /* Nothing to do (well, integer promotion). */
} else if (cp_opt(cp, '-')) {
cp_expr_unary(cp, k); k->i32 = -k->i32;
cp_expr_unary(cp, k); k->i32 = (int32_t)(~(uint32_t)k->i32+1);
} else if (cp_opt(cp, '~')) {
cp_expr_unary(cp, k); k->i32 = ~k->i32;
} else if (cp_opt(cp, '!')) {
Expand Down
3 changes: 2 additions & 1 deletion src/lj_crecord.c
Original file line number Diff line number Diff line change
Expand Up @@ -1877,7 +1877,8 @@ TRef recff_bit64_tohex(jit_State *J, RecordFFData *rd, TRef hdr)
} else {
n = id ? 16 : 8;
}
if (n < 0) { n = -n; sf |= STRFMT_F_UPPER; }
if (n < 0) { n = (int32_t)(~n+1u); sf |= STRFMT_F_UPPER; }
if ((uint32_t)n > 254) n = 254;
sf |= ((SFormat)((n+1)&255) << STRFMT_SH_PREC);
if (id) {
tr = crec_ct_tv(J, ctype_get(cts, id), 0, J->base[0], &rd->argv[0]);
Expand Down
2 changes: 1 addition & 1 deletion src/lj_ctype.c
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ GCstr *lj_ctype_repr_int64(lua_State *L, uint64_t n, int isunsigned)
if (isunsigned) {
*--p = 'U';
} else if ((int64_t)n < 0) {
n = (uint64_t)-(int64_t)n;
n = ~n+1u;
sign = 1;
}
do { *--p = (char)('0' + n % 10); } while (n /= 10);
Expand Down
2 changes: 1 addition & 1 deletion src/lj_emit_arm.h
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ static int emit_kdelta2(ASMState *as, Reg rd, int32_t i)
if (other) {
int32_t delta = i - other;
uint32_t sh, inv = 0, k2, k;
if (delta < 0) { delta = -delta; inv = ARMI_ADD^ARMI_SUB; }
if (delta < 0) { delta = (int32_t)(~(uint32_t)delta+1u); inv = ARMI_ADD^ARMI_SUB; }
sh = lj_ffs(delta) & ~1;
k2 = emit_isk12(0, delta & (255 << sh));
k = emit_isk12(0, delta & ~(255 << sh));
Expand Down
9 changes: 5 additions & 4 deletions src/lj_emit_arm64.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ static uint64_t get_k64val(ASMState *as, IRRef ref)
/* Encode constant in K12 format for data processing instructions. */
static uint32_t emit_isk12(int64_t n)
{
uint64_t k = (n < 0) ? -n : n;
uint32_t m = (n < 0) ? 0x40000000 : 0;
uint64_t k = n < 0 ? ~(uint64_t)n+1u : (uint64_t)n;
uint32_t m = n < 0 ? 0x40000000 : 0;
if (k < 0x1000) {
return A64I_K12|m|A64F_U12(k);
} else if ((k & 0xfff000) == k) {
Expand Down Expand Up @@ -177,7 +177,7 @@ static int emit_kdelta(ASMState *as, Reg rd, uint64_t k, int lim)
emit_dm(as, A64I_MOVx, rd, r);
return 1;
} else {
uint32_t k12 = emit_isk12(delta < 0 ? -delta : delta);
uint32_t k12 = emit_isk12(delta < 0 ? (int64_t)(~(uint64_t)delta+1u) : delta);
if (k12) {
emit_dn(as, (delta < 0 ? A64I_SUBx : A64I_ADDx)^k12, rd, r);
return 1;
Expand Down Expand Up @@ -415,7 +415,8 @@ static void emit_addptr(ASMState *as, Reg r, int32_t ofs)
{
if (ofs)
emit_opk(as, ofs < 0 ? A64I_SUBx : A64I_ADDx, r, r,
ofs < 0 ? -ofs : ofs, rset_exclude(RSET_GPR, r));
ofs < 0 ? (int32_t)(~(uint32_t)ofs+1u) : ofs,
rset_exclude(RSET_GPR, r));
}

#define emit_spsub(as, ofs) emit_addptr(as, RID_SP, -(ofs))
Expand Down
2 changes: 1 addition & 1 deletion src/lj_obj.h
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ typedef struct GCproto {
#define PROTO_UV_IMMUTABLE 0x4000 /* Immutable upvalue. */

#define proto_kgc(pt, idx) \
check_exp((uintptr_t)(intptr_t)(idx) >= (uintptr_t)-(intptr_t)(pt)->sizekgc, \
check_exp((uintptr_t)(intptr_t)(idx) >= ~(uintptr_t)(pt)->sizekgc+1u, \
gcref(mref((pt)->k, GCRef)[(idx)]))
#define proto_knumtv(pt, idx) \
check_exp((uintptr_t)(idx) < (pt)->sizekn, &mref((pt)->k, TValue)[(idx)])
Expand Down
6 changes: 3 additions & 3 deletions src/lj_opt_fold.c
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ static int32_t kfold_intop(int32_t k1, int32_t k2, IROp op)
case IR_SUB: k1 -= k2; break;
case IR_MUL: k1 *= k2; break;
case IR_MOD: k1 = lj_vm_modi(k1, k2); break;
case IR_NEG: k1 = -k1; break;
case IR_NEG: k1 = (int32_t)(~(uint32_t)k1+1u); break;
case IR_BAND: k1 &= k2; break;
case IR_BOR: k1 |= k2; break;
case IR_BXOR: k1 ^= k2; break;
Expand Down Expand Up @@ -1337,7 +1337,7 @@ LJFOLDF(simplify_intsub_k)
if (fright->i == 0) /* i - 0 ==> i */
return LEFTFOLD;
fins->o = IR_ADD; /* i - k ==> i + (-k) */
fins->op2 = (IRRef1)lj_ir_kint(J, -fright->i); /* Overflow for -2^31 ok. */
fins->op2 = (IRRef1)lj_ir_kint(J, (int32_t)(~(uint32_t)fright->i+1u)); /* Overflow for -2^31 ok. */
return RETRYFOLD;
}

Expand Down Expand Up @@ -1368,7 +1368,7 @@ LJFOLDF(simplify_intsub_k64)
if (k == 0) /* i - 0 ==> i */
return LEFTFOLD;
fins->o = IR_ADD; /* i - k ==> i + (-k) */
fins->op2 = (IRRef1)lj_ir_kint64(J, (uint64_t)-(int64_t)k);
fins->op2 = (IRRef1)lj_ir_kint64(J, ~k+1u);
return RETRYFOLD;
}

Expand Down
17 changes: 6 additions & 11 deletions src/lj_parse.c
Original file line number Diff line number Diff line change
Expand Up @@ -939,11 +939,6 @@ static void bcemit_binop(FuncState *fs, BinOpr op, ExpDesc *e1, ExpDesc *e2)
}

/* Emit unary operator. */
#if LUAJIT_USE_UBSAN
/* See https://github.com/LuaJIT/LuaJIT/issues/928. */
static void bcemit_unop(FuncState *fs, BCOp op, ExpDesc *e)
__attribute__((no_sanitize("signed-integer-overflow")));
#endif
static void bcemit_unop(FuncState *fs, BCOp op, ExpDesc *e)
{
if (op == BC_NOT) {
Expand Down Expand Up @@ -975,22 +970,22 @@ static void bcemit_unop(FuncState *fs, BCOp op, ExpDesc *e)
#if LJ_HASFFI
if (e->k == VKCDATA) { /* Fold in-place since cdata is not interned. */
GCcdata *cd = cdataV(&e->u.nval);
int64_t *p = (int64_t *)cdataptr(cd);
uint64_t *p = (uint64_t *)cdataptr(cd);
if (cd->ctypeid == CTID_COMPLEX_DOUBLE)
p[1] ^= (int64_t)U64x(80000000,00000000);
p[1] ^= U64x(80000000,00000000);
else
*p = -*p;
*p = ~*p+1u;
return;
} else
#endif
if (expr_isnumk(e) && !expr_numiszero(e)) { /* Avoid folding to -0. */
TValue *o = expr_numtv(e);
if (tvisint(o)) {
int32_t k = intV(o);
if (k == -k)
int32_t k = intV(o), negk = (int32_t)(~(uint32_t)k+1u);
if (k == negk)
setnumV(o, -(lua_Number)k);
else
setintV(o, -k);
setintV(o, negk);
return;
} else {
o->u64 ^= U64x(80000000,00000000);
Expand Down
9 changes: 2 additions & 7 deletions src/lj_strfmt.c
Original file line number Diff line number Diff line change
Expand Up @@ -93,15 +93,10 @@ SFormat LJ_FASTCALL lj_strfmt_parse(FormatState *fs)
{ uint32_t d = (x*(((1<<sh)+sc-1)/sc))>>sh; x -= d*sc; *p++ = (char)('0'+d); }

/* Write integer to buffer. */
#if LUAJIT_USE_UBSAN
/* See https://github.com/LuaJIT/LuaJIT/issues/928. */
char * LJ_FASTCALL lj_strfmt_wint(char *p, int32_t k)
__attribute__((no_sanitize("signed-integer-overflow")));
#endif
char * LJ_FASTCALL lj_strfmt_wint(char *p, int32_t k)
{
uint32_t u = (uint32_t)k;
if (k < 0) { u = (uint32_t)-k; *p++ = '-'; }
if (k < 0) { u = ~u+1u; *p++ = '-'; }
if (u < 10000) {
if (u < 10) goto dig1;
if (u < 100) goto dig2;
Expand Down Expand Up @@ -269,7 +264,7 @@ SBuf *lj_strfmt_putfxint(SBuf *sb, SFormat sf, uint64_t k)
/* Figure out signed prefixes. */
if (STRFMT_TYPE(sf) == STRFMT_INT) {
if ((int64_t)k < 0) {
k = (uint64_t)-(int64_t)k;
k = ~k+1u;
prefix = 256 + '-';
} else if ((sf & STRFMT_F_PLUS)) {
prefix = 256 + '+';
Expand Down
26 changes: 13 additions & 13 deletions src/lj_strscan.c
Original file line number Diff line number Diff line change
Expand Up @@ -124,19 +124,19 @@ static StrScanFmt strscan_hex(const uint8_t *p, TValue *o,
case STRSCAN_INT:
if (!(opt & STRSCAN_OPT_TONUM) && x < 0x80000000u+neg &&
!(x == 0 && neg)) {
o->i = neg ? -(int32_t)x : (int32_t)x;
o->i = neg ? (int32_t)(~x+1u) : (int32_t)x;
return STRSCAN_INT; /* Fast path for 32 bit integers. */
}
if (!(opt & STRSCAN_OPT_C)) { fmt = STRSCAN_NUM; break; }
/* fallthrough */
case STRSCAN_U32:
if (dig > 8) return STRSCAN_ERROR;
o->i = neg ? -(int32_t)x : (int32_t)x;
o->i = neg ? (int32_t)(~x+1u) : (int32_t)x;
return STRSCAN_U32;
case STRSCAN_I64:
case STRSCAN_U64:
if (dig > 16) return STRSCAN_ERROR;
o->u64 = neg ? (uint64_t)-(int64_t)x : x;
o->u64 = neg ? ~x+1u : x;
return fmt;
default:
break;
Expand Down Expand Up @@ -168,12 +168,12 @@ static StrScanFmt strscan_oct(const uint8_t *p, TValue *o,
/* fallthrough */
case STRSCAN_U32:
if ((x >> 32)) return STRSCAN_ERROR;
o->i = neg ? -(int32_t)x : (int32_t)x;
o->i = neg ? (int32_t)(~(uint32_t)x+1u) : (int32_t)x;
break;
default:
case STRSCAN_I64:
case STRSCAN_U64:
o->u64 = neg ? (uint64_t)-(int64_t)x : x;
o->u64 = neg ? ~x+1u : x;
break;
}
return fmt;
Expand Down Expand Up @@ -229,18 +229,18 @@ static StrScanFmt strscan_dec(const uint8_t *p, TValue *o,
switch (fmt) {
case STRSCAN_INT:
if (!(opt & STRSCAN_OPT_TONUM) && x < 0x80000000u+neg) {
o->i = neg ? -(int32_t)x : (int32_t)x;
o->i = neg ? (int32_t)(~x+1u) : (int32_t)x;
return STRSCAN_INT; /* Fast path for 32 bit integers. */
}
if (!(opt & STRSCAN_OPT_C)) { fmt = STRSCAN_NUM; goto plainnumber; }
/* fallthrough */
case STRSCAN_U32:
if ((x >> 32) != 0) return STRSCAN_ERROR;
o->i = neg ? -(int32_t)x : (int32_t)x;
o->i = neg ? (int32_t)(~x+1u) : (int32_t)x;
return STRSCAN_U32;
case STRSCAN_I64:
case STRSCAN_U64:
o->u64 = neg ? (uint64_t)-(int64_t)x : x;
o->u64 = neg ? ~x+1u : x;
return fmt;
default:
plainnumber: /* Fast path for plain numbers < 2^63. */
Expand Down Expand Up @@ -348,18 +348,18 @@ static StrScanFmt strscan_bin(const uint8_t *p, TValue *o,
switch (fmt) {
case STRSCAN_INT:
if (!(opt & STRSCAN_OPT_TONUM) && x < 0x80000000u+neg) {
o->i = neg ? -(int32_t)x : (int32_t)x;
o->i = neg ? (int32_t)(~x+1u) : (int32_t)x;
return STRSCAN_INT; /* Fast path for 32 bit integers. */
}
if (!(opt & STRSCAN_OPT_C)) { fmt = STRSCAN_NUM; break; }
/* fallthrough */
case STRSCAN_U32:
if (dig > 32) return STRSCAN_ERROR;
o->i = neg ? -(int32_t)x : (int32_t)x;
o->i = neg ? (int32_t)(~x+1u) : (int32_t)x;
return STRSCAN_U32;
case STRSCAN_I64:
case STRSCAN_U64:
o->u64 = neg ? (uint64_t)-(int64_t)x : x;
o->u64 = neg ? ~x+1u : x;
return fmt;
default:
break;
Expand Down Expand Up @@ -468,7 +468,7 @@ StrScanFmt lj_strscan_scan(const uint8_t *p, MSize len, TValue *o,
if (xx >= STRSCAN_MAXEXP) return STRSCAN_ERROR;
p++;
}
ex += negx ? -(int32_t)xx : (int32_t)xx;
ex += negx ? (int32_t)(~xx+1u) : (int32_t)xx;
}

/* Parse suffix. */
Expand Down Expand Up @@ -507,7 +507,7 @@ StrScanFmt lj_strscan_scan(const uint8_t *p, MSize len, TValue *o,
o->n = -0.0;
return STRSCAN_NUM;
} else {
o->i = neg ? -(int32_t)x : (int32_t)x;
o->i = neg ? (int32_t)(~x+1u) : (int32_t)x;
return STRSCAN_INT;
}
}
Expand Down
6 changes: 3 additions & 3 deletions src/lj_vmmath.c
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,11 @@ int32_t LJ_FASTCALL lj_vm_modi(int32_t a, int32_t b)
uint32_t y, ua, ub;
/* This must be checked before using this function. */
lj_assertX(b != 0, "modulo with zero divisor");
ua = a < 0 ? (uint32_t)-a : (uint32_t)a;
ub = b < 0 ? (uint32_t)-b : (uint32_t)b;
ua = a < 0 ? ~(uint32_t)a+1u : (uint32_t)a;
ub = b < 0 ? ~(uint32_t)b+1u : (uint32_t)b;
y = ua % ub;
if (y != 0 && (a^b) < 0) y = y - ub;
if (((int32_t)y^b) < 0) y = (uint32_t)-(int32_t)y;
if (((int32_t)y^b) < 0) y = ~y+1u;
return (int32_t)y;
}
#endif
Expand Down
Loading

0 comments on commit 78f4de4

Please sign in to comment.