univalue: respect token end pointer #35281

ferminquant commented at 2:25 PM on May 13, 2026: none

Fixes #28260.

Avoid reading past the supplied end pointer in two places:

matching JSON keywords with strncmp;
validating number prefixes with firstDigit[1].

Also update the UniValue unit test and parse_univalue fuzz target to exercise bounded input.

DrahtBot added the label RPC/REST/ZMQ on May 13, 2026

DrahtBot commented at 2:25 PM on May 13, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35281.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	sedited
Stale ACK	carloantinarella

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

ferminquant force-pushed on May 13, 2026

in src/test/fuzz/parse_univalue.cpp:22 in e9a2bbe3ef

  17 | @@ -18,7 +18,9 @@ void initialize_parse_univalue()
  18 |  
  19 |  FUZZ_TARGET(parse_univalue, .init = initialize_parse_univalue)
  20 |  {
  21 | -    const std::string random_string(buffer.begin(), buffer.end());
  22 | +    const char* const buffer_data{
  23 | +        buffer.empty() ? "" : reinterpret_cast<const char*>(buffer.data())};

maflcko commented at 3:56 PM on May 13, 2026:

why the ??

ferminquant commented at 1:16 AM on May 14, 2026:

You're right, it's not needed. I fixed it in a new commit.

DrahtBot added the label CI failed on May 13, 2026

DrahtBot commented at 4:06 PM on May 13, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/25806359727/job/75815150297</sub> <sub>LLM reason (✨ experimental): CI failed because the IWYU (include-what-you-use) check flagged missing/wrong includes and intentionally returned non-zero exit status (“Failure generated from IWYU”).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

ferminquant force-pushed on May 14, 2026

DrahtBot removed the label CI failed on May 14, 2026

DrahtBot added the label CI failed on May 21, 2026

DrahtBot removed the label CI failed on May 21, 2026

sedited requested review from maflcko on Jun 8, 2026

in src/univalue/test/unitester.cpp:218 in db49f974fe outdated

 213 | +    expect_json_token("false", /*size=*/5, JTOK_KW_FALSE, /*expected_consumed=*/5);
 214 | +
 215 | +    expect_json_token("-0", 1, JTOK_ERR);
 216 | +    expect_json_token("-x", 1, JTOK_ERR);
 217 | +    expect_json_token("01", /*size=*/1, JTOK_NUMBER, /*expected_consumed=*/1, "0");
 218 | +    expect_json_token("-01", /*size=*/2, JTOK_NUMBER, /*expected_consumed=*/2, "-0");

carloantinarella commented at 4:58 PM on June 8, 2026:

Would it be the case to add also few tests for fractions and exp numbers?

ferminquant commented at 5:56 PM on June 9, 2026:

Added in commit 28f609c7bb — eight tests covering fractions, exponents, and combined, using the same size/end-pointer pattern as the existing tests in this function.

carloantinarella commented at 8:01 PM on June 9, 2026: contributor

tACK 28f609c7bb78c233356470b9399c35b3183563a6

Thanks for the added tests. One comment: my suggestion is to squash the two commits into a single one, as they do not seem to represent different logical steps.

in src/univalue/lib/univalue_read.cpp:99 in 28f609c7bb

  94 | @@ -95,21 +95,23 @@ enum jtokentype getJsonToken(std::string& tokenVal, unsigned int& consumed,
  95 |  
  96 |      case 'n':
  97 |      case 't':
  98 | -    case 'f':
  99 | -        if (!strncmp(raw, "null", 4)) {
 100 | +    case 'f': {
 101 | +        const std::string_view raw_view{raw, static_cast<size_t>(end - raw)};

sedited commented at 10:58 AM on July 6, 2026:

I would prefer if this used the more straight forward std::string_view raw_view{raw, end} than doing pointer arithmetic.

in src/univalue/test/unitester.cpp:235 in 28f609c7bb

 230 | +    expect_json_token("-1e-5", /*size=*/5, JTOK_NUMBER, /*expected_consumed=*/5, "-1e-5");
 231 | +    expect_json_token("1e", /*size=*/2, JTOK_ERR);
 232 | +
 233 | +    // fractions + exponents combined
 234 | +    expect_json_token("1.5e3", /*size=*/5, JTOK_NUMBER, /*expected_consumed=*/5, "1.5e3");
 235 | +    expect_json_token("-1.5e-3", /*size=*/7, JTOK_NUMBER, /*expected_consumed=*/7, "-1.5e-3");

sedited commented at 11:22 AM on July 6, 2026:

Isn't this tested already in the fixture files?

in src/univalue/test/unitester.cpp:209 in db49f974fe

 204 | +
 205 | +void get_json_token_end_test()
 206 | +{
 207 | +    expect_json_token("null", 3, JTOK_ERR);
 208 | +    expect_json_token("true", 3, JTOK_ERR);
 209 | +    expect_json_token("false", 4, JTOK_ERR);

sedited commented at 11:24 AM on July 6, 2026:

Is this really worthwhile? Seems like these kind of cases are better covered in the fuzz test anyway.

in src/univalue/test/unitester.cpp:213 in db49f974fe

 208 | +    expect_json_token("true", 3, JTOK_ERR);
 209 | +    expect_json_token("false", 4, JTOK_ERR);
 210 | +
 211 | +    expect_json_token("null", /*size=*/4, JTOK_KW_NULL, /*expected_consumed=*/4);
 212 | +    expect_json_token("true", /*size=*/4, JTOK_KW_TRUE, /*expected_consumed=*/4);
 213 | +    expect_json_token("false", /*size=*/5, JTOK_KW_FALSE, /*expected_consumed=*/5);

sedited commented at 11:24 AM on July 6, 2026:

Isn't this covered in the test fixtures already?

sedited commented at 11:31 AM on July 6, 2026: contributor

Concept ACK

I think most of these unit tests are redundant however. Also not sure we should be introducing a new utility for this. I think the only cases actually adding a bit of coverage is the leading -01 case. Maybe that should just be covered by some more test fixture files?

EDIT:

It would also be good to mention that this is fixing two separate out of bounds reads.

sedited commented at 3:28 PM on August 4, 2026: contributor

@ferminquant can you respond to the review here? It's been a month.

in src/univalue/test/unitester.cpp:245 in 28f609c7bb outdated

 242 | @@ -194,6 +243,7 @@ int main(int argc, char* argv[])
 243 |  
 244 |      unescape_unicode_test();
 245 |      no_nul_test();

maflcko commented at 3:38 PM on August 4, 2026:

can be removed as well, given the fuzz test, and the other tests.

ferminquant commented at 9:28 PM on August 6, 2026: none

Thanks for the review. Done in commit 7f633b3beaa7778d0e2dcc87b9cd2baf5490c939:

Reduced the unit coverage to the minimal bounded-end regressions and removed the extra helper.
Updated the std::string_view construction.
Updated the PR description to mention the two independent out-of-bounds reads.
Squashed the two commits.

The focused UniValue test builds and passes locally.

ferminquant force-pushed on Aug 6, 2026

in src/univalue/test/unitester.cpp:203 in 7f633b3bea

maflcko commented at 6:12 AM on August 7, 2026:

This should be a string view on a buffer, like in the fuzz test

maflcko commented at 3:25 PM on August 7, 2026:

Well, this is adding back the null-byte

in src/univalue/test/unitester.cpp:198 in 7f633b3bea outdated

 193 | +
 194 | +    char number[] = "-01";
 195 | +    assert(val.read({number, 2}));
 196 | +    assert(val.isNum());
 197 | +    assert(val.getValStr() == "-0");
 198 | +}

maflcko commented at 6:16 AM on August 7, 2026:

Seems fine, but this should either be a correctly-sized buffer (without null-term), or it should be moved to a fixture file.

ferminquant force-pushed on Aug 7, 2026

ferminquant commented at 3:36 PM on August 7, 2026: none

Thanks, fixed in d107f950e8:

Kept fixture inputs as std::string_view without copying.
Removed no_nul_test().
Changed bounded_input_test() to use correctly sized, non-null-terminated buffers.

The focused UniValue test passes locally.

ferminquant force-pushed on Aug 7, 2026

univalue: respect token end pointer

Avoid reading past the supplied end pointer when matching JSON keywords and validating number prefixes.

Also update the UniValue test and parse_univalue fuzz target to exercise bounded input.

b9a54af80b

in src/univalue/test/unitester.cpp:71 in e40cccfda5

  65 | @@ -67,15 +66,14 @@
  66 |  #include <string_view>
  67 |  #include <tuple>
  68 |  
  69 | -static std::string rtrim(std::string s)
  70 | +static std::string_view rtrim(std::string_view s)
  71 |  {
  72 | -    s.erase(s.find_last_not_of(" \n\r\t") + 1);
  73 | -    return s;
  74 | +    return s.substr(0, s.find_last_not_of(" \n\r\t") + 1);

maflcko commented at 4:20 PM on August 7, 2026:

Either this needs to be copied into a fresh buffer (so that truly no data follows), or the tests need to be adjusted to not have trailing whitespace.

ferminquant force-pushed on Aug 7, 2026