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.

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

Fix constant folding of division and reminder with zero divisor #2797

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

itchyny
Copy link
Contributor

@itchyny itchyny commented Jul 31, 2023

Previously constant folding of zero division (e.x. 1/0) produces a compile error. This was incorrectly implemented by checking if the division result is infinite, so produces wrong results compared to the query where no constant folding is processed (e.x. 1e308/0.1). This patch delays the operation when the divisor is zero. This makes the results more consistent, but changes the exit code on zero division from 3 to 5. Also 0/0 now produces the zero division error, not NaN.

This patch also fixes the modulo operation. Previously constant folding logic does not take care of the % operator, but now it folds if the both dividend and divisor are safe numbers to cast to the integer type, and the divisor is not zero. This patch also fixes some code that relies on undefined cast behaviors in C. The modulo operation produces NaN if either the dividend or divisor is NaN.

Closes #2252, also closes #2780.

@itchyny itchyny added the bug label Jul 31, 2023
@itchyny itchyny added this to the 1.7 release milestone Jul 31, 2023
Previously constant folding of zero division (e.x. 1/0) produces a
compile error. This was incorrectly implemented by checking if the
division result is infinite, so produces wrong results compared to the
query where no constant folding is processed (e.x. 1e308/0.1). This
patch delays the operation when the divisor is zero. This makes the
results more consistent, but changes the exit code on zero division from
3 to 5. Also 0/0 now produces the zero division error, not NaN.

This patch also fixes the modulo operation. Previously constant folding
logic does not take care of the % operator, but now it folds if the both
dividend and divisor are safe numbers to cast to the integer type, and
the divisor is not zero. This patch also fixes some code that relies on
undefined cast behaviors in C. The modulo operation produces NaN if
either the dividend or divisor is NaN.
@@ -396,21 +396,30 @@ 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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, intmax_t... intmax_t has to be used very carefully, and... you are using it carefully. Good!

@nicowilliams
Copy link
Contributor

Looks good to me!

@nicowilliams nicowilliams requested a review from wader July 31, 2023 15:11
Copy link
Member

@wader wader left a comment

Choose a reason for hiding this comment

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

LGTM but i'm not very familiar with div/mod int safety

@nicowilliams nicowilliams merged commit 0f80921 into jqlang:master Jul 31, 2023
28 checks passed
@nicowilliams
Copy link
Contributor

Thanks!

@emanuele6
Copy link
Member

emanuele6 commented Jul 31, 2023

This should have fixed https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=60848

0/0 used to trigger an UBSAN warning when it was constant folded.

@emanuele6
Copy link
Member

I think you changed the name of your is_unsafe_double macro to is_unsafe_to_int_cast in src/parser.y, but you forgot to regenerate src/parser.c after that

@nicowilliams
Copy link
Contributor

I think you changed the name of your is_unsafe_double macro to is_unsafe_to_int_cast in src/parser.y, but you forgot to regenerate src/parser.c after that

Good catch!

I just pushed a re-gen in 90a6b2d.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants