From 9484943b82c119c5f692ec50b04f8a92bda98c51 Mon Sep 17 00:00:00 2001 From: jdymitarai Date: Sat, 12 Sep 2026 10:51:27 +0800 Subject: [PATCH] Harden reporters against formula injection and invalid JSON / input shape (fixes #2299) This resolves the three defense-in-depth hardening notes raised in #2299: 1. CSV formula-prefix neutralization: - Neutralize spreadsheet formula prefixes ('=', '+', '-', '@') in CsvEscape by prepending a single quote (') when enclosed in double quotes. - Properly escape embedded double quotes as "" and handle user counter header names with CsvEscape. 2. Full C0 control character escaping in JSON reporter: - Adhere strictly to RFC 8259 Section 7 by escaping all C0 control characters (0x00 to 0x1F) as \u00XX in internal::JsonStrEscape. - Preserves standard two-character escapes (\b, \f, \n, \r, \t, \", \\) and raw UTF-8 multi-byte sequences. 3. Input shape and type diagnostics in gbench tooling: - Add check_benchmark_results() in tools/gbench/util.py to validate JSON artifact structure, benchmark arrays, names, numeric times, and valid time units. - Provide informative diagnostics before processing benchmark comparisons. - Add comprehensive unit tests in tools/gbench/util.py, test/string_util_gtest.cc, and test/reporter_list_gtest.cc. --- src/csv_reporter.cc | 17 +--- src/json_reporter.cc | 32 +------ src/string_util.cc | 65 +++++++++++++++ src/string_util.h | 10 +++ test/reporter_list_gtest.cc | 41 +++++++++ test/string_util_gtest.cc | 58 +++++++++++++ tools/gbench/report.py | 5 +- tools/gbench/util.py | 162 +++++++++++++++++++++++++++++++++++- 8 files changed, 340 insertions(+), 50 deletions(-) diff --git a/src/csv_reporter.cc b/src/csv_reporter.cc index 3e21d11f0a..b5fb68551d 100644 --- a/src/csv_reporter.cc +++ b/src/csv_reporter.cc @@ -21,6 +21,7 @@ #include "benchmark_api_internal.h" #include "check.h" #include "complexity.h" +#include "string_util.h" // File format reference: http://edoceo.com/utilitas/csv-file-format. @@ -33,19 +34,7 @@ const std::vector elements = { "error_occurred", "error_message"}; std::string CsvEscape(const std::string& s) { - std::string tmp; - tmp.reserve(s.size() + 2); - for (char c : s) { - switch (c) { - case '"': - tmp += "\"\""; - break; - default: - tmp += c; - break; - } - } - return '"' + tmp + '"'; + return internal::CsvEscape(s); } } // namespace @@ -80,7 +69,7 @@ void CSVReporter::ReportRuns(const std::vector& reports) { } for (auto B = user_counter_names_.begin(); B != user_counter_names_.end();) { - Out << ",\"" << *B++ << "\""; + Out << "," << CsvEscape(*B++); } Out << "\n"; diff --git a/src/json_reporter.cc b/src/json_reporter.cc index 37da17ba03..71add44f2d 100644 --- a/src/json_reporter.cc +++ b/src/json_reporter.cc @@ -35,37 +35,7 @@ namespace benchmark { namespace { std::string StrEscape(const std::string& s) { - std::string tmp; - tmp.reserve(s.size()); - for (char c : s) { - switch (c) { - case '\b': - tmp += "\\b"; - break; - case '\f': - tmp += "\\f"; - break; - case '\n': - tmp += "\\n"; - break; - case '\r': - tmp += "\\r"; - break; - case '\t': - tmp += "\\t"; - break; - case '\\': - tmp += "\\\\"; - break; - case '"': - tmp += "\\\""; - break; - default: - tmp += c; - break; - } - } - return tmp; + return internal::JsonStrEscape(s); } std::string FormatKV(std::string const& key, std::string const& value) { diff --git a/src/string_util.cc b/src/string_util.cc index 9a0d54234c..f6026dc9c5 100644 --- a/src/string_util.cc +++ b/src/string_util.cc @@ -267,4 +267,69 @@ double stod(const std::string& str, size_t* pos) { } #endif +namespace internal { + +std::string CsvEscape(const std::string& s) { + std::string tmp; + tmp.reserve(s.size() + 2); + if (!s.empty() && + (s[0] == '=' || s[0] == '+' || s[0] == '-' || s[0] == '@')) { + tmp += '\''; + } + for (char c : s) { + switch (c) { + case '"': + tmp += "\"\""; + break; + default: + tmp += c; + break; + } + } + return '"' + tmp + '"'; +} + +std::string JsonStrEscape(const std::string& s) { + std::string tmp; + tmp.reserve(s.size()); + for (char c : s) { + switch (c) { + case '\b': + tmp += "\\b"; + break; + case '\f': + tmp += "\\f"; + break; + case '\n': + tmp += "\\n"; + break; + case '\r': + tmp += "\\r"; + break; + case '\t': + tmp += "\\t"; + break; + case '\\': + tmp += "\\\\"; + break; + case '"': + tmp += "\\\""; + break; + default: + if (static_cast(c) < 0x20) { + char buf[7]; + snprintf(buf, sizeof(buf), "\\u%04x", + static_cast(static_cast(c))); + tmp += buf; + } else { + tmp += c; + } + break; + } + } + return tmp; +} + +} // end namespace internal + } // end namespace benchmark diff --git a/src/string_util.h b/src/string_util.h index 1a846668ba..7cc932c2e0 100644 --- a/src/string_util.h +++ b/src/string_util.h @@ -58,6 +58,16 @@ using std::stoul; // NOLINT(misc-unused-using-decls) #endif // NOLINTEND +namespace internal { + +BENCHMARK_EXPORT +std::string CsvEscape(const std::string& s); + +BENCHMARK_EXPORT +std::string JsonStrEscape(const std::string& s); + +} // end namespace internal + } // end namespace benchmark #endif // BENCHMARK_STRING_UTIL_H_ diff --git a/test/reporter_list_gtest.cc b/test/reporter_list_gtest.cc index 74d5d02dd6..9aea04b618 100644 --- a/test/reporter_list_gtest.cc +++ b/test/reporter_list_gtest.cc @@ -82,6 +82,47 @@ TEST(ReporterListTest, CSVListsNameColumn) { BENCHMARK_RESTORE_DEPRECATED_WARNING } +const std::vector& ListEscapedBenchmarks() { + static const std::vector* const benchmarks = [] { + RegisterBenchmark("=FormulaInjection", BM_ReporterListDummy); + RegisterBenchmark("+PlusPrefix", BM_ReporterListDummy); + RegisterBenchmark("-MinusPrefix", BM_ReporterListDummy); + RegisterBenchmark("@AtPrefix", BM_ReporterListDummy); + RegisterBenchmark("BM_ANSI_\x1b[31mRed\x1b[0m", BM_ReporterListDummy); + auto* result = new std::vector(); + std::ostringstream err_stream; + FindBenchmarksInternal( + "(=FormulaInjection|\\+PlusPrefix|-MinusPrefix|@AtPrefix|BM_ANSI_).*", + result, &err_stream); + return result; + }(); + return *benchmarks; +} + +TEST(ReporterListTest, CSVEscapesFormulaPrefixes) { + BENCHMARK_DISABLE_DEPRECATED_WARNING + CSVReporter reporter; + std::ostringstream out; + reporter.SetOutputStream(&out); + reporter.List(ListEscapedBenchmarks()); + std::string s = out.str(); + EXPECT_NE(s.find("\"'=FormulaInjection\"\n"), std::string::npos); + EXPECT_NE(s.find("\"'+PlusPrefix\"\n"), std::string::npos); + EXPECT_NE(s.find("\"'-MinusPrefix\"\n"), std::string::npos); + EXPECT_NE(s.find("\"'@AtPrefix\"\n"), std::string::npos); + BENCHMARK_RESTORE_DEPRECATED_WARNING +} + +TEST(ReporterListTest, JSONEscapesC0ControlChars) { + JSONReporter reporter; + std::ostringstream out; + reporter.SetOutputStream(&out); + reporter.List(ListEscapedBenchmarks()); + std::string s = out.str(); + EXPECT_NE(s.find("\"name\": \"BM_ANSI_\\u001b[31mRed\\u001b[0m\""), + std::string::npos); +} + } // namespace } // namespace internal } // namespace benchmark diff --git a/test/string_util_gtest.cc b/test/string_util_gtest.cc index 5a9a09e19b..5b2b357cb9 100644 --- a/test/string_util_gtest.cc +++ b/test/string_util_gtest.cc @@ -200,4 +200,62 @@ TEST_P(HumanReadableFixture, HumanReadableNumber) { ASSERT_THAT(str, ::testing::MatchesRegex(std::get<2>(GetParam()))); } +TEST(StringUtilTest, CsvEscape) { + // Empty string + EXPECT_EQ(benchmark::internal::CsvEscape(""), "\"\""); + + // Standard safe strings + EXPECT_EQ(benchmark::internal::CsvEscape("BM_basic"), "\"BM_basic\""); + EXPECT_EQ(benchmark::internal::CsvEscape("hello world"), "\"hello world\""); + + // Embedded double-quotes + EXPECT_EQ(benchmark::internal::CsvEscape("foo\"bar"), "\"foo\"\"bar\""); + + // Formula prefixes neutralized with leading single-quote + EXPECT_EQ(benchmark::internal::CsvEscape("=1+1"), "\"'=1+1\""); + EXPECT_EQ(benchmark::internal::CsvEscape("=SUM(A1:A2)"), "\"'=SUM(A1:A2)\""); + EXPECT_EQ(benchmark::internal::CsvEscape("+cmd"), "\"'+cmd\""); + EXPECT_EQ(benchmark::internal::CsvEscape("-10"), "\"'-10\""); + EXPECT_EQ(benchmark::internal::CsvEscape("@admin"), "\"'@admin\""); + + // Formula prefix with embedded quotes and carriage return + EXPECT_EQ(benchmark::internal::CsvEscape("=cmd|' /C calc'!A0"), + "\"'=cmd|' /C calc'!A0\""); + EXPECT_EQ(benchmark::internal::CsvEscape("=foo\r\n\"bar\""), + "\"'=foo\r\n\"\"bar\"\"\""); +} + +TEST(StringUtilTest, JsonStrEscape) { + // Empty string + EXPECT_EQ(benchmark::internal::JsonStrEscape(""), ""); + + // Safe strings + EXPECT_EQ(benchmark::internal::JsonStrEscape("hello world"), "hello world"); + + // Standard short escapes + EXPECT_EQ(benchmark::internal::JsonStrEscape("\"quoted\\backslash\""), + "\\\"quoted\\\\backslash\\\""); + EXPECT_EQ(benchmark::internal::JsonStrEscape( + "tab\tnewline\nreturn\rbackspace\bformfeed\f"), + "tab\\tnewline\\nreturn\\rbackspace\\bformfeed\\f"); + + // C0 control characters (RFC 8259 Section 7: 0x00 to 0x1F) + EXPECT_EQ(benchmark::internal::JsonStrEscape(std::string("\x00", 1)), + "\\u0000"); + EXPECT_EQ(benchmark::internal::JsonStrEscape("\x01"), "\\u0001"); + EXPECT_EQ(benchmark::internal::JsonStrEscape("\x07"), "\\u0007"); + EXPECT_EQ(benchmark::internal::JsonStrEscape("\x0b"), "\\u000b"); + EXPECT_EQ(benchmark::internal::JsonStrEscape("\x1b"), "\\u001b"); + EXPECT_EQ(benchmark::internal::JsonStrEscape("\x1f"), "\\u001f"); + + // ANSI escape sequences in skip/error messages + EXPECT_EQ(benchmark::internal::JsonStrEscape("\x1b[31mred text\x1b[0m"), + "\\u001b[31mred text\\u001b[0m"); + + // Printable ASCII and UTF-8 multi-byte characters remain unescaped + EXPECT_EQ(benchmark::internal::JsonStrEscape(" 0123456789!@#$%^&*()~`"), + " 0123456789!@#$%^&*()~`"); + EXPECT_EQ(benchmark::internal::JsonStrEscape("中文测试"), "中文测试"); +} + } // end namespace diff --git a/tools/gbench/report.py b/tools/gbench/report.py index 16b674ab09..ff746a6d0b 100644 --- a/tools/gbench/report.py +++ b/tools/gbench/report.py @@ -1489,7 +1489,10 @@ def load_result(): cls.json = load_result() def test_json_diff_report_pretty_printing(self): - import util + try: + from gbench import util + except ImportError: + import util expected_names = [ "99 family 0 instance 0 repetition 0", diff --git a/tools/gbench/util.py b/tools/gbench/util.py index 7847e65444..72e536801d 100644 --- a/tools/gbench/util.py +++ b/tools/gbench/util.py @@ -8,6 +8,7 @@ import subprocess import sys import tempfile +import unittest # Input file type enumeration IT_Invalid = 0 @@ -118,6 +119,70 @@ def remove_benchmark_flags(prefix, benchmark_flags): return [f for f in benchmark_flags if not f.startswith(prefix)] +VALID_TIME_UNITS = {"ns", "us", "ms", "s"} + + +def check_benchmark_results(results, fname): + """ + Validate the shape and types of a benchmark output artifact. + Fails with an informative diagnostic if the artifact is malformed. + """ + if not isinstance(results, dict): + print( + f"In {fname}, expected root JSON to be an object, got" + f" {type(results).__name__}" + ) + sys.exit(1) + context = results.get("context") + if context is not None and not isinstance(context, dict): + print( + f"In {fname}, 'context' must be an object, got" + f" {type(context).__name__}" + ) + sys.exit(1) + if "benchmarks" not in results: + print(f"In {fname}, missing required 'benchmarks' array") + sys.exit(1) + if not isinstance(results["benchmarks"], list): + print( + f"In {fname}, 'benchmarks' must be an array, got" + f" {type(results['benchmarks']).__name__}" + ) + sys.exit(1) + for i, run in enumerate(results["benchmarks"]): + if not isinstance(run, dict): + print( + f"In {fname}, run[{i}] must be an object, got" + f" {type(run).__name__}" + ) + sys.exit(1) + name = run.get("name") + if name is None: + print(f"In {fname}, run[{i}] missing 'name'") + sys.exit(1) + if not isinstance(name, str): + print( + f"In {fname}, run[{i}].name is not a string, got" + f" {type(name).__name__}" + ) + sys.exit(1) + if run.get("error_occurred", False): + continue + for time_key in ("real_time", "cpu_time"): + if time_key in run and not isinstance(run[time_key], (int, float)): + print( + f"In {fname}, run[{i}].{time_key} must be numeric, got" + f" {type(run[time_key]).__name__}" + ) + sys.exit(1) + if "time_unit" in run and run["time_unit"] not in VALID_TIME_UNITS: + print( + f"In {fname}, run[{i}].time_unit '{run['time_unit']}' is unknown" + f" (expected one of: {', '.join(sorted(VALID_TIME_UNITS))})" + ) + sys.exit(1) + + def load_benchmark_results(fname, benchmark_filter): """ Read benchmark output from a file and return the JSON object. @@ -147,10 +212,10 @@ def benchmark_wanted(benchmark): f" {json_schema_version}, expected 1" ) sys.exit(1) - if "benchmarks" in results: - results["benchmarks"] = list( - filter(benchmark_wanted, results["benchmarks"]) - ) + check_benchmark_results(results, fname) + results["benchmarks"] = list( + filter(benchmark_wanted, results["benchmarks"]) + ) return results @@ -227,3 +292,92 @@ def run_or_load_benchmark(filename, benchmark_flags): if ftype == IT_Executable: return run_benchmark(filename, benchmark_flags) raise ValueError("Unknown file type %s" % ftype) + + +class TestCheckBenchmarkResults(unittest.TestCase): + def test_valid_results(self): + valid = { + "context": {"json_schema_version": 1}, + "benchmarks": [ + { + "name": "BM_test", + "real_time": 10.0, + "cpu_time": 10.0, + "time_unit": "ns", + }, + { + "name": "BM_error", + "error_occurred": True, + "error_message": "some error", + }, + ], + } + # Should not raise SystemExit + check_benchmark_results(valid, "valid.json") + + def test_root_not_dict(self): + with self.assertRaises(SystemExit): + check_benchmark_results(["not", "a", "dict"], "test.json") + + def test_context_not_dict(self): + with self.assertRaises(SystemExit): + check_benchmark_results( + {"context": "bad", "benchmarks": []}, "test.json" + ) + + def test_missing_benchmarks(self): + with self.assertRaises(SystemExit): + check_benchmark_results({}, "test.json") + + def test_benchmarks_not_list(self): + with self.assertRaises(SystemExit): + check_benchmark_results({"benchmarks": "bad"}, "test.json") + + def test_run_not_dict(self): + with self.assertRaises(SystemExit): + check_benchmark_results({"benchmarks": ["bad_run"]}, "test.json") + + def test_run_missing_name(self): + with self.assertRaises(SystemExit): + check_benchmark_results( + {"benchmarks": [{"real_time": 10}]}, "test.json" + ) + + def test_run_name_not_string(self): + with self.assertRaises(SystemExit): + check_benchmark_results( + {"benchmarks": [{"name": 123}]}, "test.json" + ) + + def test_run_time_not_numeric(self): + with self.assertRaises(SystemExit): + check_benchmark_results( + { + "benchmarks": [ + { + "name": "BM_bad", + "real_time": "fast", + "cpu_time": 10, + "time_unit": "ns", + } + ] + }, + "test.json", + ) + + def test_run_unknown_time_unit(self): + with self.assertRaises(SystemExit): + check_benchmark_results( + { + "benchmarks": [ + { + "name": "BM_bad", + "real_time": 10, + "cpu_time": 10, + "time_unit": "hours", + } + ] + }, + "test.json", + ) +