#35281 univalue: respect token end pointer
https://github.com/bitcoin/bitcoin/pull/35281 · · +34/-31 in 10 files, 1 commits · labels: RPC/REST/ZMQ, Needs rebase
Goal
- Prevent out-of-bounds memory reads when parsing JSON inputs that lack null terminators
- Protect node operators and callers from memory safety bugs during bounded text parsing
Fixes two out-of-bounds read bugs in UniValue's getJsonToken parser. Specifically, it stops reading past the token end pointer when matching keyword tokens ('null', 'true', 'false') and when checking for leading zeros in number tokens. It also updates the parse_univalue fuzz target and unit tests to exercise bounded, non-null-terminated string views.
Problem: UniValue's getJsonToken assumed underlying strings were null-terminated despite receiving an end pointer, leading to out-of-bounds memory reads when parsing string_view buffers that are not null-terminated.
Category: Utilities (logging, arguments, libraries) (#23 of 66)
P3 · bug fix
- P3 because it fixes two out-of-bounds memory safety bugs in shared utility parsing code
- Restricts benefits to callers passing non-null-terminated string views
P3 because it fixes out-of-bounds read bugs when parsing bounded string_view inputs in the UniValue utility library. While memory safety bugs can be severe, standard RPC strings in Bitcoin Core are null-terminated, so this primarily fixes edge cases under string_view usage rather than an active crash vulnerability.
Membership: Modifies UniValue, a vendored subtree covered under utils
Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 1
Category: RPC / REST / ZMQ (#49 of 52)
P4 · bug fix
- P3 because production exposure is limited since RPC handlers typically pass null-terminated buffers
- Improves parsing safety for the RPC layer when handling bounded string inputs
P4 because no RPC interface, command, serialization schema, or dispatch behavior is modified. The fix applies to the underlying JSON library.
Membership: Carries the RPC/REST/ZMQ maintainer label because UniValue is the JSON engine used across RPC
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 0
Reviewability: Stale: Needs rebase
- Needs rebase due to merge conflicts with the base branch
- Author has been inactive for over a month
The PR has merge conflicts with master and requires a rebase.
Author status: silent since 2026-08-07 after addressing test feedback; needs to rebase
Resolved concerns:
- sedited requested using direct std::string_view constructor syntax rather than pointer arithmetic and removing redundant unit test cases.
- carloantinarella suggested adding numeric tests, and maflcko gave feedback on ensuring test buffers truly lack null termination.
Agreement: Mild
- General agreement on fixing the out-of-bounds memory reads in JSON parsing
- Concept approval noting it fixes two distinct out-of-bounds reads (sedited)
- Verified by testing bounded input test cases (carloantinarella)
- Suggested minor test cleanup around buffer handling (maflcko)
Mild: nonblocking objection open (maflcko)
sedited provided a Concept ACK endorsing the fix for two separate out-of-bounds reads, carloantinarella tested the changes, and no participant opposes the approach.
- sedited gave Concept ACK noting it fixes two separate out of bounds reads
- carloantinarella gave tACK on 2026-06-09
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| sedited | maintenance | redundant unit test fixtures and unnecessary helper utility in unitester | resolved | no | yes | 2026-07-06: 'I think most of these unit tests are redundant however. Also not sure we should be introducing a new utility for this.' Settled: 2026-08-06: 'Reduced the unit coverage to the minimal bounded-end regressions and removed the extra helper.' |
| maflcko | correctness | tests do not properly isolate bounded input if trailing data or whitespace is present | open | no | no | 2026-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:
- sedited: fixing two separate out of bounds reads
- carloantinarella: tested after additional test coverage added
Participants: maflcko (objection), carloantinarella (support), sedited (support)
State derived from the lists: nonblocking objection open (maflcko) (model's own read: Strong)
Review verdicts (DrahtBot): 0 (+1)
- Stale ACK: carloantinarella
- Concept ACK: sedited
Files
5 lines under test/bench/ci.
- src/univalue/test/unitester.cpp +13/-15
- src/univalue/lib/univalue_read.cpp +11/-7
- 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
This PR fixes two out-of-bounds read bugs in the UniValue parser where token parsing previously assumed null termination instead of respecting the supplied end pointer. It resolves issue #28260 and updates the univalue fuzz target and unit tests to exercise non-null-terminated buffers. There is solid concept support from sedited and carloantinarella, with test comments addressed. Review is currently blocked by merge conflicts requiring a rebase.