Skip to content

fix: reject out-of-range integers when parsing JSON literals - #878

Open
LuciferYang wants to merge 2 commits into
apache:mainfrom
LuciferYang:fix-json-integer-overflow
Open

fix: reject out-of-range integers when parsing JSON literals#878
LuciferYang wants to merge 2 commits into
apache:mainfrom
LuciferYang:fix-json-integer-overflow

Conversation

@LuciferYang

@LuciferYang LuciferYang commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

What

LiteralFromJson accepted JSON integers beyond the signed 64-bit range and produced a wrong literal instead of a parse error. nlohmann reports unsigned integers as is_number_integer(), and get<int64_t>() converts values above INT64_MAX silently rather than throwing. So the kInt branch ran its int32 range check on the already-wrapped value, and the kLong branch had no range check at all: 18446744073709551615 parsed as Literal::Int(-1), and 9223372036854775808 as Literal::Long(INT64_MIN). The untyped overload had the same hole.

Table metadata is the path where this changes returned data. initial-default / write-default go through this parser, ValidateDefault does not check integer range, and the value later gets materialized into a column. A metadata file that Java rejects reads back as -1 here. The expression path is latent for now, since nothing evaluates ReaderOptions::filter yet.

Fixes #877.

How

GetInt64Checked rejects unsigned nodes above INT64_MAX before the conversion, and the kInt branch, the kLong branch and the untyped overload all go through it. Java guards the same paths with canConvertToInt() / canConvertToLong() in SingleValueParser and ExpressionParser.

The is_number_unsigned() half of the guard is load-bearing: get<uint64_t>() on a negative node yields its two's-complement value, which compares above INT64_MAX and would reject every negative literal.

Two smaller things on lines this fix already touches: the int32 narrowing on the kInt path gained the coverage it never had, and the two out-of-range messages now use the same wording instead of saying "int" in one and "long" in the other.

Testing

expression_test (526 tests) and the full ctest suite (18/18) pass. Each new test was checked against a mutation of the code it guards:

  • > to >= in the guard: only LongMax fails. The ULL suffix there matters, since a signed INT64_MAX node skips the unsigned branch entirely.
  • is_number_unsigned() && dropped: LongMin, IntNegative and AcceptsNegativeIntegerUntyped fail.
  • int32 range check deleted: IntAboveInt32Max and IntBelowInt32Min fail.

The three overflow-rejection cases were also confirmed to fail without the fix and pass with it.

Out of scope

GetTypedJsonValue in src/iceberg/util/json_util_internal.h truncates out-of-range integers the same way, so FieldFromJson({"id": 2147483648, ...}) yields field_id = -2147483648 silently. That helper has roughly 80 call sites, so it belongs in its own PR.

nlohmann reports unsigned integers as is_number_integer(), and get<int64_t>()
converts values above INT64_MAX silently instead of throwing. So an integer
beyond the signed range was accepted as a literal: the kInt branch ran its
int32 range check on the already-wrapped value, and the kLong branch had no
range check at all. 18446744073709551615 parsed as Literal::Int(-1) rather
than returning a parse error.

Add a GetInt64Checked helper that rejects unsigned values above INT64_MAX
before the conversion, and use it in the kInt and kLong branches of the
type-aware parser plus the untyped overload. This matches Java, where
SingleValueParser and ExpressionParser guard the same paths with
canConvertToInt()/canConvertToLong().
The out-of-range check is a conjunction: is_number_unsigned() plus a
comparison against INT64_MAX. Neither accept-side had a test, so dropping
either half went unnoticed. Add LongMax (an unsigned node exactly at
INT64_MAX, where the ULL suffix is load-bearing), LongMin and IntNegative
for the signed path, and an untyped negative case.

Also cover the int32 narrowing that follows the shared guard on the kInt
path, move the helper into an anonymous namespace so it stops taking an
external symbol, and use one wording for both out-of-range messages.
Copilot AI lite review requested due to automatic review settings August 7, 2026 16:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens JSON literal parsing in LiteralFromJson to correctly reject integral JSON values that exceed the signed 64-bit range, preventing silent wrap/truncation when converting nlohmann unsigned integer nodes to int64_t. This aligns C++ behavior with Java parsers and avoids cross-engine inconsistencies when reading table metadata defaults.

Changes:

  • Add GetInt64Checked to explicitly reject unsigned integer JSON nodes greater than INT64_MAX before converting to int64_t.
  • Route both typed (kInt, kLong) and untyped integral parsing through GetInt64Checked, and unify the out-of-range error wording.
  • Add regression tests covering unsigned overflow rejection, int32 narrowing, and negative-literal acceptance (typed and untyped).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/iceberg/expression/json_serde.cc Introduces GetInt64Checked and uses it in typed/untyped integral literal parsing to reject out-of-range unsigned integers.
src/iceberg/test/expression_json_test.cc Adds targeted test coverage for overflow rejection, boundary acceptance, and int32 narrowing behavior.

@LuciferYang

Copy link
Copy Markdown
Contributor Author

friendly ping @wgtmac

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.

bug: JSON literal parsing silently accepts integers beyond the signed 64-bit range

2 participants