Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix overflow on integer casting on modulo operator #2806

Closed
wants to merge 1 commit into from
Closed

Fix overflow on integer casting on modulo operator #2806

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/builtin.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

@nicowilliams nicowilliams Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

But INTMAX_MAX here gets promoted to a double, so it should be safe.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you get a warning with UBSAN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just noticed while renaming the macro (you were faster though). I originally referred to https://stackoverflow.com/a/60801229/1363488.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

https://stackoverflow.com/questions/51104995/can-a-conversion-from-double-to-int-be-written-in-portable-c

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.

static jv f_mod(jq_state *jq, jv input, jv a, jv b) {
jv_free(input);
if (jv_get_kind(a) == JV_KIND_NUMBER && jv_get_kind(b) == JV_KIND_NUMBER) {
Expand Down
2 changes: 1 addition & 1 deletion src/parser.c
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion src/parser.y
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

if (is_unsafe_to_int_cast(na) || is_unsafe_to_int_cast(nb) || (intmax_t)nb == 0) return gen_noop();
#undef is_unsafe_to_int_cast
res = jv_number((intmax_t)na % (intmax_t)nb);
Expand Down