google / benchmark

A microbenchmark support library
Apache License 2.0
9.03k stars 1.63k forks source link

[BUG] Uninitialized pointer access when using --benchmark_repetitions in some scenarios #1406

Open Sparta142 opened 2 years ago

Sparta142 commented 2 years ago

Describe the bug An uninitialized pointer is dereferenced when calculating statistics for a repeated benchmark that fails a certain number of times, which causes a segv/access violation/other mayhem.

This doesn't seem to happen for benchmarks that always fail, nor if you run without --benchmark_repetitions=N.

System Which OS, compiler, and compiler version are you using:

In my experience this happens on various compiler (MSVC, GCC) and execution environments (Windows x64, Linux arm64), as well as in both Debug and Release builds.

To reproduce

#include <benchmark/benchmark.h>

static int count = 0;

static void BM_StringCreation(benchmark::State &state)
{
    if (++count == 3)
    {
        state.SkipWithError("Simulate a flaky benchmark");
        return;
    }

    for (auto _ : state)
        std::string empty_string;
}
BENCHMARK(BM_StringCreation);

int main(int argc, char *argv[])
{
    benchmark::Initialize(&argc, argv);
    benchmark::RunSpecifiedBenchmarks();
    benchmark::Shutdown();
    return 0;
}

Steps to reproduce the behavior: Compile the given code above on MSVC 19 with benchmark 1.6.1, and run using:

./example.exe --benchmark_repetitions=5

Expected behavior Benchmark does not crash. Statistics are calculated for only the passing runs. (If this is incorrect usage of the library, it should be documented somewhere.)

Screenshots image

Additional context Looks like the uninitialized pointer is reports[0].statistics: https://github.com/google/benchmark/blob/6d50251d8e6ed6b7f6eb1e12c558d2808fb4ddaa/src/statistics.cc#L159

It seems to not be initialized here (on line 98) when a run has an error: https://github.com/google/benchmark/blob/6d50251d8e6ed6b7f6eb1e12c558d2808fb4ddaa/src/benchmark_runner.cc#L88-L99

dmah42 commented 2 years ago

i agree, this looks to be a bug. though i would have expected us to skip error reports in the statistics calculations.