io/tokenizer: parse string escapes with bounds-checked indexing - #29220
Open
DarkLycn1976 wants to merge 1 commit into
Open
io/tokenizer: parse string escapes with bounds-checked indexing#29220DarkLycn1976 wants to merge 1 commit into
DarkLycn1976 wants to merge 1 commit into
Conversation
ReadHexDigits, FetchUnicodePoint and Tokenizer::ParseStringAppend decoded escape sequences from attacker-controllable text (protobuf text format, via TextFormat::ParseFromString) by walking a raw const char* with pointer arithmetic and one-past lookahead (ptr[1], *(p+1), p += len). The accesses were in bounds only by the input's NUL terminator and by the order of the short-circuit checks. Rewrite the three routines to index an absl::string_view through a small CharAt() helper that returns '\0' for out-of-range indices. Behaviour is unchanged (verified by differential testing over 20M random inputs and the existing tokenizer/text-format unit tests) but the escape-decoding path no longer relies on NUL-termination or raw pointer arithmetic. This removes 21 -Wunsafe-buffer-usage findings from the file, a step toward adopting the Safe Buffers programming model for the tokenizer. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
What
ReadHexDigits,FetchUnicodePoint, andTokenizer::ParseStringAppenddecodeescape sequences (
\ooo,\xHH,\uXXXX,\UXXXXXXXX, and\uUTF-16surrogate pairs) by walking a raw
const char*with pointer arithmetic andone-past-the-current lookahead (
ptr[1],*(p + 1),p += len). Those readsstay in bounds only because the input happens to be NUL-terminated and because
of the order of the short-circuit checks.
This reworks the three routines to index an
absl::string_viewthrough a smallCharAt()helper that returns'\0'for out-of-range indices. There is nofunctional change — it just removes the raw pointer arithmetic and the reliance
on NUL-termination from the escape-decoding path, which runs on
externally-supplied text via
TextFormat::ParseFromString.Why
The escape parsers are the part of the tokenizer that consumes the most
attacker-influenced structure (arbitrary
\x/\u/octal runs, truncatedsequences, lone surrogates). Expressing them with bounds-checked indexing rather
than pointer walking makes them easier to reason about and removes a class of
easy-to-reintroduce off-by-one mistakes. Under
-Wunsafe-buffer-usage(Clang)the file drops from 263 to 242 findings; the 21 removed are exactly these
escape-parser sites.
Behavior preservation
Verified two ways:
(
*Tokenizer*,*TextFormat*).new index-based code side by side and compares output byte-for-byte over
20,000,000 randomized inputs (quotes, backslashes,
\u/\U/\x/octal,surrogate pairs, truncated escapes, embedded NULs, mismatched quotes) plus a
set of targeted edge cases. Zero divergences.
Notes
No behavior or performance change is intended; the tokenizer is not on the
wire-format decoding path. This is a self-contained step toward adopting the
Safe Buffers programming model in
io/tokenizer; the per-character scanner andthe numeric parsers can follow separately.