#35281 univalue: respect token end pointer

ReviewabilityReviewsAgreementSize
#35281 ferminquant univalue: respect token end pointerReady0 (+1)Blocked31 + 3 tests
  • Prevent out-of-bounds reads when parsing JSON inputs that lack null termination
  • Avoid potential node crashes when processing truncated or bounded JSON requests
  • Ready for review
  • Unaddressed objection: tests fail to properly isolate bounded inputs with trailing data (maflcko)
  • Concept approval because it fixes two separate out-of-bounds reads (sedited)
  • Verified by testing added coverage for fractions and exponents (carloantinarella)
  • 31 lines added or modified outside tests
  • 3 lines added or modified in tests
  • 30 lines removed in total
  • 10 files, 1 commits
RPC / REST / ZMQ#13 of 55
P3 · bug fix
  • P3 because it hardens RPC parsing against out-of-bounds reads on truncated input
  • RPC inputs are typically null-terminated in practice making this a preventative hardening fix
  • Ranking pass: Moved up 8: out-of-bounds reads in the JSON tokenizer used by every RPC request is the most security-adjacent item left in P3.
Ranking notes
Utilities (logging, arguments, libraries)#3 of 59
P1 · bug fix
  • P1 after comparing with the other PRs here (assessed alone as P2): Raised to P1: out-of-bounds reads in the shared JSON tokenizer are a memory-safety defect in a core parsing primitive, which this category ranks first.
  • P2 because it fixes memory-safety flaws in core JSON token parsing primitives
  • Ensuring parsers respect explicit buffer boundaries provides durable value across the codebase
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Enforces buffer end boundaries in UniValue's JSON tokenizer when matching keyword literals and inspecting number prefixes. Replaces `strncmp` keyword checks with `std::string_view::starts_with` and adds bounds checks before indexing into adjacent digit characters. Also updates the UniValue unit tests, roundtrip test data, and fuzz harness to exercise non-null-terminated string views.

Problem: UniValue's `getJsonToken` accepts explicit `raw` and `end` pointers to parse bounded string views without requiring null termination. In two places—matching `null`, `true`, and `false` via `strncmp`, and checking `firstDigit[1]` to reject leading zeros—the parser inspected bytes past `end` if the input terminated prematurely, causing out-of-bounds reads.

Evidence of importance:

  • Fixes #28260 ('getJsonToken assumes underlying string is null-terminated but requires end pointer')
  • PR description: 'Avoid reading past the supplied end pointer in two places: matching JSON keywords with strncmp; validating number prefixes with firstDigit[1].'

Scope: Modifies internal UniValue parsing code, strips trailing newlines from UniValue test JSON files to eliminate rtrim logic, and changes the parse_univalue fuzz target from std::string to std::string_view.

Risk of a mistake: medium (rpc_api, other). A flaw in JSON tokenization could incorrectly reject valid JSON or crash the node during RPC and REST request handling.

Changed since the previous assessment: Removed trailing whitespace trimming from the UniValue unit tester by stripping newlines from round-trip JSON test files.

Reviewability

Ready

Nothing known that would invalidate a review now.

Waiting on (from the thread read):

  • author: address maflcko's objection regarding test buffer isolation 2026-08-07
  • reviewer: sedited to re-review following requested rebase 2026-10-02

Agreement

Blocked

objection that needs a change, nobody has replied (maflcko)

Thread notes: sedited requested a rebase which ferminquant completed; maflcko's objection regarding bounded input test isolation remains open.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
maflcko (member)question, stylewhy the ? syntax is needed in parse_univalue fuzz targetfixed · fix pushed · author confirmed it was not needed and removed it in a commitansweredyes2026-05-13: 'why the `?`?'
Status from: 2026-05-14: ferminquant: 'You're right, it's not needed. I fixed it in a new commit.'
carloantinarella (contributor)suggestion, scopeadd tests for fractions and exponent numbersaccepted · fix pushed · author added requested tests and reviewer verified with tACKansweredyes2026-06-08: 'Would it be the case to add also few tests for fractions and exp numbers?'
Status from: 2026-06-09: carloantinarella: 'Thanks for the added tests.'
sedited (member)suggestion, maintenancereduce redundant unit tests and avoid introducing a new helper utilityfixed · fix pushed · author reduced unit tests to minimal regressions and removed the helperansweredyes2026-07-06: 'I think most of these unit tests are redundant however. Also not sure we should be introducing a new utility for this.'
Status from: 2026-08-06: ferminquant: 'Reduced the unit coverage to the minimal bounded-end regressions and removed the extra helper.'
sedited (member)suggestion, approachuse std::string_view raw_view{raw, end} instead of pointer arithmeticfixed · fix pushed · author updated std::string_view construction in pushansweredyes2026-07-06: 'I would prefer if this used the more straight forward `std::string_view raw_view{raw, end}` than doing pointer arithmetic.'
Status from: 2026-08-06: ferminquant: 'Updated the std::string_view construction.'
maflcko (member)suggestion, maintenanceremove test as it is covered by the fuzz test and other testsfixed · fix pushed · author removed redundant test coverageansweredno2026-08-04: 'can be removed as well, given the fuzz test, and the other tests.'
Status from: 2026-08-06: ferminquant: 'Reduced the unit coverage to the minimal bounded-end regressions and removed the extra helper.'
maflcko (member)suggestion, approachuse a string view on a buffer instead of copyingfixed · fix pushed · author kept fixture inputs as std::string_view without copyingansweredyes2026-08-07: 'This should be a string view on a buffer, like in the fuzz test'
Status from: 2026-08-07: ferminquant: 'Kept fixture inputs as std::string_view without copying.'
maflcko (member)suggestion, approachuse a correctly-sized buffer without null-termination or move to a fixture filefixed · fix pushed · author updated bounded_input_test to use non-null-terminated buffersansweredyes2026-08-07: 'Seems fine, but this should either be a correctly-sized buffer (without null-term), or it should be moved to a fixture file.'
Status from: 2026-08-07: ferminquant: 'Changed bounded_input_test() to use correctly sized, non-null-terminated buffers.'
maflcko (member)objection, correctnesstest is adding back a null-byte, defeating non-null-terminated buffer testingfixed · fix pushed · author removed no_nul_test()changedyes2026-08-07: 'Well, this is adding back the null-byte'
Status from: 2026-08-07: ferminquant: 'Removed no_nul_test().'
maflcko (member)objection, correctnesstests do not properly isolate bounded input if trailing data or whitespace is presentopenchangedno2026-08-07: '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.'

Support:

  • carloantinarella (contributor) (Tested ACK, 2026-06-09) [tested]: tested after additional coverage for fractions and exponents was added
  • sedited (member) (Concept ACK, 2026-07-06) [read]: fixes two separate out of bounds reads

Participants:

  • maflcko (member, objection, 6 statements 2026-05-13 to 2026-08-07): concerned tests do not properly isolate bounded input if trailing data or whitespace is present
  • carloantinarella (contributor, support, 2 statements 2026-06-08 to 2026-06-09): Tested ACK after fractions and exponent tests were added
  • sedited (member, support, 7 statements 2026-07-06 to 2026-10-02): Concept ACK for fixing out-of-bounds reads; suggested dropping redundant tests and requested a rebase

Files

5 lines under test/bench/ci.

  • src/univalue/test/unitester.cpp +13/-15
  • src/univalue/lib/univalue_read.cpp +11/-6
  • src/test/fuzz/parse_univalue.cpp +3/-2
  • src/univalue/test/round1.json +1/-1
  • src/univalue/test/round2.json +1/-1
  • src/univalue/test/round3.json +1/-1
  • src/univalue/test/round4.json +1/-1
  • src/univalue/test/round5.json +1/-1
  • src/univalue/test/round6.json +1/-1
  • src/univalue/test/round7.json +1/-1

Card

The short text the ranking pass compares across PRs.

Fixes out-of-bounds reads in UniValue's JSON tokenizer by respecting the buffer end pointer during keyword matching and number validation. Previously, parsing non-null-terminated string views could read past the buffer end when encountering truncated keywords or bare minus/zero prefixes. This resolves issue #28260 and updates UniValue unit and fuzz tests to exercise bounded string views.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-04T01:17, confidence high, input hash 74a3f8445f8b5155
thread read through 2026-10-02 (20 statements), head 045b0ef7b4
labels: RPC/REST/ZMQ

Categories

RPC / REST / ZMQ

P3 · bug fix (ranking pass: Moved up 8: out-of-bounds reads in the JSON tokenizer used by every RPC request is the most security-adjacent item left in P3.)

Fixes out-of-bounds reads in JSON parsing when matching keywords or digit prefixes without null termination (#28260). Because RPC inputs are typically null-terminated strings in practice, this provides a worthwhile hardening fix rather than resolving an active failure.

Membership: Carries the RPC/REST/ZMQ label and fixes token parsing in the UniValue JSON library used by RPC interfaces.

Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 0

Utilities (logging, arguments, libraries)

P1 · bug fix (assessed alone as P2; Raised to P1: out-of-bounds reads in the shared JSON tokenizer are a memory-safety defect in a core parsing primitive, which this category ranks first.)

Enforces buffer boundaries in UniValue's getJsonToken to eliminate two out-of-bounds reads when processing non-null-terminated string views (#28260). Correctness and memory-safety fixes in vendored parsing primitives carry durable value across the codebase.

Membership: Directly modifies src/univalue/lib/univalue_read.cpp, part of the vendored UniValue library covered by utilities.

Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 0