Skip to content

io/tokenizer: parse string escapes with bounds-checked indexing - #29220

Open
DarkLycn1976 wants to merge 1 commit into
protocolbuffers:mainfrom
DarkLycn1976:pr-tokenizer-escapes
Open

io/tokenizer: parse string escapes with bounds-checked indexing#29220
DarkLycn1976 wants to merge 1 commit into
protocolbuffers:mainfrom
DarkLycn1976:pr-tokenizer-escapes

Conversation

@DarkLycn1976

Copy link
Copy Markdown

What

ReadHexDigits, FetchUnicodePoint, and Tokenizer::ParseStringAppend decode
escape sequences (\ooo, \xHH, \uXXXX, \UXXXXXXXX, and \u UTF-16
surrogate pairs) by walking a raw const char* with pointer arithmetic and
one-past-the-current lookahead (ptr[1], *(p + 1), p += len). Those reads
stay 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_view through a small
CharAt() helper that returns '\0' for out-of-range indices. There is no
functional 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, truncated
sequences, 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:

  • The existing tokenizer and text-format unit tests pass unchanged
    (*Tokenizer*, *TextFormat*).
  • A standalone differential harness runs the original pointer-based code and the
    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 and
the numeric parsers can follow separately.

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>
@google-cla

google-cla Bot commented Aug 17, 2026

Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant