Fix overflow on integer casting on modulo operator #2806
Conversation
itchyny
force-pushed
the
fix-cast-overflow-modulo
branch
from
6885c7f
to
d5e7caf
Compare
itchyny
changed the title
|
Did that raise a warning somewhere? |
| @@ -396,7 +396,7 @@ static jv f_divide(jq_state *jq, jv input, jv a, jv b) { | |||
| } | |||
| } | |||
|
|
|||
| #define dtoi(n) ((n) < INTMAX_MIN ? INTMAX_MIN : -(n) < INTMAX_MIN ? INTMAX_MAX : (intmax_t)(n)) | |||
| #define dtoi(n) ((n) < INTMAX_MIN ? INTMAX_MIN : -(n) <= INTMAX_MIN ? INTMAX_MAX : (intmax_t)(n)) | |||
There was a problem hiding this comment.
I thought about this when reviewing the original, but n is a double, so I don't think there's a problem here because INTMAX_MIN should be promoted to double.
There was a problem hiding this comment.
But n = 9223372036854775808.0 = pow(2, 63) exceeds INTMAX_MAX = 9223372036854775807, thus unsafe to cast, and -n < INTMAX_MIN = -9223372036854775808 (exactly in double) is falsy. Assuming INTX_MAX == -1-INTX_MIN, casting a double n overflows when n >= INTX_MAX+1 i.e. n >= -INTX_MIN i.e. -n <= INTX_MIN (the last one is only safe comparison in double).
There was a problem hiding this comment.
But INTMAX_MAX here gets promoted to a double, so it should be safe.
There was a problem hiding this comment.
Did you get a warning with UBSAN?
There was a problem hiding this comment.
No, I just noticed while renaming the macro (you were faster though). I originally referred to https://stackoverflow.com/a/60801229/1363488.
There was a problem hiding this comment.
Ok, so we can't represent INTMAX_MIN and INTMAX_MAX exactly as doubles, but then changing the comparison from < to <= won't make much difference. We're relying on the cast from double to intmax_t to be safe.
If we want to be extra careful we should probably have global read-only double constants that represent the smallest and largest integer that can be represented exactly in double, but being careful to allow double to not be an IEEE 754 64-bit double.
| @@ -235,7 +235,7 @@ static block constant_fold(block a, block b, int op) { | |||
| res = jv_number(na / nb); | |||
| break; | |||
| case '%': | |||
| #define is_unsafe_to_int_cast(n) (isnan(n) || (n) < INTMAX_MIN || -(n) < INTMAX_MIN) | |||
| #define is_unsafe_to_int_cast(n) (isnan(n) || (n) < INTMAX_MIN || -(n) <= INTMAX_MIN) | |||
I noticed that
pow(2, 63)is unsafe to cast to int. I was carefully flipped the sign, but failed to notice the boundary bug. Ref: #2797