Skip to content

Reject out-of-range integers instead of silently truncating - #112

Open
youdie006 wants to merge 1 commit into
treeform:masterfrom
youdie006:fix/integer-overflow-parse
Open

Reject out-of-range integers instead of silently truncating#112
youdie006 wants to merge 1 commit into
treeform:masterfrom
youdie006:fix/integer-overflow-parse

Conversation

@youdie006

Copy link
Copy Markdown

Summary

Fixes #109. Parsing an out-of-range integer silently wraps/truncates instead of raising.

"256".fromJson(uint8)  # => 0        (should be an error)
"128".fromJson(int8)   # => -128     (under -d:release; should be an error)

Root cause

parseHook for unsigned integers accumulates digits into a uint64 and converts with type(v)(v2). Nim's unsigned conversions wrap modulo with no range check even in debug, so "256".fromJson(uint8) returns 0 and very large numbers wrap silently.

The signed path wrapped the conversion in a try/except to raise "Number type to small to contain the number.", but Nim's range checks are disabled under -d:release/-d:danger, so over-large values wrapped there too. The negative branch had no check at all.

The signed path already intends to reject over-large numbers (that error message), so this makes the intent build-mode-independent and extends it to the unsigned path.

Fix

Range-check both paths explicitly against the target type's bounds, raising the existing error:

  • Unsigned: check each digit against high(type(v)) during accumulation (this also guards the uint64 accumulation itself against overflow).
  • Signed: replace the debug-only try/except with explicit bounds checks; the negative branch handles the extra low(T) value (e.g. int8 reaches -128).

No API/signature change; one comparison per number on the non-nimvm path.

Test

Added cases to tests/test_numbers.nim: out-of-range values (uint8/uint64/int8) now raise JsonError, and boundary values (255, 127, -128, 65535, uint64.high) still parse correctly.

  • Before the fix: nim r tests/tests.nim fails at doAssertRaises(JsonError): discard "256".fromJson(uint8) (nothing raised).
  • After the fix: all tests pass.

Verified locally with the repo's CI command nim r tests/tests.nim (default debug), which exercises the clean red→green for the unsigned path.


Disclosure: this fix was prepared with AI assistance (Claude). I reviewed it, traced the parse path, and verified the red-green test with the repo's own CI command myself.

parseHook for unsigned integers accumulated into a uint64 and converted
with `type(v)(v2)`, and Nim's unsigned conversions wrap modulo with no
range check even in debug, so `"256".fromJson(uint8)` returned 0 and huge
numbers wrapped silently. The signed path relied on a `try/except` around
the conversion, but Nim's range checks are disabled under -d:release, so
over-large values wrapped there too (e.g. `"128".fromJson(int8)` == -128).

Range-check both paths explicitly against the target type's bounds (the
unsigned check also guards the uint64 accumulation itself), raising the
existing "Number type to small to contain the number." error. The signed
negative branch handles the extra low(T) value (e.g. int8 reaches -128).

Fixes treeform#109
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integer overflow is unsafe and release-dependent

1 participant