Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C

Andrzej Hunt [off-list ref] writes:

quoted

+	struct strbuf command = STRBUF_INIT;
+	struct strvec args = STRVEC_INIT;
+	struct strvec run_args = STRVEC_INIT;
+ ...
+	run_args.v[0] = xstrdup(command.buf);
+	run_args.nr = 1;

AFAIUI manipulating the strvec directly like this means that we will
violate the promise that strvec.v is always NULL-terminated. It's 
probably safer to call 'strvec_push(run_args, command.buf)' instead of
manipulating v and nr?

True.

Violating the NULL-termination promise a problem because... (continued
below)

quoted

+
+	while (1) {
+		strvec_clear(&args);
+		exit = 1;
+
+		printf(_("running %s"), command.buf);
+		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);

run_command_v_opt() implicitly expects a NULL-terminated list of
strings. It's not documented in run_command_v_opt()'s comments,
however run_command_v_opt() does explain that it's a wrapper around 
start_command(), which uses child_process, and child_process.argv is
documented to require a NULL-terminated list.

If argv is not NULL-terminated, we hit a buffer overflow read  in
prepare_shell_cmd(), which can be reproduced by running something
like:

  make CC=clang-11 SANITIZE=address COPTS="-Og -g" GIT_TEST_OPTS=-v
  T=t6030-bisect-porcelain.sh test

which results in ASAN reporting this error:
...

Thanks for a careful explanation.