Skip to content

fix(json_reporter): escape C0 control characters per RFC 8259 - #2358

Open
Aryan9891 wants to merge 1 commit into
google:mainfrom
Aryan9891:fix/json-reporter-c0-escape-2299
Open

Aryan9891 wants to merge 1 commit into
google:mainfrom
Aryan9891:fix/json-reporter-c0-escape-2299

Conversation

@Aryan9891

Copy link
Copy Markdown

Problem

As noted in Note 2 of #2299, RFC 8259 Section 7 requires characters U+0000 through U+001F inside JSON strings to be escaped. In src/json_reporter.cc, StrEscape explicitly escapes \b \f \n \r \t \" \\ but allows other C0 control characters (e.g. U+0001 or ANSI escape characters like U+001B in benchmark names or labels) to pass through unescaped, causing strict JSON parsers to reject the output.

Solution

In StrEscape, escape any remaining byte in the C0 control range (< 0x20) as \u00XX via std::snprintf. Printable characters and UTF-8 multibyte sequences remain unaffected.

Fixes #2299.

Comment thread src/json_reporter.cc Outdated
break;
default:
tmp += c;
if (static_cast<unsigned char>(c) < 0x20) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we slightly simplify this with auto uc = static_cast<unsigned char>(c); which can then be reused in the snprintf below?

@dxnnystyles dxnnystyles left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like a good fix! it'd be worth adding a regression test with something like \x00 and \x1f in a benchmark name, then checking that the generated report is valid JSON and the characters survive decoding. that would protect the behavior this PR is fixing.

…#2299)

RFC 8259 requires characters U+0000 through U+001F in JSON strings to be
escaped. While common escapes (\b, \f, \n, \r, \t) are already handled,
other control bytes below 0x20 were previously passed through raw.
Escape all remaining C0 control bytes as \u00XX and add unit tests
verifying C0 control byte escaping in JSON reporter list output.

Fixes google#2299.

Signed-off-by: Aryan Sachin <aryansachinixd@gmail.com>
@Aryan9891
Aryan9891 force-pushed the fix/json-reporter-c0-escape-2299 branch from bec54bc to c200398 Compare October 30, 2026 21:37
@Aryan9891

Copy link
Copy Markdown
Author

Thanks for the review and suggestions! I have updated the PR to address your feedback:

  • Simplified the C0 control character handler using \�uto uc = static_cast(c);\ and reused it directly in \std::snprintf.
  • Added unit tests in \ est/reporter_list_gtest.cc\ verifying that C0 control characters (\x01, \x1f) in benchmark names are escaped as \u0001\ and \u001f\ in the JSON output per RFC 8259.
  • Rebased cleanly onto the latest \main\ branch.

@dxnnystyles

Copy link
Copy Markdown

thanke! I checked the revised escaping logic + regression test. the new assertions cover the expected \u0001 and \u001f output, and the unsigned-char handling looks good. that addresses my original concern 👍

@dmah42

dmah42 commented Nov 2, 2026

Copy link
Copy Markdown
Member

Thanks for the review and suggestions! I have updated the PR to address your feedback:

  • Simplified the C0 control character handler using \�uto uc = static_cast(c);\ and reused it directly in \std::snprintf.
  • Added unit tests in \ est/reporter_list_gtest.cc\ verifying that C0 control characters (\x01, \x1f) in benchmark names are escaped as \u0001\ and \u001f\ in the JSON output per RFC 8259.
  • Rebased cleanly onto the latest \main\ branch.

whatever agent you're using (which you haven't declared per AGENTS.md) is having a hard time with escaping characters. might want to double check that.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

3 participants