LuciferYang opened a new issue, #877:
URL: https://github.com/apache/iceberg-cpp/issues/877
**Summary**
`LiteralFromJson` accepts JSON integers beyond the signed 64-bit range and
turns them into a wrong literal instead of returning a parse error. nlohmann
reports unsigned integers as `is_number_integer()`, and `get<int64_t>()`
converts a value above INT64_MAX silently rather than throwing, so:
- the `kInt` branch runs its int32 range check on the already-wrapped value,
and
- the `kLong` branch has no range check at all.
`18446744073709551615` parses as `Literal::Int(-1)`; `9223372036854775808`
parses as `Literal::Long(INT64_MIN)`. The untyped overload, which is the one
the REST expression parser actually reaches, has the same hole.
**Root Cause**
```cpp
case TypeId::kInt: {
if (!json.is_number_integer()) { // true for unsigned nodes too
return JsonParseError(...);
}
auto val = json.get<int64_t>(); // wraps silently above INT64_MAX
if (val < INT32_MIN || val > INT32_MAX) { // checks the wrapped value
return JsonParseError(...);
}
return Literal::Int(static_cast<int32_t>(val));
}
case TypeId::kLong:
if (!json.is_number_integer()) { ... }
return Literal::Long(json.get<int64_t>()); // no range check
```
Java guards the same paths explicitly: `SingleValueParser` uses
`canConvertToInt()` / `canConvertToLong()`, and `ExpressionParser.asObject`
uses `canConvertToLong()`, all of which throw on out-of-range input.
**Impact**
The live path today is table metadata: `initial-default` / `write-default`
go through the type-aware parser (`json_serde.cc` `FieldFromJson`),
`ValidateDefault` has no integer range check, and the value is later
materialized into a returned column via `MakeDefaultArray`. So a metadata file
with an out-of-range integer default reads back as `-1` in C++ while Java
rejects the file, which is an observable cross-engine difference.
The expression path is latent rather than live: a residual filter parsed
from a REST response reaches `ReaderOptions::filter`, but no reader evaluates
that field yet (`file_scan_task_reader.h` still has the TODO), and the
scan-planning response parsers aren't wired into `RestCatalog`. Worth fixing
now so the hole isn't inherited when evaluation does land.
Note this is a correctness/parser-hardening issue, not a security one:
`SECURITY-THREAT-MODEL.md` treats catalog-supplied metadata as trusted input.
**Proposed Fix**
Check the range before the conversion, mirroring `canConvertToLong()`:
```cpp
Result<int64_t> GetInt64Checked(const nlohmann::json& json) {
if (json.is_number_unsigned() &&
json.get<uint64_t>() >
static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) {
return JsonParseError("Cannot parse {} as an integer value: out of
range", ...);
}
return json.get<int64_t>();
}
```
and call it from the `kInt` branch, the `kLong` branch, and the untyped
overload. The `is_number_unsigned()` half matters: `get<uint64_t>()` on a
negative node yields its two's-complement value, which would compare above
INT64_MAX and reject every negative literal.
**Out of scope (follow-up)**
`GetTypedJsonValue` in `src/iceberg/util/json_util_internal.h` truncates
out-of-range integers the same way (nlohmann's `get<int32_t>` does not throw
either), so `FieldFromJson({"id": 2147483648, ...})` yields `field_id =
-2147483648` with no error. That helper has on the order of 80 call sites
across the repo and deserves its own PR.
I have a fix and regression tests ready and will open a PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
