fix: range-check magnitude before the double->int cast in the JSON encoders (#816) - #855
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes undefined behavior in JSON number encoders by ensuring the range check is evaluated before narrowing a double to int, and strengthens the sanitizer gate to catch this class of UB in CI.
Changes:
- Reorders the JSON number “int fast-path” guard in
store_json_encodeand builtinjson_encodeso the magnitude check runs before the(int)cast. - Extends
make asanto compile with-fsanitize=float-cast-overflow. - Adds boundary-focused regression tests for JSON/store encoding at and beyond the
intrange, plus a changelog entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/ext_store.c |
Reorders numeric guard to avoid out-of-range double→int UB in store JSON encoding. |
src/builtins.c |
Applies the same guard reorder to the builtin JSON encoder. |
Makefile |
Adds float-cast-overflow to ASan/UBSan build flags to detect this UB class. |
tests/test_store.eigs |
Adds store round-trip boundary tests around int limits and huge magnitudes. |
tests/test_json_roundtrip.eigs |
Adds exact-output boundary tests for json_encode and a huge-number round-trip. |
CHANGELOG.md |
Documents the fix and the sanitizer gate enhancement under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * first. The bound is int's own range: integral values beyond | ||
| * it never took the %d path anyway (the equality failed), so | ||
| * output is unchanged and the cast is now always defined. */ | ||
| if (fabs(n) < 2147483648.0 && n == (int)n) |
| * first. The bound is int's own range: integral values beyond | ||
| * it never took the %d path anyway (the equality failed), so | ||
| * output is unchanged and the cast is now always defined. */ | ||
| if (fabs(n) < 2147483648.0 && n == (int)n) |
…coders (#816) store_json_encode and the builtin json_encode ran (int)n before the magnitude guard — UB for any number beyond int's range (C11 6.3.1.4p1; store_put of [db, {"n": 1e300}] reached it). Same class as #695's value_to_string fix. Output was correct only by hardware accident (x86-64 cvttsd2si -> INT_MIN -> equality fails -> %.15g). The guard now checks int's own range first (the old 1e15 bound never protected the cast and never mattered: integral values past 2^31 always failed the equality), so the cast is always defined and the encoded bytes are unchanged. make asan now also compiles with -fsanitize=float-cast-overflow — GCC's 'undefined' set does not include it, which is why the existing sanitizer gate was silent on this class. Both sites fire under the flag without the reorder; the full suite is clean with it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2572791 to
3f043ed
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/builtins.c:829
- Same as in ext_store.c: the guard is a 32-bit magic constant and it unintentionally excludes INT_MIN due to the
fabs(n) < 2147483648.0form. PreferINT_MIN/INT_MAXbounds to make the check portable and cover the fullintrange while still avoiding out-of-range casts.
if (fabs(n) < 2147483648.0 && n == (int)n)
src/ext_store.c:172
- The int-range guard is hard-coded to 2147483648.0 and (because it’s symmetric with
fabs) it excludes INT_MIN even though that cast is defined. UsingINT_MIN/INT_MAXavoids the magic number, works for non-32-bitint, and cleanly includes the full representable range while still guarding the cast.
if (fabs(n) < 2147483648.0 && n == (int)n)
…r) — caught by the new gate in CI The variant shape d == (double)(int)d escaped the pattern-grep re-audit; the float-cast-overflow sanitizer caught it in the asan-http CI leg (1.23e15 via test_json_hard, http-build-only path). Both sites reordered; json_hard green under make asan-http locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
CI's asan-http leg immediately proved the new gate's worth: it caught a third and fourth instance of the class that the pattern-grep re-audit missed — the variant shape |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/ext_store.c:172
- The new range guard hard-codes 2^31 via
fabs(n) < 2147483648.0. This is (1) less portable than usingINT_MIN/INT_MAX, and (2) it unintentionally excludesINT_MINitself (fabs(-2147483648.0) == 2147483648.0, so the%dpath can never take it). Prefer an explicit int-range check usingINT_MIN/INT_MAXbefore the cast.
if (fabs(n) < 2147483648.0 && n == (int)n)
src/builtins.c:829
- The new range guard hard-codes 2^31 via
fabs(n) < 2147483648.0. This is less portable than usingINT_MIN/INT_MAXand it excludesINT_MINitself (fabs(INT_MIN)equals 2^31), so the%dpath can never take that boundary. Prefer checkingINT_MIN/INT_MAXbefore performing the narrowing cast.
if (fabs(n) < 2147483648.0 && n == (int)n)
Closes #816
store_json_encode(src/ext_store.c) and the builtinjson_encode(src/builtins.c — same defect, found by the #731-style re-audit of the class) ran(int)nbefore the magnitude guard:n == (int)n && fabs(n) < 1e15. Converting a double beyond int's range is UB (C11 6.3.1.4p1), andstore_put of [db, {\"n\": 1e300}]reached it. Output was correct only by hardware accident (x86-64cvttsd2si→ INT_MIN → equality fails →%.15g). Same class as #695'svalue_to_stringfix.Fix: the guard checks int's own range first —
fabs(n) < 2147483648.0 && n == (int)n. The old 1e15 bound never protected the cast and never mattered (integral values past 2^31 always failed the equality on the hardware path), so the encoded bytes are unchanged; the cast is now always defined.Gate:
make asannow compiles with-fsanitize=float-cast-overflow— the issue's key observation is that GCC'sundefinedset does NOT include it, so the existing sanitizer gate was structurally blind to this class. Planted-fault verified: with the flag armed and the reorder reverted, BOTH sites reportruntime error: 2.14748e+09 is outside the range of representable values of type 'int'from the new boundary tests; with the fix, the full suite under ASan+UBSan+float-cast-overflow is clean (3726/3726, detect_leaks=1, zero reports — no other latent float-cast UB in suite reach).Tests: boundary pins in
tests/test_store.eigs(round-trip INT_MAX / 2^31 / -2^31 / ±1e300 through a store) andtests/test_json_roundtrip.eigs(encoded bytes at the same boundaries). Release suite 3728/3728.Closes #816
🤖 Generated with Claude Code