Bound every registry read to its buffer rather than a null terminator - #2732
Merged
Conversation
The registry parses bytes it does not own: a wire span handed to
registry::call(std::span<const char>, std::string&), a raw pointer and length
handed to repe::plugin_call across an FFI boundary, a std::string_view handed to
the JSON RPC call. None of those carry the '\0' sentinel a null_terminated read
relies on when it drops its end checks, yet the registry read with that option
on, because it defaults on and no caller can reach it. Three reads over the
caller's buffer ran past its end. Each one is a heap-buffer-overflow on a
well formed request, not a malformed one:
registry.call(span_over_exact_sized_buffer{"/value" body "7"}, response)
-> READ of size 1, 0 bytes after a 55 byte region (repe read_params)
registry<opts{}, JSONRPC>.call("[7")
-> READ of size 1, 0 bytes after a 52 byte region (batch read_json)
registry<opts{}, JSONRPC>.call(truncated_object)
-> READ of size 1, 0 bytes after a 52 byte region (validate_json)
The registry now reads with null_terminated off no matter what the user asked
for, since the buffer shape is known here and not at the call sites that hand it
bytes. Two of the three reads above reached glz::read_json and glz::validate_json
directly, which hardcode default options, so those move onto the registry's
options; get_as_json and get_sv_json took an Opts parameter and then discarded it
in favor of defaults, which is fixed with them.
Reading unterminated input trades the sentinel scan for per-byte end checks,
which costs about 9% end to end on a 120 byte REPE object body and 18% on a
parse in isolation. That is the price of the bound.
Two defects surfaced with the change and are fixed with it.
repe::read_params returned bytes consumed and every caller read 0 as failure.
Without a terminator a completed read can consume nothing: a variant alternative
that resolves at the end of the buffer rewinds its iterator, so the read reports
zero bytes with the value applied. The registry took that for an error and
returned without writing anything, so a non-notify request got no response at all
and its client waited forever:
struct api { std::variant<std::string, amount> measure{}; };
// body "42.5", buffer ends there -> response buffer empty, value applied
read_params returns bool now. The count was never used for anything else.
A buffer holding only whitespace was read as a completed value. Without a
terminator the reader runs out of input looking for a value and reports
end_reached, which at the top level otherwise means the value ended with the
buffer, so glz::read cleared it and returned success with the destination
untouched. glz::read<{.null_terminated = false}>(v, " ") did this before this
change; through the registry it would have answered a malformed request with a
success response. Whitespace with no value in it is now no_read_input, the same
code an empty buffer reports.
The out of bounds read in the REPE zero-copy path and the test that covers it are
from #2729 by @uwezkhan, which this supersedes.
Three follow-ups to the registry read bound. The whitespace check missed comments. `consumed_only_whitespace` tested for ` \t\n\r`, so with `.comments = true` a comment-only buffer still reported success with the destination untouched -- the same defect, one option away, and reachable through a registry now that the registry always reads without a terminator. The check is now `finalize_top_level_read`, which replays `skip_ws` over the consumed span instead of holding a second opinion about which bytes count as whitespace. Since the span ends where the buffer ends, that replay is an exact re-run of the parse's own leading `skip_ws`. It stays after the parse so readers that take the buffer verbatim (`glz::text`, `glz::raw_json`) still see every byte the caller passed; they complete without error and never reach it. The three copies of the condition collapse into that one helper. The registry bound could be silently skipped. `registry_read_opts` turns `null_terminated` off through a requires-expression, which an options struct that fixes the member (`static constexpr bool null_terminated = true`, the shape `opts_csv` uses) fails -- leaving the registry reading past the caller's buffer with no diagnostic. A `static_assert` makes it a compile error. The JSON RPC syntax check ran under the wrong options. It hardcoded the defaults `validate_json` is fixed to, so a registry configured with comments or relaxed key handling validated by different rules than it read by, and that comparison decides -32700 vs -32600. It now extends the registry's options. Also parse with `is_padded_off<Opts>()` in `read_params`, mirroring `glz::read`, so a path that bypasses `glz::read` does not inherit the padding assumption from whatever the caller passed. `read_streaming` has the same absent-value gap and forces `null_terminated` off, but refills relocate and consume the buffer, so the parsed bytes are no longer addressable as a span by the time the read returns. Its behavior is unchanged and the reason is now written down at the call site.
`read_streaming` was the one top-level entry point left unbounded. It forces `null_terminated` off, so a stream holding nothing but whitespace or comments ended in the same `end_reached` a value ending with the buffer does, and `finalize_read_context` cleared it -- returning success over an untouched destination. The reason given for leaving it alone was wrong. Refills were said to make the parsed bytes unaddressable, but `consume_and_refill` is only ever called inside array and object element loops; nothing refills while the top level skips leading whitespace. A stream that held no value never leaves its first window, so the span is exactly what the parse walked. The origin comes from the streaming state rather than an iterator captured before the parse, since a refill relocates the buffer. Only an exhausted stream can be said to have held no value, so the check runs after `consume_buffer` and behind `at_eof`, which also requires the window to be drained. A stream that still has data ran out of window instead: whitespace wider than the window stops the parse short of a value that is really there. That is a separate limitation of streaming, and it keeps the behavior it has always had rather than being reported as an absent value. Both halves are pinned by tests. `finalize_top_level_read` now requires `start < it` rather than a non-empty span. A streaming origin can also overshoot on an error path, which `!=` did not catch. The guard is a no-op for buffered reads, where an empty buffer is rejected before the helper.
Same content, half the lines. The two conditions that are load-bearing rather than defensive are now a list instead of prose, and the sentence noting that only a non-null-terminated JSON read reaches any of this is dropped -- the if constexpr directly above says so.
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #2729, whose diagnosis and REPE test this builds on (thanks @uwezkhan).
The registry parses bytes it does not own — a wire span given to
registry::call(std::span<const char>, std::string&), a raw pointer and length given torepe::plugin_callacross an FFI boundary, astd::string_viewgiven to the JSON RPCcall. None of them carry the'\0'anull_terminatedread relies on when it drops its end checks, and no caller can reach the option to turn it off. Three reads over the caller's buffer ran past its end, each on a well formed request:registry::call(span, string&)7, buffer ends thererepe.hppread_params)registry<opts{}, JSONRPC>::call(sv)[7registry.hpp:471, batchread_json)registry<opts{}, JSONRPC>::call(sv)registry.hpp:496,validate_json)#2729 bounds the first. This bounds all three by making it a property of the registry rather than of one call site: the registry reads with
null_terminatedoff regardless of the user's options. Two of those reads went throughglz::read_json/glz::validate_json, which hardcode default options, so they move onto the registry's options.get_as_json/get_sv_jsontook anOptsparameter and then discarded it in favor of defaults; fixed here too.Because that bound is what keeps the registry inside the caller's buffer, an options struct that fixes
null_terminated(static constexpr bool null_terminated = true, the shapeopts_csvuses and the "create your own options struct" note invites) must not quietly opt out of it. The requires-expression that turns the option off simply fails for such a type, so astatic_assertturns that into a compile error rather than a silent out of bounds read.Two defects the change surfaced
A completed read could report zero bytes consumed.
repe::read_paramsreturned bytes consumed and all six callers read0as failure. Without a terminator, a variant alternative that resolves at the end of the buffer rewinds its iterator, so the read finishes with the value applied and nothing consumed. The registry took that for an error and returned without writing anything — a non-notify request got no response at all:read_paramsreturnsboolnow; the count had no other use. This bites any fix that turnsnull_terminatedoff, including #2729 as written.Input holding no value read as a completed value. The reader runs out of input looking for a value and reports
end_reached, which at the top level otherwise means "the value ended with the buffer", soglz::readcleared it and returned success with the destination untouched —glz::read<{.null_terminated = false}>(v, " ")does this onmain, and so doesglz::read<{.null_terminated = false, .comments = true}>(v, "// hi\n"). Through the registry that would answer a malformed request with a success response, and would have turned a RESTPUTwith a blank body from 400 into 204. A buffer holding nothing but whitespace and comments is nowno_read_input, the code an empty buffer already reports.finalize_top_level_readdraws that line, in one place forglz::readand bothread_paramsoverloads. It answers "did this hold a value?" by replayingskip_wsover the consumed span rather than by holding a second opinion about which bytes count as whitespace — and since the span ends where the buffer ends, that replay is an exact re-run of the parse's own leadingskip_wsunder the same options. Two constraints shape where it sits:glz::text,glz::text_view,glz::raw_json) capture bytes straight off the iterator, so nothing may consume input ahead of them; they complete without error and never reach the check.it == endguard. A variant alternative resolved by shape rewinds its iterator once it settles, so a successful read can end withend_reached, depth 0, and a consumed span that is nothing but the whitespace ahead of the value it applied. Without the guard that read would be rejected.read_streamingis bounded too, so every top-level entry point is. It forcesnull_terminatedoff, so it had the same defect. Refills looked like an obstacle — they relocate the buffer — butconsume_and_refillis only ever reached inside array and object element loops, so nothing refills while the top level skips leading whitespace and a stream that held no value never leaves its first window. The span origin comes from the streaming state rather than from an iterator captured before the parse.That last property cuts both ways, so the streaming check is narrower than the buffered one: it runs behind
at_eof, because only an exhausted stream can be said to have held no value. Leading whitespace wider than the window also stops the parse short — of a value that is really there — and that case keeps the behavior it has always had rather than being reported as an absent value. Both halves are pinned by tests.Cost
Measured,
-O2 -DNDEBUG, interleaved runs:registry::callspan, 120 byte object bodyregistry::callspan, int bodyglz::readinto astd::stringbody, parse onlyThe REPE figures match #2729 — the extra coverage is free there. The REST path is the one that pays for a bound it does not strictly need today, since
request::bodyis astd::string. I would not exempt it: "which registry buffers happen to bestd::string" is exactly the invariant that rotted here, sincestate_view's body used to be one and is now a wire span.Nothing outside a non-null-terminated JSON read pays anything. The whole check is behind
if constexpr (Opts.format == JSON && !check_null_terminated(Opts)), so the default read path compiles it away entirely.Tests
New tests in
registry_view_test(REPE spans over exactly sized buffers: scalar, object, truncated, whitespace-only, comment-only, both variant shapes, and theplugin_callFFI entry point),jsonrpc_registry_test(exactly sized requests: valid, truncated batch, truncated object, bare scalar),json_test(whitespace-only, comment-only and malformed-comment input undernull_terminated = false, which also runs in theGLZ_NULL_TERMINATED=falsebuild of that file), andistream_buffer_test(streams holding no value, a value read across refills, and leading whitespace wider than the window). Both networking test files trip ASAN againstmainas written and pass here. Full suite is 108/108 under-fsanitize=address,undefined -fno-sanitize-recover=all.Follow-up, not in this PR
registry.callfrom exactly sized buffers would have found all three of these and would cover the whole wire-facing surface.read_jsoncon a streaming buffer reads out of bounds. It has nois_input_streamingoverload, unlikeread_json, so it binds to the bufferedglz::readwithnull_terminated = trueover a window that carries no terminator and cannot be padded (istream_bufferis not resizable). ASAN reports a heap-buffer-overflow one byte past the window. This is the same defect class as this PR reached through a different door, and the missing-overload shape applies to the other per-format helpers as well, so it wants a systematic fix rather than a one-off. Left out of this PR to keep the change focused, but it is more severe than anything fixed here.read_streamingignoresvalidate_trailing_whitespace; that block lives only inglz::read.no_read_input.skip_commentruns off the end without flagging it, so that is the honest answer available here, and it beats the silent successmainreturns. A real fix belongs inskip_commentand would change the null-terminated path too.