python: fix segfault on empty argv in main()

[senyai]

test:
python -c 'import google_benchmark as b;b.register(print);b.main([]);'

[senyai]

I think there's more explicit and expressive way to update the logic:

if (!argv.empty()) {
  static std::string executable_name(argv[0]);
  ptrs[0] = const_cast<char*>(executable_name.c_str());
}

should I change it, @LebedevRI?

[senyai]

Now I'm glad with this simple fix @LebedevRI

[senyai]

btw, my initial command for reproducing the issue is no longer giving me segmentation fault, it just says ValueError: argv cannot be an empty list, and must contain the program name as the first element. Not sure why, I didn't change anything. But this command python -c 'import google_benchmark as b;b.register(print);b.initialize([]);' should still give you a segfault in main.

[LebedevRI]

Doesn't this just exchange one bug for another?
The comment suggests that something uses the binary name (argv[0]) later on,
but now instead of "crashing" here, we'll crash the code that tries to take argv[0].

[senyai]

The code that tries to take argv[0] is in benchmark::Initialize, and there's already a check that ensures that it does not happen when argc is 0. That's why this little change is enough. I'm not sure how to make the comment better.

[LebedevRI]

-std::vector<std::string> Initialize(const std::vector<std::string>& argv) {
-  // The `argv` pointers here become invalid when this function returns, but
-  // benchmark holds the pointer to `argv[0]`. We create a static copy of it
-  // so it persists, and replace the pointer below.
-  static std::string executable_name(argv[0]);
-  std::vector<char*> ptrs;
-  ptrs.reserve(argv.size());
-  for (auto& arg : argv) {
-    ptrs.push_back(const_cast<char*>(arg.c_str()));
-  }
-  ptrs[0] = const_cast<char*>(executable_name.c_str());
-  int argc = static_cast<int>(argv.size());
-  benchmark::Initialize(&argc, ptrs.data());
-  std::vector<std::string> remaining_argv;
-  remaining_argv.reserve(argc);
-  for (int i = 0; i < argc; ++i) {
-    remaining_argv.emplace_back(ptrs[i]);
-  }
-  return remaining_argv;
-}

+std::vector<std::string> Initialize(const std::vector<std::string>& argv) {
+  // The `argv` pointers here become invalid when this function returns, but
+  // benchmark holds the pointer to `argv[0]`. We create a static copy of it
+  // so it persists, and replace the pointer below.
+  std::vector<char*> ptrs;
+  if(argv.empty()) {
+    static std::string executable_name("unknown");
+    ptrs.emplace_back(executable_name.c_str());
+  } else {
+    ptrs.reserve(argv.size());
+    for (auto& arg : argv) {
+      ptrs.push_back(const_cast<char*>(arg.c_str()));
+    }
+    static std::string executable_name(argv[0]);
+    ptrs[0] = const_cast<char*>(executable_name.c_str());
+  }
+  int argc = static_cast<int>(ptrs.size());
+  benchmark::Initialize(&argc, ptrs.data());
+  std::vector<std::string> remaining_argv;
+  remaining_argv.reserve(argc);
+  for (int i = 0; i < argc; ++i) {
+    remaining_argv.emplace_back(ptrs[i]);
+  }
+  return remaining_argv;
+}

[senyai]

It is okay to pass empty array to benchmark::Initialize

benchmark/src/benchmark.cc

Lines 753 to 754 in b20cea6

	BenchmarkReporter::Context::executable_name =
	((argc != nullptr) && *argc > 0) ? argv[0] : "unknown";

[LebedevRI]

I don't really touch python stuff here.
I've suggested how i think this would be best done, but i'm perfectly happy to just leave it for other reviewers.