Andrzej Hunt <andrzej@xxxxxxxxx> writes:
>> + 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)
>
>> +
>> + 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.
