diff --git a/src/benchmark.cc b/src/benchmark.cc index 91280295e..8b78cc48b 100644 --- a/src/benchmark.cc +++ b/src/benchmark.cc @@ -586,11 +586,14 @@ ConsoleReporter::OutputOptions GetOutputOptions(bool force_no_color) { } // end namespace internal BenchmarkReporter* CreateDefaultDisplayReporter() { - static auto* default_display_reporter = - internal::CreateReporter(FLAGS_benchmark_format, - internal::GetOutputOptions()) - .release(); - return default_display_reporter; + // Ownership of the returned reporter is transferred to the caller (note the + // `.release()` below and the `unique_ptr` in `RunSpecifiedBenchmarks`). + // Do NOT cache this in a `static`: the caller deletes the reporter, which + // would leave a cached pointer dangling and cause a use-after-free on the + // next call. Create a fresh reporter each time instead. + return internal::CreateReporter(FLAGS_benchmark_format, + internal::GetOutputOptions()) + .release(); } size_t RunSpecifiedBenchmarks() { diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index fe88841dd..d10e780a5 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -257,6 +257,7 @@ if (BENCHMARK_ENABLE_GTEST_TESTS) add_gtest(string_util_gtest) add_gtest(perf_counters_gtest) add_gtest(reporter_list_gtest) + add_gtest(create_default_display_reporter_gtest) add_gtest(time_unit_gtest) add_gtest(min_time_parse_gtest) add_gtest(profiler_manager_gtest) diff --git a/test/create_default_display_reporter_gtest.cc b/test/create_default_display_reporter_gtest.cc new file mode 100644 index 000000000..8afd2db4b --- /dev/null +++ b/test/create_default_display_reporter_gtest.cc @@ -0,0 +1,27 @@ +#include + +#include "benchmark/benchmark.h" +#include "gtest/gtest.h" + +namespace benchmark { +namespace { + +// Regression test for a heap-use-after-free (issue #2240): +// CreateDefaultDisplayReporter() transfers ownership of the returned reporter +// to the caller (RunSpecifiedBenchmarks() wraps it in a unique_ptr and deletes +// it). It must therefore return a fresh object on every call. A previous +// implementation cached the pointer in a function-local `static`, so the second +// call returned an already-deleted reporter -> use-after-free. +TEST(CreateDefaultDisplayReporterTest, ReturnsFreshOwnedReporterEachCall) { + std::unique_ptr first(CreateDefaultDisplayReporter()); + std::unique_ptr second(CreateDefaultDisplayReporter()); + + ASSERT_NE(first, nullptr); + ASSERT_NE(second, nullptr); + // Distinct objects: neither call hands back a shared/cached pointer that the + // other unique_ptr would also try to delete (double-free / use-after-free). + EXPECT_NE(first.get(), second.get()); +} + +} // namespace +} // namespace benchmark