Repository navigation
Conversation
| break; | ||
| default: | ||
| tmp += c; | ||
| if (static_cast<unsigned char>(c) < 0x20) { |
There was a problem hiding this comment.
can we slightly simplify this with auto uc = static_cast<unsigned char>(c); which can then be reused in the snprintf below?
dxnnystyles
left a comment
There was a problem hiding this comment.
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>
bec54bc to
c200398
Compare
|
Thanks for the review and suggestions! I have updated the PR to address your feedback:
|
|
thanke! I checked the revised escaping logic + regression test. the new assertions cover the expected |
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. |
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,StrEscapeexplicitly 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\u00XXviastd::snprintf. Printable characters and UTF-8 multibyte sequences remain unaffected.Fixes #2299.