Skip to content

reject out-of-range integers in JsonReader::ParseInt #16240

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.

Sign up for GitHub

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

Open
alhudz wants to merge 3 commits into
base: main
Choose a base branch
from
Open

reject out-of-range integers in JsonReader::ParseInt #16240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
29b2685
reject out-of-range integers in JsonReader::ParseInt
alhudz Jun 8, 2026
5e2228f
allow non-negative signed integers to decode into unsigned types
alhudz Jun 16, 2026
a1fdcc3
compute parse range bounds without narrowing IntType::max
alhudz Jun 29, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions Firestore/core/src/util/json_reader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#ifndef FIRESTORE_CORE_SRC_UTIL_JSON_READER_H_
#define FIRESTORE_CORE_SRC_UTIL_JSON_READER_H_

#include <limits>
#include <string>
#include <vector>

Expand Down Expand Up @@ -99,6 +100,40 @@ class JsonReader : public util::ReadContext {
template <typename IntType>
IntType ParseInt(const nlohmann::json& value, JsonReader& reader) {
if (value.is_number_integer()) {
// nlohmann stores integers as int64_t or uint64_t, so `get<IntType>()`
// silently wraps a value that does not fit `IntType`. Reject it instead,
// matching the range checking the string branch below gets from
// `absl::SimpleAtoi`.
bool out_of_range = false;
if (value.is_number_unsigned()) {
out_of_range =
value.get<uint64_t>() >
static_cast<uint64_t>(std::numeric_limits<IntType>::max());
} else {
// A non-negative value may still be stored as a signed `int64_t`, so
// accept it when it fits an unsigned `IntType` rather than rejecting
// every signed representation outright. Compute the bounds as
// `int64_t`/`uint64_t` so the comparisons never narrow `IntType::max()`
// into a signed type, which would warn for unsigned instantiations.
const int64_t val = value.get<int64_t>();
const int64_t min_limit =
std::numeric_limits<IntType>::is_signed
? static_cast<int64_t>(std::numeric_limits<IntType>::min())
: 0;
const uint64_t max_limit =
static_cast<uint64_t>(std::numeric_limits<IntType>::max());
if (std::numeric_limits<IntType>::is_signed) {
out_of_range = val < min_limit ||
(val > 0 && static_cast<uint64_t>(val) > max_limit);
} else {
out_of_range = val < 0 || static_cast<uint64_t>(val) > max_limit;
}
}
Comment on lines +108 to +131

@gemini-code-assist gemini-code-assist Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

low

The current implementation duplicates the error handling logic (reader.Fail(...) and return 0;) three times across different branches. Since ParseInt is a template function, this duplication is repeated for every instantiation of the template (e.g., for int32_t, uint32_t, int64_t, uint64_t), leading to unnecessary template code bloat and larger binary sizes.

We can refactor this by using a boolean flag out_of_range to track the validation status and perform a single error handling call at the end. This improves maintainability and reduces the compiled code size.

      bool out_of_range = false;
      if (value.is_number_unsigned()) {
        if (value.get<uint64_t>() >
            static_cast<uint64_t>(std::numeric_limits<IntType>::max())) {
          out_of_range = true;
        }
      } else {
        // A non-negative value may still be stored as a signed int64_t, so
        // accept it when it fits an unsigned IntType rather than rejecting
        // every signed representation outright.
        const int64_t val = value.get<int64_t>();
        if (std::numeric_limits<IntType>::is_signed) {
          if (val < static_cast<int64_t>(std::numeric_limits<IntType>::min()) ||
              val > static_cast<int64_t>(std::numeric_limits<IntType>::max())) {
            out_of_range = true;
          }
        } else if (val < 0 ||
                   static_cast<uint64_t>(val) >
                       static_cast<uint64_t>(
                           std::numeric_limits<IntType>::max())) {
          out_of_range = true;
        }
      }

      if (out_of_range) {
        reader.Fail("Integer value out of range: " + value.dump());
        return 0;
      }

@alhudz alhudz Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Folded into the same change, error handling is now a single out_of_range flag checked once instead of the three duplicated Fail/return blocks.

Comment on lines +108 to +131

@gemini-code-assist gemini-code-assist Bot Jun 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The current implementation of bounds checking can trigger compiler warnings (such as -Wconstant-conversion or -Woverflow) during template instantiation when IntType is unsigned. This happens because the compiler still compiles the signed branch where std::numeric_limits<IntType>::max() (which is uint64_t::max()) is cast to int64_t, resulting in an out-of-range constant conversion.

We can avoid these warnings and simplify the code by calculating the limits safely as int64_t and uint64_t based on the signedness of IntType, and then performing warning-free comparisons. This also eliminates the duplicated error-handling logic.

      bool out_of_range = false;
      if (value.is_number_unsigned()) {
        out_of_range = value.get<uint64_t>() >
                       static_cast<uint64_t>(std::numeric_limits<IntType>::max());
      } else {
        // A non-negative value may still be stored as a signed int64_t, so
        // accept it when it fits an unsigned IntType rather than rejecting
        // every signed representation outright.
        const int64_t val = value.get<int64_t>();
        const int64_t min_limit =
            std::numeric_limits<IntType>::is_signed
                ? static_cast<int64_t>(std::numeric_limits<IntType>::min())
                : 0;
        const uint64_t max_limit =
            static_cast<uint64_t>(std::numeric_limits<IntType>::max());

        if (std::numeric_limits<IntType>::is_signed) {
          out_of_range = val < min_limit ||
                         (val > 0 && static_cast<uint64_t>(val) > max_limit);
        } else {
          out_of_range = val < 0 || static_cast<uint64_t>(val) > max_limit;
        }
      }

      if (out_of_range) {
        reader.Fail("Integer value out of range: " + value.dump());
        return 0;
      }

@alhudz alhudz Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done. The bounds are now computed up front as min_limit/max_limit, so IntType::max() is never narrowed into int64_t and the -Wconstant-conversion path for unsigned instantiations is gone. Verified clean under -Wall -Wextra -Wconstant-conversion -Werror across int32_t/uint32_t/int64_t/uint64_t. Also folded the duplicated Fail calls into a single out_of_range check. Pushed in a1fdcc3.


if (out_of_range) {
reader.Fail("Integer value out of range: " + value.dump());
return 0;
}
Comment on lines +108 to +131

@gemini-code-assist gemini-code-assist Bot Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The current implementation unconditionally rejects parsing any signed integer representation into an unsigned IntType (e.g., parsing a positive integer stored as a signed int64_t into uint32_t). In nlohmann::json, integers can be stored as signed int64_t even if they are positive (for example, if constructed from a signed variable or initializer list).

Instead of failing immediately when !std::numeric_limits<IntType>::is_signed is true on the signed branch, we should allow non-negative values and check if they fit within the range of the unsigned IntType.

      if (value.is_number_unsigned()) {
        if (value.get<uint64_t>() >
            static_cast<uint64_t>(std::numeric_limits<IntType>::max())) {
          reader.Fail("Integer value out of range: " + value.dump());
          return 0;
        }
      } else {
        int64_t val = value.get<int64_t>();
        if (std::numeric_limits<IntType>::is_signed) {
          if (val < static_cast<int64_t>(std::numeric_limits<IntType>::min()) ||
              val > static_cast<int64_t>(std::numeric_limits<IntType>::max())) {
            reader.Fail("Integer value out of range: " + value.dump());
            return 0;
          }
        } else {
          if (val < 0 ||
              static_cast<uint64_t>(val) >
                  static_cast<uint64_t>(std::numeric_limits<IntType>::max())) {
            reader.Fail("Integer value out of range: " + value.dump());
            return 0;
          }
        }
      }

@alhudz alhudz Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Reworked the signed branch so a non-negative value stored as int64_t decodes when it fits an unsigned IntType (e.g. version/totalDocuments as uint32_t), instead of rejecting every signed representation. Negatives into unsigned and genuinely out-of-range values still fail. Pushed in 5e2228f.

return value.get<IntType>();
}

Expand Down
15 changes: 15 additions & 0 deletions Firestore/core/test/unit/bundle/bundle_serializer_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,21 @@ TEST_F(BundleSerializerTest, DecodesInvalidIntegerValueFails) {
VerifyJsonStringDecodeFails(json_copy);
}

TEST_F(BundleSerializerTest, DecodesOutOfRangeIntegerValueFails) {
ProtoValue value;
value.set_integer_value(22222);
ProtoDocument document = TestDocument(value);

std::string json_string;
MessageToJsonString(document, &json_string);

// A raw (unquoted) JSON number that does not fit int64 must be rejected
// rather than silently wrapped, the same way the string-encoded form is.
auto json_copy =
ReplacedCopy(json_string, "\"22222\"", "18446744073709551615");
VerifyJsonStringDecodeFails(json_copy);
}

TEST_F(BundleSerializerTest, DecodesDoubleValues) {
for (double_t v :
{-std::numeric_limits<double>::infinity(),
Expand Down
Loading