PATCH] bisect--helper: plug strvec leak in bisect_start()

PATCH] bisect--helper: plug strvec leak in bisect_start()

[René Scharfe]

The strvec "argv" is used to build a command for run_command_v_opt(),
but never freed.  Use the "args" strvec of struct child_process and
run_command() instead, which releases the allocated memory both on
success and on error.  We just also need to set the "git_cmd" bit
directly.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/bisect--helper.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 501245fac9..9fe0c08479 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -765,11 +765,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
 		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
 		strbuf_trim(&start_head);
 		if (!no_checkout) {
-			struct strvec argv = STRVEC_INIT;
+			struct child_process cmd = CHILD_PROCESS_INIT;

-			strvec_pushl(&argv, "checkout", start_head.buf,
+			cmd.git_cmd = 1;
+			strvec_pushl(&cmd.args, "checkout", start_head.buf,
 				     "--", NULL);
-			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+			if (run_command(&cmd)) {
 				res = error(_("checking out '%s' failed."
 						 " Try 'git bisect start "
 						 "<valid-branch>'."),
--
2.37.3

Re: PATCH] bisect--helper: plug strvec leak in bisect_start()

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 04 2022, René Scharfe wrote:

> The strvec "argv" is used to build a command for run_command_v_opt(),
> but never freed.  Use the "args" strvec of struct child_process and
> run_command() instead, which releases the allocated memory both on
> success and on error.  We just also need to set the "git_cmd" bit
> directly.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/bisect--helper.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 501245fac9..9fe0c08479 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -765,11 +765,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
>  		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>  		strbuf_trim(&start_head);
>  		if (!no_checkout) {
> -			struct strvec argv = STRVEC_INIT;
> +			struct child_process cmd = CHILD_PROCESS_INIT;
>
> -			strvec_pushl(&argv, "checkout", start_head.buf,
> +			cmd.git_cmd = 1;
> +			strvec_pushl(&cmd.args, "checkout", start_head.buf,
>  				     "--", NULL);
> -			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
> +			if (run_command(&cmd)) {
>  				res = error(_("checking out '%s' failed."
>  						 " Try 'git bisect start "
>  						 "<valid-branch>'."),

Okey, so we leak the strvec, and instead of adding a strvec_clear()
you're just switching the lower-level API, which we'd need in some cases
with this API, and would often be cleaner.

But I don't get it in this case, why not just:
	
	diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
	index 4e97817fba5..f9645a9d0df 100644
	--- a/builtin/bisect--helper.c
	+++ b/builtin/bisect--helper.c
	@@ -763,11 +763,9 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
	 		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
	 		strbuf_trim(&start_head);
	 		if (!no_checkout) {
	-			struct strvec argv = STRVEC_INIT;
	+			const char *argv[] = { "checkout", start_head.buf, "--", NULL };
	 
	-			strvec_pushl(&argv, "checkout", start_head.buf,
	-				     "--", NULL);
	-			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
	+			if (run_command_v_opt(argv, RUN_GIT_CMD)) {
	 				res = error(_("checking out '%s' failed."
	 						 " Try 'git bisect start "
	 						 "<valid-branch>'."),

The common pattern for run_command_v_opt() callers that don't need a
dynamic list is exactly that, e.g.:
	
	builtin/difftool.c=static int print_tool_help(void)
	builtin/difftool.c-{
	builtin/difftool.c-     const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
	builtin/difftool.c:     return run_command_v_opt(argv, RUN_GIT_CMD);
	builtin/difftool.c-}
	
And:
	
	fsmonitor-ipc.c=static int spawn_daemon(void)
	fsmonitor-ipc.c-{
	fsmonitor-ipc.c-        const char *args[] = { "fsmonitor--daemon", "start", NULL };
	fsmonitor-ipc.c-
	fsmonitor-ipc.c:        return run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,
	fsmonitor-ipc.c-                                    "fsmonitor");
	fsmonitor-ipc.c-}

Here we have the "start_head" which we'll need to strbuf_release(), but
we're not returning directly, and the function is doing that already.

Your version is slightly more memory efficient, i.e. we'll end up having
to push this to a "struct child_process"'s argv anyway, but this is less
code & we don't need to carefully eyeball run_command_v_opt_cd_env_tr2()
to see that it's correct (which I did, your version does the right thing
too).

Re: PATCH] bisect--helper: plug strvec leak in bisect_start()

[René Scharfe]

Am 05.10.22 um 09:29 schrieb Ævar Arnfjörð Bjarmason:
>
> On Tue, Oct 04 2022, René Scharfe wrote:
>
>> The strvec "argv" is used to build a command for run_command_v_opt(),
>> but never freed.  Use the "args" strvec of struct child_process and
>> run_command() instead, which releases the allocated memory both on
>> success and on error.  We just also need to set the "git_cmd" bit
>> directly.
>>
>> Signed-off-by: René Scharfe <l.s.r@web.de>
>> ---
>>  builtin/bisect--helper.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 501245fac9..9fe0c08479 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -765,11 +765,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
>>  		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>>  		strbuf_trim(&start_head);
>>  		if (!no_checkout) {
>> -			struct strvec argv = STRVEC_INIT;
>> +			struct child_process cmd = CHILD_PROCESS_INIT;
>>
>> -			strvec_pushl(&argv, "checkout", start_head.buf,
>> +			cmd.git_cmd = 1;
>> +			strvec_pushl(&cmd.args, "checkout", start_head.buf,
>>  				     "--", NULL);
>> -			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
>> +			if (run_command(&cmd)) {
>>  				res = error(_("checking out '%s' failed."
>>  						 " Try 'git bisect start "
>>  						 "<valid-branch>'."),
>
> Okey, so we leak the strvec, and instead of adding a strvec_clear()
> you're just switching the lower-level API, which we'd need in some cases
> with this API, and would often be cleaner.
>
> But I don't get it in this case, why not just:
>
> 	diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> 	index 4e97817fba5..f9645a9d0df 100644
> 	--- a/builtin/bisect--helper.c
> 	+++ b/builtin/bisect--helper.c
> 	@@ -763,11 +763,9 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
> 	 		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
> 	 		strbuf_trim(&start_head);
> 	 		if (!no_checkout) {
> 	-			struct strvec argv = STRVEC_INIT;
> 	+			const char *argv[] = { "checkout", start_head.buf, "--", NULL };
>
> 	-			strvec_pushl(&argv, "checkout", start_head.buf,
> 	-				     "--", NULL);
> 	-			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
> 	+			if (run_command_v_opt(argv, RUN_GIT_CMD)) {
> 	 				res = error(_("checking out '%s' failed."
> 	 						 " Try 'git bisect start "
> 	 						 "<valid-branch>'."),

That looks even better.  I didn't think of that because we had a phase
where we could only use constant expressions for initialization.  We do
have a precedent in 359f0d754a (range-diff/format-patch: handle commit
ranges other than A..B, 2021-02-05), though, which does use a variable:

+       char *copy = xstrdup(arg); /* setup_revisions() modifies it */
+       const char *argv[] = { "", copy, "--", NULL };

(There may be earlier ones, that's just the one I found quickly.) So is
it time for that C99 feature already?  Yay! 🥳

> The common pattern for run_command_v_opt() callers that don't need a
> dynamic list is exactly that, e.g.:
>
> 	builtin/difftool.c=static int print_tool_help(void)
> 	builtin/difftool.c-{
> 	builtin/difftool.c-     const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
> 	builtin/difftool.c:     return run_command_v_opt(argv, RUN_GIT_CMD);
> 	builtin/difftool.c-}
>
> And:
>
> 	fsmonitor-ipc.c=static int spawn_daemon(void)
> 	fsmonitor-ipc.c-{
> 	fsmonitor-ipc.c-        const char *args[] = { "fsmonitor--daemon", "start", NULL };
> 	fsmonitor-ipc.c-
> 	fsmonitor-ipc.c:        return run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,
> 	fsmonitor-ipc.c-                                    "fsmonitor");
> 	fsmonitor-ipc.c-}
>
> Here we have the "start_head" which we'll need to strbuf_release(), but
> we're not returning directly, and the function is doing that already.
>
> Your version is slightly more memory efficient, i.e. we'll end up having
> to push this to a "struct child_process"'s argv anyway, but this is less
> code & we don't need to carefully eyeball run_command_v_opt_cd_env_tr2()
> to see that it's correct (which I did, your version does the right thing
> too).

Having two ways to set the same flags adds some unnecessary friction.

René

Re: PATCH] bisect--helper: plug strvec leak in bisect_start()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> The strvec "argv" is used to build a command for run_command_v_opt(),
> but never freed.  Use the "args" strvec of struct child_process and
> run_command() instead, which releases the allocated memory both on
> success and on error.  We just also need to set the "git_cmd" bit
> directly.

Well reasoned and explained.

Thanks.

>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/bisect--helper.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 501245fac9..9fe0c08479 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -765,11 +765,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
>  		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>  		strbuf_trim(&start_head);
>  		if (!no_checkout) {
> -			struct strvec argv = STRVEC_INIT;
> +			struct child_process cmd = CHILD_PROCESS_INIT;
>
> -			strvec_pushl(&argv, "checkout", start_head.buf,
> +			cmd.git_cmd = 1;
> +			strvec_pushl(&cmd.args, "checkout", start_head.buf,
>  				     "--", NULL);
> -			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
> +			if (run_command(&cmd)) {
>  				res = error(_("checking out '%s' failed."
>  						 " Try 'git bisect start "
>  						 "<valid-branch>'."),
> --
> 2.37.3

Re: PATCH] bisect--helper: plug strvec leak in bisect_start()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> But I don't get it in this case, why not just:
> 	
> 	diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> 	index 4e97817fba5..f9645a9d0df 100644
> 	--- a/builtin/bisect--helper.c
> 	+++ b/builtin/bisect--helper.c
> 	@@ -763,11 +763,9 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
> 	 		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
> 	 		strbuf_trim(&start_head);
> 	 		if (!no_checkout) {
> 	-			struct strvec argv = STRVEC_INIT;
> 	+			const char *argv[] = { "checkout", start_head.buf, "--", NULL };
> 	 
> 	-			strvec_pushl(&argv, "checkout", start_head.buf,
> 	-				     "--", NULL);
> 	-			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
> 	+			if (run_command_v_opt(argv, RUN_GIT_CMD)) {
> 	 				res = error(_("checking out '%s' failed."
> 	 						 " Try 'git bisect start "
> 	 						 "<valid-branch>'."),
>
> The common pattern for run_command_v_opt() callers that don't need a
> dynamic list is exactly that.

I think you answered it yourself.  start_head.buf is not known at
compilation time, and there may be some superstition (it may not be
a mere superstition, but conservatism) about older compiler not
grokking it.

Re: PATCH] bisect--helper: plug strvec leak in bisect_start()

[Ævar Arnfjörð Bjarmason]

On Wed, Oct 05 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> But I don't get it in this case, why not just:
>> 	
>> 	diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> 	index 4e97817fba5..f9645a9d0df 100644
>> 	--- a/builtin/bisect--helper.c
>> 	+++ b/builtin/bisect--helper.c
>> 	@@ -763,11 +763,9 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
>> 	 		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>> 	 		strbuf_trim(&start_head);
>> 	 		if (!no_checkout) {
>> 	-			struct strvec argv = STRVEC_INIT;
>> 	+			const char *argv[] = { "checkout", start_head.buf, "--", NULL };
>> 	 
>> 	-			strvec_pushl(&argv, "checkout", start_head.buf,
>> 	-				     "--", NULL);
>> 	-			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
>> 	+			if (run_command_v_opt(argv, RUN_GIT_CMD)) {
>> 	 				res = error(_("checking out '%s' failed."
>> 	 						 " Try 'git bisect start "
>> 	 						 "<valid-branch>'."),
>>
>> The common pattern for run_command_v_opt() callers that don't need a
>> dynamic list is exactly that.
>
> I think you answered it yourself.  start_head.buf is not known at
> compilation time, and there may be some superstition (it may not be
> a mere superstition, but conservatism) about older compiler not
> grokking it.

I think we're thoroughly past that hump as we have a hard requirement on
designated initializers.

Anyway, I believe GCC's -std=c89 wtith -pedantic catches this, e.g. for
bisect--helper.c (the latter is the above patch):

	$ make -k git-objs CFLAGS=-std=c89 2>&1|grep 'initializer element is not computable at load time'|grep bisect
	builtin/bisect--helper.c:534:43: error: initializer element is not computable at load time [-Werror=pedantic]
	builtin/bisect--helper.c:768:60: error: initializer element is not computable at load time [-Werror=pedantic]

For the former we've had:

	static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)
	[...]
		struct add_bisect_ref_data cb = { revs };

In the same file since 517ecb3161d (bisect--helper: reimplement
`bisect_next` and `bisect_auto_next` shell functions in C, 2020-09-24).

Other prior art, just taking the char[] ones (and not even all of them):
	
	builtin/merge-index.c:12:37: error: initializer element is not computable at load time [-Werror=pedantic]
	   12 |         const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
	builtin/remote.c:95:41: error: initializer element is not computable at load time [-Werror=pedantic]
	   95 |         const char *argv[] = { "fetch", name, NULL, NULL };
	archive.c:408:33: error: initializer element is not computable at load time [-Werror=pedantic]
	  408 |         const char *paths[] = { path, NULL };
	merge-ort.c:1699:45: error: initializer element is not computable at load time [-Werror=pedantic]
	 1699 |                                    "--all", merged_revision, NULL };

So I think we can safely use this.	

Re: PATCH] bisect--helper: plug strvec leak in bisect_start()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>>> The common pattern for run_command_v_opt() callers that don't need a
>>> dynamic list is exactly that.

So, scratching "that don't need a dynamic list", and with ...

> Other prior art, just taking the char[] ones (and not even all of them):
> ...
> 	builtin/merge-index.c:12:37: error: initializer element is not computable at load time [-Werror=pedantic]
> 	   12 |         const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
> 	builtin/remote.c:95:41: error: initializer element is not computable at load time [-Werror=pedantic]
> 	   95 |         const char *argv[] = { "fetch", name, NULL, NULL };
> 	archive.c:408:33: error: initializer element is not computable at load time [-Werror=pedantic]
> 	  408 |         const char *paths[] = { path, NULL };
> 	merge-ort.c:1699:45: error: initializer element is not computable at load time [-Werror=pedantic]
> 	 1699 |                                    "--all", merged_revision, NULL };

... these, I agree that we are not making anything worse.

Thanks.

[PATCH v2] bisect--helper: plug strvec leak

[René Scharfe]

The strvec "argv" is used to build a command for run_command_v_opt(),
but never freed.  Use a constant string array instead, which doesn't
require any cleanup.

Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/bisect--helper.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 501245fac9..28ef7ec2a4 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -765,11 +765,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
 		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
 		strbuf_trim(&start_head);
 		if (!no_checkout) {
-			struct strvec argv = STRVEC_INIT;
+			const char *argv[] = { "checkout", start_head.buf,
+					       "--", NULL };

-			strvec_pushl(&argv, "checkout", start_head.buf,
-				     "--", NULL);
-			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+			if (run_command_v_opt(argv, RUN_GIT_CMD)) {
 				res = error(_("checking out '%s' failed."
 						 " Try 'git bisect start "
 						 "<valid-branch>'."),
--
2.38.0

Re: [PATCH v2] bisect--helper: plug strvec leak

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> The strvec "argv" is used to build a command for run_command_v_opt(),
> but never freed.  Use a constant string array instead, which doesn't
> require any cleanup.
>
> Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/bisect--helper.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)

Thanks, both.  Looking good.

>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 501245fac9..28ef7ec2a4 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -765,11 +765,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
>  		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>  		strbuf_trim(&start_head);
>  		if (!no_checkout) {
> -			struct strvec argv = STRVEC_INIT;
> +			const char *argv[] = { "checkout", start_head.buf,
> +					       "--", NULL };
>
> -			strvec_pushl(&argv, "checkout", start_head.buf,
> -				     "--", NULL);
> -			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
> +			if (run_command_v_opt(argv, RUN_GIT_CMD)) {
>  				res = error(_("checking out '%s' failed."
>  						 " Try 'git bisect start "
>  						 "<valid-branch>'."),
> --
> 2.38.0

Re: [PATCH v2] bisect--helper: plug strvec leak

[Jeff King]

On Fri, Oct 07, 2022 at 05:08:42PM +0200, René Scharfe wrote:

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 501245fac9..28ef7ec2a4 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -765,11 +765,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
>  		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>  		strbuf_trim(&start_head);
>  		if (!no_checkout) {
> -			struct strvec argv = STRVEC_INIT;
> +			const char *argv[] = { "checkout", start_head.buf,
> +					       "--", NULL };
> 
> -			strvec_pushl(&argv, "checkout", start_head.buf,
> -				     "--", NULL);
> -			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
> +			if (run_command_v_opt(argv, RUN_GIT_CMD)) {

This is OK with me, but note that one thing we lose is compiler
protection that we remembered the trailing NULL pointer in the argv
array (whereas strvec_pushl() has an attribute that makes sure the last
argument is a NULL).

Probably not that big a deal in practice. It would be nice if there was
a way to annotate this for the compiler, but I don't think there's any
attribute for "this pointer-to-pointer parameter should have a NULL at
the end".

-Peff

Re: [PATCH v2] bisect--helper: plug strvec leak

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Oct 07, 2022 at 05:08:42PM +0200, René Scharfe wrote:
>
>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>> index 501245fac9..28ef7ec2a4 100644
>> --- a/builtin/bisect--helper.c
>> +++ b/builtin/bisect--helper.c
>> @@ -765,11 +765,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
>>  		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>>  		strbuf_trim(&start_head);
>>  		if (!no_checkout) {
>> -			struct strvec argv = STRVEC_INIT;
>> +			const char *argv[] = { "checkout", start_head.buf,
>> +					       "--", NULL };
>> 
>> -			strvec_pushl(&argv, "checkout", start_head.buf,
>> -				     "--", NULL);
>> -			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
>> +			if (run_command_v_opt(argv, RUN_GIT_CMD)) {
>
> This is OK with me, but note that one thing we lose is compiler
> protection that we remembered the trailing NULL pointer in the argv
> array (whereas strvec_pushl() has an attribute that makes sure the last
> argument is a NULL).

The first parameter to run_command_v_opt() must be a NULL terminated
array of strings.  argv.v[] after strvec_push*() is such a NULL
terminated array, and is suitable to be passed to the function.

That much human programmers would know.

But does the compiler know that run_command_v_opt() requires a NULL
terminated array of strings, and does it know to check that argv.v[]
came from strvec_pushl() without any annotation in the first place?

For such a check to happen, I think we need to tell the compiler
with some annotation that the first parameter to run_command_v_opt()
is supposed to be a NULL terminated char *[] array.

In the code before the patch, strvec_pushl() is annotated to require
the last arg to be NULL, but without following data/control flow, it
may not be clear to the compiler that argv.v[] will be NULL terminated
after the function returns.  If the compiler is sufficiently clever
to figure it out, with the knowledge that run_command_v_opt() must
be given a NULL terminated array of strings, we do have compiler
protection to make the run_command_v_opt() safe.

But if the code tells the compiler that run_command_v_opt() must be
given a NULL terminated array of strings, it is obvious that the
array passed by the code after the patch, i.e. argv[], is NULL
terminated, and the compiler would be happy, as well.

> Probably not that big a deal in practice. It would be nice if there was
> a way to annotate this for the compiler, but I don't think there's any
> attribute for "this pointer-to-pointer parameter should have a NULL at
> the end".

But the code before this patch is safe only for strvec_pushl() call,
not run_command_v_opt() call, so we are not losing anything, I would
think.

Re: [PATCH v2] bisect--helper: plug strvec leak

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 10 2022, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
>
>> On Fri, Oct 07, 2022 at 05:08:42PM +0200, René Scharfe wrote:
>>
>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
>>> index 501245fac9..28ef7ec2a4 100644
>>> --- a/builtin/bisect--helper.c
>>> +++ b/builtin/bisect--helper.c
>>> @@ -765,11 +765,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
>>>  		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>>>  		strbuf_trim(&start_head);
>>>  		if (!no_checkout) {
>>> -			struct strvec argv = STRVEC_INIT;
>>> +			const char *argv[] = { "checkout", start_head.buf,
>>> +					       "--", NULL };
>>> 
>>> -			strvec_pushl(&argv, "checkout", start_head.buf,
>>> -				     "--", NULL);
>>> -			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
>>> +			if (run_command_v_opt(argv, RUN_GIT_CMD)) {
>>
>> This is OK with me, but note that one thing we lose is compiler
>> protection that we remembered the trailing NULL pointer in the argv
>> array (whereas strvec_pushl() has an attribute that makes sure the last
>> argument is a NULL).
>
> The first parameter to run_command_v_opt() must be a NULL terminated
> array of strings.  argv.v[] after strvec_push*() is such a NULL
> terminated array, and is suitable to be passed to the function.
>
> That much human programmers would know.
>
> But does the compiler know that run_command_v_opt() requires a NULL
> terminated array of strings, and does it know to check that argv.v[]
> came from strvec_pushl() without any annotation in the first place?
>
> For such a check to happen, I think we need to tell the compiler
> with some annotation that the first parameter to run_command_v_opt()
> is supposed to be a NULL terminated char *[] array.
>
> In the code before the patch, strvec_pushl() is annotated to require
> the last arg to be NULL, but without following data/control flow, it
> may not be clear to the compiler that argv.v[] will be NULL terminated
> after the function returns.  If the compiler is sufficiently clever
> to figure it out, with the knowledge that run_command_v_opt() must
> be given a NULL terminated array of strings, we do have compiler
> protection to make the run_command_v_opt() safe.
>
> But if the code tells the compiler that run_command_v_opt() must be
> given a NULL terminated array of strings, it is obvious that the
> array passed by the code after the patch, i.e. argv[], is NULL
> terminated, and the compiler would be happy, as well.
>
>> Probably not that big a deal in practice. It would be nice if there was
>> a way to annotate this for the compiler, but I don't think there's any
>> attribute for "this pointer-to-pointer parameter should have a NULL at
>> the end".
>
> But the code before this patch is safe only for strvec_pushl() call,
> not run_command_v_opt() call, so we are not losing anything, I would
> think.

Yes, and if we suppose a bug like this sneaking in one way or the other:
	
	diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
	index 28ef7ec2a48..a7f9d43a6f1 100644
	--- a/builtin/bisect--helper.c
	+++ b/builtin/bisect--helper.c
	@@ -766,7 +766,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
	                strbuf_trim(&start_head);
	                if (!no_checkout) {
	                        const char *argv[] = { "checkout", start_head.buf,
	-                                              "--", NULL };
	+                                              "--" };
	 
	                        if (run_command_v_opt(argv, RUN_GIT_CMD)) {
	                                res = error(_("checking out '%s' failed."

I don't know a way to statically flag that, but we'll catch it with
SANITIZE=address:
	
	+ git bisect start 32a594a3fdac2d57cf6d02987e30eec68511498c 88bcdc1839f0ad191ffdd65cae2a2a862d682151 --
	=================================================================
	==1734==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe6bbf5c18 at pc 0x55d31729bc7a bp 0x7ffe6bbf5920 sp 0x7ffe6bbf5918
	READ of size 8 at 0x7ffe6bbf5c18 thread T0
	    #0 0x55d31729bc79 in strvec_pushv strvec.c:55
	    #1 0x55d317232be9 in run_command_v_opt_cd_env_tr2 run-command.c:1026
	    #2 0x55d317232a3c in run_command_v_opt_cd_env run-command.c:1019
	    #3 0x55d3172329d1 in run_command_v_opt run-command.c:1009
	    #4 0x55d316c1337a in bisect_start builtin/bisect--helper.c:771
	    #5 0x55d316c17d06 in cmd_bisect__helper builtin/bisect--helper.c:1346
	    #6 0x55d316bf190f in run_builtin git.c:466
	    #7 0x55d316bf22bb in handle_builtin git.c:721
	    #8 0x55d316bf29e5 in run_argv git.c:788
	    #9 0x55d316bf375f in cmd_main git.c:921
	    #10 0x55d316e79a06 in main common-main.c:57
	    #11 0x7fceab0a0209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
	    #12 0x7fceab0a02bb in __libc_start_main_impl ../csu/libc-start.c:389
	    #13 0x55d316bed210 in _start (git+0x1d7210)

So I'm not worried about it happening in the first place, but if it does
we'd also need to have no test coverage of the relevant code to miss it.

Re: [PATCH v2] bisect--helper: plug strvec leak

[Jeff King]

On Mon, Oct 10, 2022 at 10:42:42PM -0700, Junio C Hamano wrote:

> >> -			struct strvec argv = STRVEC_INIT;
> >> +			const char *argv[] = { "checkout", start_head.buf,
> >> +					       "--", NULL };
> >> 
> >> -			strvec_pushl(&argv, "checkout", start_head.buf,
> >> -				     "--", NULL);
> >> -			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
> >> +			if (run_command_v_opt(argv, RUN_GIT_CMD)) {
> >
> > This is OK with me, but note that one thing we lose is compiler
> > protection that we remembered the trailing NULL pointer in the argv
> > array (whereas strvec_pushl() has an attribute that makes sure the last
> > argument is a NULL).
> 
> The first parameter to run_command_v_opt() must be a NULL terminated
> array of strings.  argv.v[] after strvec_push*() is such a NULL
> terminated array, and is suitable to be passed to the function.
> 
> That much human programmers would know.
> 
> But does the compiler know that run_command_v_opt() requires a NULL
> terminated array of strings, and does it know to check that argv.v[]
> came from strvec_pushl() without any annotation in the first place?

No, but I don't think that's the interesting part. If you're using
strvec, it does the right thing, and it's hard to get it wrong. I'm more
concerned about places where we manually write a list of strings, and
it's easy to forget the trailing NULL.

In the existing code, that's done in the interface of strvec_pushl(),
which will remind you if you write:

  strvec_pushl(&arg, "checkout", start_head.buf, "--");

But after it is done in an initializer, which has no clue about the
expected semantics. We only have to get strvec's invariants right once.
But every ad-hoc command argv has to remember the trailing NULL.

> For such a check to happen, I think we need to tell the compiler
> with some annotation that the first parameter to run_command_v_opt()
> is supposed to be a NULL terminated char *[] array.

Right, but I would not expect the compiler to realize that strvec
maintains the ends-in-NULL invariant. It would have to be quite a clever
compiler.

In theory it could realize that argv is declared as an array locally,
and could make sure it ends in NULL as a compile-time check.

So it would have to be: "check this statically if you can, but otherwise
assume it's OK" kind of warning. But it's all kind of moot since I don't
think any such annotation exists. :)

Possibly a linter like sparse could complain about declaring a variable
called argv that doesn't end in NULL. I don't think it's worth anybody
spending too much time on it, though. This hasn't historically been a
big source of bugs.

> > Probably not that big a deal in practice. It would be nice if there was
> > a way to annotate this for the compiler, but I don't think there's any
> > attribute for "this pointer-to-pointer parameter should have a NULL at
> > the end".
> 
> But the code before this patch is safe only for strvec_pushl() call,
> not run_command_v_opt() call, so we are not losing anything, I would
> think.

The bug I'm worried about it is in a human writing the list of strings
and forgetting the NULL, so there we are losing the (admittedly minor)
protection.

-Peff

Re: [PATCH v2] bisect--helper: plug strvec leak

[Jeff King]

On Tue, Oct 11, 2022 at 09:29:01AM +0200, Ævar Arnfjörð Bjarmason wrote:

> > But the code before this patch is safe only for strvec_pushl() call,
> > not run_command_v_opt() call, so we are not losing anything, I would
> > think.
> 
> Yes, and if we suppose a bug like this sneaking in one way or the other:
> 	
> 	diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> 	index 28ef7ec2a48..a7f9d43a6f1 100644
> 	--- a/builtin/bisect--helper.c
> 	+++ b/builtin/bisect--helper.c
> 	@@ -766,7 +766,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
> 	                strbuf_trim(&start_head);
> 	                if (!no_checkout) {
> 	                        const char *argv[] = { "checkout", start_head.buf,
> 	-                                              "--", NULL };
> 	+                                              "--" };
> 	 
> 	                        if (run_command_v_opt(argv, RUN_GIT_CMD)) {
> 	                                res = error(_("checking out '%s' failed."
> 
> I don't know a way to statically flag that, but we'll catch it with
> SANITIZE=address:

I'd expect we'd even catch it in a non-sanitizing build, since we'd
likely feed garbage to exec (unless we get lucky and there's a NULL on
the stack). I like catching bugs as early as possible, but I agree this
kind is not likely to get very far (assuming there's test coverage).

-Peff

Re: [PATCH v2] bisect--helper: plug strvec leak

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The bug I'm worried about it is in a human writing the list of strings
> and forgetting the NULL, so there we are losing the (admittedly minor)
> protection.

I expect that this story will repeat itself, especially given that
we asserted that it is OK to initialize such an array with variable
reference recently in this thread.

Here are a couple that I found with a quick eyeballing of the output
of "git grep -e 'run_command_v_opt([^,]*\.v,' \*.c" command.



diff --git a/builtin/clone.c b/builtin/clone.c
index ed8d44bb6a..c93345bc75 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -651,9 +651,8 @@ static void update_head(const struct ref *our, const struct ref *remote,
 
 static int git_sparse_checkout_init(const char *repo)
 {
-	struct strvec argv = STRVEC_INIT;
 	int result = 0;
-	strvec_pushl(&argv, "-C", repo, "sparse-checkout", "set", NULL);
+	const char *argv[] = { "-C", repo, "sparse-checkout", "set", NULL };
 
 	/*
 	 * We must apply the setting in the current process
@@ -661,12 +660,11 @@ static int git_sparse_checkout_init(const char *repo)
 	 */
 	core_apply_sparse_checkout = 1;
 
-	if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+	if (run_command_v_opt(argv, RUN_GIT_CMD)) {
 		error(_("failed to initialize sparse-checkout"));
 		result = 1;
 	}
 
-	strvec_clear(&argv);
 	return result;
 }
 
diff --git a/builtin/merge.c b/builtin/merge.c
index 0a0ca8b7c4..d261bc652f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -384,24 +384,20 @@ static void reset_hard(const struct object_id *oid, int verbose)
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	struct strvec args = STRVEC_INIT;
-
 	reset_hard(head, 1);
 
-	if (is_null_oid(stash))
-		goto refresh_cache;
-
-	strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
-	strvec_push(&args, oid_to_hex(stash));
+	if (!is_null_oid(stash)) {
+		const char *argv[] = {
+			"stash", "apply", "--index", "--quiet", oid_to_hex(stash), NULL
+		};
 
-	/*
-	 * It is OK to ignore error here, for example when there was
-	 * nothing to restore.
-	 */
-	run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
+		/*
+		 * It is OK to ignore error here, for example when there was
+		 * nothing to restore.
+		 */
+		run_command_v_opt(argv, RUN_GIT_CMD);
+	}
 
-refresh_cache:
 	if (discard_cache() < 0 || read_cache() < 0)
 		die(_("could not read index"));
 }

Re: [PATCH v2] bisect--helper: plug strvec leak

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 11 2022, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
>
>> The bug I'm worried about it is in a human writing the list of strings
>> and forgetting the NULL, so there we are losing the (admittedly minor)
>> protection.
>
> I expect that this story will repeat itself, especially given that
> we asserted that it is OK to initialize such an array with variable
> reference recently in this thread.
>
> Here are a couple that I found with a quick eyeballing of the output
> of "git grep -e 'run_command_v_opt([^,]*\.v,' \*.c" command.
>
>
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index ed8d44bb6a..c93345bc75 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -651,9 +651,8 @@ static void update_head(const struct ref *our, const struct ref *remote,
>  
>  static int git_sparse_checkout_init(const char *repo)
>  {
> -	struct strvec argv = STRVEC_INIT;
>  	int result = 0;
> -	strvec_pushl(&argv, "-C", repo, "sparse-checkout", "set", NULL);
> +	const char *argv[] = { "-C", repo, "sparse-checkout", "set", NULL };
>  
>  	/*
>  	 * We must apply the setting in the current process
> @@ -661,12 +660,11 @@ static int git_sparse_checkout_init(const char *repo)
>  	 */
>  	core_apply_sparse_checkout = 1;
>  
> -	if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
> +	if (run_command_v_opt(argv, RUN_GIT_CMD)) {
>  		error(_("failed to initialize sparse-checkout"));
>  		result = 1;
>  	}
>  
> -	strvec_clear(&argv);
>  	return result;
>  }
>  
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 0a0ca8b7c4..d261bc652f 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -384,24 +384,20 @@ static void reset_hard(const struct object_id *oid, int verbose)
>  static void restore_state(const struct object_id *head,
>  			  const struct object_id *stash)
>  {
> -	struct strvec args = STRVEC_INIT;
> -
>  	reset_hard(head, 1);
>  
> -	if (is_null_oid(stash))
> -		goto refresh_cache;
> -
> -	strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
> -	strvec_push(&args, oid_to_hex(stash));
> +	if (!is_null_oid(stash)) {
> +		const char *argv[] = {
> +			"stash", "apply", "--index", "--quiet", oid_to_hex(stash), NULL
> +		};
>  
> -	/*
> -	 * It is OK to ignore error here, for example when there was
> -	 * nothing to restore.
> -	 */
> -	run_command_v_opt(args.v, RUN_GIT_CMD);
> -	strvec_clear(&args);
> +		/*
> +		 * It is OK to ignore error here, for example when there was
> +		 * nothing to restore.
> +		 */
> +		run_command_v_opt(argv, RUN_GIT_CMD);
> +	}
>  
> -refresh_cache:
>  	if (discard_cache() < 0 || read_cache() < 0)
>  		die(_("could not read index"));
>  }

I was experimenting with implementing a run_command_opt_l() earlier
which would give us the safety Jeff notes. The relevant end-state for
these two files is (there's more conversions, and I manually edited the
diff to remove an unrelated change):

diff --git a/builtin/clone.c b/builtin/clone.c
index ed8d44bb6ab..8dc986b5196 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -651,23 +651,18 @@ static void update_head(const struct ref *our, const struct ref *remote,
 
 static int git_sparse_checkout_init(const char *repo)
 {
-	struct strvec argv = STRVEC_INIT;
-	int result = 0;
-	strvec_pushl(&argv, "-C", repo, "sparse-checkout", "set", NULL);
-
 	/*
 	 * We must apply the setting in the current process
 	 * for the later checkout to use the sparse-checkout file.
 	 */
 	core_apply_sparse_checkout = 1;
 
-	if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+	if (run_command_opt_l(RUN_GIT_CMD, "-C", repo, "sparse-checkout",
+			      "set", NULL)) {
 		error(_("failed to initialize sparse-checkout"));
-		result = 1;
+		return 1;
 	}
-
-	strvec_clear(&argv);
-	return result;
+	return 0;
 }
 
@@ -862,11 +856,11 @@ static void write_refspec_config(const char *src_ref_prefix,
 
 static void dissociate_from_references(void)
 {
-	static const char* argv[] = { "repack", "-a", "-d", NULL };
 	char *alternates = git_pathdup("objects/info/alternates");
 
 	if (!access(alternates, F_OK)) {
-		if (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))
+		if (run_command_opt_l(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,
+				      "repack",  "-a", "-d", NULL))
 			die(_("cannot repack to clean up"));
 		if (unlink(alternates) && errno != ENOENT)
 			die_errno(_("cannot unlink temporary alternates file"));
diff --git a/builtin/merge.c b/builtin/merge.c
index 5900b81729d..9c08de57113 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -345,60 +345,34 @@ static int save_state(struct object_id *stash)
 	return rc;
 }
 
-static void read_empty(const struct object_id *oid, int verbose)
+static void read_empty(const struct object_id *oid)
 {
-	int i = 0;
-	const char *args[7];
-
-	args[i++] = "read-tree";
-	if (verbose)
-		args[i++] = "-v";
-	args[i++] = "-m";
-	args[i++] = "-u";
-	args[i++] = empty_tree_oid_hex();
-	args[i++] = oid_to_hex(oid);
-	args[i] = NULL;
-
-	if (run_command_v_opt(args, RUN_GIT_CMD))
+	if (run_command_opt_l(RUN_GIT_CMD, "read-tree", "-m", "-u",
+			      empty_tree_oid_hex(), oid_to_hex(oid), NULL))
 		die(_("read-tree failed"));
 }
 
-static void reset_hard(const struct object_id *oid, int verbose)
+static void reset_hard(const struct object_id *oid)
 {
-	int i = 0;
-	const char *args[6];
-
-	args[i++] = "read-tree";
-	if (verbose)
-		args[i++] = "-v";
-	args[i++] = "--reset";
-	args[i++] = "-u";
-	args[i++] = oid_to_hex(oid);
-	args[i] = NULL;
-
-	if (run_command_v_opt(args, RUN_GIT_CMD))
+	if (run_command_opt_l(RUN_GIT_CMD, "read-tree", "-v", "--reset", "-u",
+			      oid_to_hex(oid), NULL))
 		die(_("read-tree failed"));
 }
 
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	struct strvec args = STRVEC_INIT;
-
-	reset_hard(head, 1);
+	reset_hard(head);
 
 	if (is_null_oid(stash))
 		goto refresh_cache;
 
-	strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
-	strvec_push(&args, oid_to_hex(stash));
-
 	/*
 	 * It is OK to ignore error here, for example when there was
 	 * nothing to restore.
 	 */
-	run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
+	run_command_opt_l(RUN_GIT_CMD, "stash", "apply", "--index", "--quiet",
+			  oid_to_hex(stash), NULL);
 
 refresh_cache:
 	if (discard_cache() < 0 || read_cache() < 0)
@@ -1470,7 +1444,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 					       check_trust_level);
 
 		remote_head_oid = &remoteheads->item->object.oid;
-		read_empty(remote_head_oid, 0);
+		read_empty(remote_head_oid);
 		update_ref("initial pull", "HEAD", remote_head_oid, NULL, 0,
 			   UPDATE_REFS_DIE_ON_ERR);
 		goto done;

Re: [PATCH v2] bisect--helper: plug strvec leak

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I was experimenting with implementing a run_command_opt_l() earlier
> which would give us the safety Jeff notes. The relevant end-state for
> these two files is (there's more conversions, and I manually edited the
> diff to remove an unrelated change):

There still are some changes that would not belong here, but for the
purpose of illustrating the usage of _l variant, this is good enough.
Of course, if we want to follow the existing naming scheme, the new
function must be called _l_opt(), not _opt_l().  _v_ in the _v_opt()
comes from execv() and you are adding execl() analogue here.

> -	if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
> +	if (run_command_opt_l(RUN_GIT_CMD, "-C", repo, "sparse-checkout",
> +			      "set", NULL)) {

And this does give us protection from the "Programmers can give
unterminated list to run_command_v_opt() by mistake", which is not
really solved mechanically even if the list is prepared with the
strvec API (because the compiler has to be smart enough to know that
argv.v was prepared with proper use of the API), which is nice.

Having to give the option flags as the first argument, of course it
is necessary because we are talking about the _l variant with
varargs, looks ugly, though.

Also, I am not sure how useful this new function actually is.  Many
hits from my quick "grep" in the message you are responding to did
use strvec because they wanted to dynamically build up the command
line, and _l variant does not really shine in these places.

We may probably be able to refactor some (but not all) of these
sites so that the command line is not built dynamically with unknown
number of elements in unknown order, but that feels like a solution
looking for a problem, changing code to use the new function, tail
wagging the dog.  So, I dunno...

Re: [PATCH v2] bisect--helper: plug strvec leak

[Jeff King]

On Tue, Oct 11, 2022 at 02:43:24PM -0700, Junio C Hamano wrote:

> > -	if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
> > +	if (run_command_opt_l(RUN_GIT_CMD, "-C", repo, "sparse-checkout",
> > +			      "set", NULL)) {
> 
> And this does give us protection from the "Programmers can give
> unterminated list to run_command_v_opt() by mistake", which is not
> really solved mechanically even if the list is prepared with the
> strvec API (because the compiler has to be smart enough to know that
> argv.v was prepared with proper use of the API), which is nice.

Yeah, I agree this addresses the point I raised (which I am somewhat
regretting raising, as IMHO it was not worth the amount of discussion
that has ensued).

Since nobody asked, my _real_ opinion is that I prefer René's original
that used an actual struct, and its auto-freeing strvec. And I'd be
happy to see all of the run_command_v() variants go away entirely. It
does save a few lines, but with modern niceties, it's not very many, and
it's much less flexible.

I resisted saying so earlier because I do not think it is even worth
anybody's time to think about, let alone implement.

-Peff

Re: [PATCH v2] bisect--helper: plug strvec leak

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Oct 11, 2022 at 02:43:24PM -0700, Junio C Hamano wrote:
>
>> > -	if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
>> > +	if (run_command_opt_l(RUN_GIT_CMD, "-C", repo, "sparse-checkout",
>> > +			      "set", NULL)) {
>> 
>> And this does give us protection from the "Programmers can give
>> unterminated list to run_command_v_opt() by mistake", which is not
>> really solved mechanically even if the list is prepared with the
>> strvec API (because the compiler has to be smart enough to know that
>> argv.v was prepared with proper use of the API), which is nice.
>
> Yeah, I agree this addresses the point I raised (which I am somewhat
> regretting raising, as IMHO it was not worth the amount of discussion
> that has ensued).
>
> Since nobody asked, my _real_ opinion is that I prefer René's original
> that used an actual struct, and its auto-freeing strvec.

Yup, it was you who worried about forgotten NULL at the end, though
;-)

Re: [PATCH v2] bisect--helper: plug strvec leak

[René Scharfe]

Am 14.10.22 um 21:44 schrieb Jeff King:
> And I'd be happy to see all of the run_command_v() variants go away
> entirely. It does save a few lines, but with modern niceties, it's
> not very many, and it's much less flexible.
That would look something like below.

---
 add-interactive.c        |   9 ++-
 bisect.c                 |  12 ++--
 builtin/add.c            |  19 +++--
 builtin/am.c             |  12 ++--
 builtin/bisect--helper.c |  64 ++++++++---------
 builtin/clone.c          |  41 ++++++-----
 builtin/difftool.c       |  24 ++++---
 builtin/fetch.c          |   9 ++-
 builtin/gc.c             |  51 ++++++++------
 builtin/merge-index.c    |   4 +-
 builtin/merge.c          |  42 +++++------
 builtin/pull.c           | 147 +++++++++++++++++++--------------------
 builtin/remote.c         |  40 +++++------
 compat/mingw.c           |  11 +--
 diff.c                   |  27 ++++---
 fsmonitor-ipc.c          |  10 ++-
 git.c                    |  15 ++--
 ll-merge.c               |   7 +-
 merge.c                  |  18 ++---
 run-command.c            |  35 ----------
 run-command.h            |  30 --------
 scalar.c                 |  13 ++--
 sequencer.c              |  32 ++++-----
 shell.c                  |  17 +++--
 t/helper/test-fake-ssh.c |   7 +-
 t/helper/test-trace2.c   |   4 +-
 tmp-objdir.h             |   4 +-
 27 files changed, 330 insertions(+), 374 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index f071b2a1b4..ecc5ae1b24 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -997,18 +997,17 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
 	count = list_and_choose(s, files, opts);
 	opts->flags = 0;
 	if (count > 0) {
-		struct strvec args = STRVEC_INIT;
+		struct child_process cmd = CHILD_PROCESS_INIT;

-		strvec_pushl(&args, "git", "diff", "-p", "--cached",
+		strvec_pushl(&cmd.args, "git", "diff", "-p", "--cached",
 			     oid_to_hex(!is_initial ? &oid :
 					s->r->hash_algo->empty_tree),
 			     "--", NULL);
 		for (i = 0; i < files->items.nr; i++)
 			if (files->selected[i])
-				strvec_push(&args,
+				strvec_push(&cmd.args,
 					    files->items.items[i].string);
-		res = run_command_v_opt(args.v, 0);
-		strvec_clear(&args);
+		res = run_command(&cmd);
 	}

 	putchar('\n');
diff --git a/bisect.c b/bisect.c
index fd581b85a7..ec7487e683 100644
--- a/bisect.c
+++ b/bisect.c
@@ -22,8 +22,6 @@ static struct oid_array skipped_revs;

 static struct object_id *current_bad_oid;

-static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
-
 static const char *term_bad;
 static const char *term_good;

@@ -729,20 +727,22 @@ static int is_expected_rev(const struct object_id *oid)
 enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
 				  int no_checkout)
 {
-	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
 	struct commit *commit;
 	struct pretty_print_context pp = {0};
 	struct strbuf commit_msg = STRBUF_INIT;

-	oid_to_hex_r(bisect_rev_hex, bisect_rev);
 	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);

-	argv_checkout[2] = bisect_rev_hex;
 	if (no_checkout) {
 		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
 			   UPDATE_REFS_DIE_ON_ERR);
 	} else {
-		if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		cmd.git_cmd = 1;
+		strvec_pushl(&cmd.args, "checkout", "-q",
+			     oid_to_hex(bisect_rev), "--", NULL);
+		if (run_command(&cmd))
 			/*
 			 * Errors in `run_command()` itself, signaled by res < 0,
 			 * and errors in the child process, signaled by res > 0
diff --git a/builtin/add.c b/builtin/add.c
index f84372964c..626c71ec6a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -240,8 +240,8 @@ static int refresh(int verbose, const struct pathspec *pathspec)
 int run_add_interactive(const char *revision, const char *patch_mode,
 			const struct pathspec *pathspec)
 {
-	int status, i;
-	struct strvec argv = STRVEC_INIT;
+	int i;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int use_builtin_add_i =
 		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);

@@ -272,19 +272,18 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 		return !!run_add_p(the_repository, mode, revision, pathspec);
 	}

-	strvec_push(&argv, "add--interactive");
+	strvec_push(&cmd.args, "add--interactive");
 	if (patch_mode)
-		strvec_push(&argv, patch_mode);
+		strvec_push(&cmd.args, patch_mode);
 	if (revision)
-		strvec_push(&argv, revision);
-	strvec_push(&argv, "--");
+		strvec_push(&cmd.args, revision);
+	strvec_push(&cmd.args, "--");
 	for (i = 0; i < pathspec->nr; i++)
 		/* pass original pathspec, to be re-parsed */
-		strvec_push(&argv, pathspec->items[i].original);
+		strvec_push(&cmd.args, pathspec->items[i].original);

-	status = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-	return status;
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 int interactive_add(const char **argv, const char *prefix, int patch)
diff --git a/builtin/am.c b/builtin/am.c
index 39fea24833..20aea0d248 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2187,14 +2187,12 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
 	int len;

 	if (!is_null_oid(&state->orig_commit)) {
-		const char *av[4] = { "show", NULL, "--", NULL };
-		char *new_oid_str;
-		int ret;
+		struct child_process cmd = CHILD_PROCESS_INIT;

-		av[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit));
-		ret = run_command_v_opt(av, RUN_GIT_CMD);
-		free(new_oid_str);
-		return ret;
+		strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit),
+			     "--", NULL);
+		cmd.git_cmd = 1;
+		return run_command(&cmd);
 	}

 	switch (sub_mode) {
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 501245fac9..542f0cb8f5 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -220,18 +220,17 @@ static int bisect_reset(const char *commit)
 	}

 	if (!ref_exists("BISECT_HEAD")) {
-		struct strvec argv = STRVEC_INIT;
+		struct child_process cmd = CHILD_PROCESS_INIT;

-		strvec_pushl(&argv, "checkout", branch.buf, "--", NULL);
-		if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+		strvec_pushl(&cmd.args, "checkout", branch.buf, "--", NULL);
+		cmd.git_cmd = 1;
+		if (run_command(&cmd)) {
 			error(_("could not check out original"
 				" HEAD '%s'. Try 'git bisect"
 				" reset <commit>'."), branch.buf);
 			strbuf_release(&branch);
-			strvec_clear(&argv);
 			return -1;
 		}
-		strvec_clear(&argv);
 	}

 	strbuf_release(&branch);
@@ -765,11 +764,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
 		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
 		strbuf_trim(&start_head);
 		if (!no_checkout) {
-			struct strvec argv = STRVEC_INIT;
+			struct child_process cmd = CHILD_PROCESS_INIT;

-			strvec_pushl(&argv, "checkout", start_head.buf,
+			strvec_pushl(&cmd.args, "checkout", start_head.buf,
 				     "--", NULL);
-			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+			cmd.git_cmd = 1;
+			if (run_command(&cmd)) {
 				res = error(_("checking out '%s' failed."
 						 " Try 'git bisect start "
 						 "<valid-branch>'."),
@@ -1099,40 +1099,38 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **ar

 static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)
 {
-	struct strvec args = STRVEC_INIT;
-	int flags = RUN_COMMAND_NO_STDIN, res = 0;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;

 	if (bisect_next_check(terms, NULL) != 0)
 		return BISECT_FAILED;

+	cmd.no_stdin = 1;
 	if (!argc) {
 		if ((getenv("DISPLAY") || getenv("SESSIONNAME") || getenv("MSYSTEM") ||
 		     getenv("SECURITYSESSIONID")) && exists_in_PATH("gitk")) {
-			strvec_push(&args, "gitk");
+			strvec_push(&cmd.args, "gitk");
 		} else {
-			strvec_push(&args, "log");
-			flags |= RUN_GIT_CMD;
+			strvec_push(&cmd.args, "log");
+			cmd.git_cmd = 1;
 		}
 	} else {
 		if (argv[0][0] == '-') {
-			strvec_push(&args, "log");
-			flags |= RUN_GIT_CMD;
+			strvec_push(&cmd.args, "log");
+			cmd.git_cmd = 1;
 		} else if (strcmp(argv[0], "tig") && !starts_with(argv[0], "git"))
-			flags |= RUN_GIT_CMD;
+			cmd.git_cmd = 1;

-		strvec_pushv(&args, argv);
+		strvec_pushv(&cmd.args, argv);
 	}

-	strvec_pushl(&args, "--bisect", "--", NULL);
+	strvec_pushl(&cmd.args, "--bisect", "--", NULL);

 	strbuf_read_file(&sb, git_path_bisect_names(), 0);
-	sq_dequote_to_strvec(sb.buf, &args);
+	sq_dequote_to_strvec(sb.buf, &cmd.args);
 	strbuf_release(&sb);

-	res = run_command_v_opt(args.v, flags);
-	strvec_clear(&args);
-	return res;
+	return run_command(&cmd);
 }

 static int get_first_good(const char *refname UNUSED,
@@ -1143,8 +1141,7 @@ static int get_first_good(const char *refname UNUSED,
 	return 1;
 }

-static int verify_good(const struct bisect_terms *terms,
-		       const char **quoted_argv)
+static int verify_good(const struct bisect_terms *terms, const char *command)
 {
 	int rc;
 	enum bisect_error res;
@@ -1152,6 +1149,7 @@ static int verify_good(const struct bisect_terms *terms,
 	struct object_id current_rev;
 	char *good_glob = xstrfmt("%s-*", terms->term_good);
 	int no_checkout = ref_exists("BISECT_HEAD");
+	struct child_process cmd = CHILD_PROCESS_INIT;

 	for_each_glob_ref_in(get_first_good, good_glob, "refs/bisect/",
 			     &good_rev);
@@ -1164,8 +1162,10 @@ static int verify_good(const struct bisect_terms *terms,
 	if (res != BISECT_OK)
 		return -1;

-	printf(_("running %s\n"), quoted_argv[0]);
-	rc = run_command_v_opt(quoted_argv, RUN_USING_SHELL);
+	printf(_("running %s\n"), command);
+	strvec_push(&cmd.args, command);
+	cmd.use_shell = 1;
+	rc = run_command(&cmd);

 	res = bisect_checkout(&current_rev, no_checkout);
 	if (res != BISECT_OK)
@@ -1178,7 +1178,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 {
 	int res = BISECT_OK;
 	struct strbuf command = STRBUF_INIT;
-	struct strvec run_args = STRVEC_INIT;
 	const char *new_state;
 	int temporary_stdout_fd, saved_stdout;
 	int is_first_run = 1;
@@ -1193,11 +1192,13 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		return BISECT_FAILED;
 	}

-	strvec_push(&run_args, command.buf);
-
 	while (1) {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
 		printf(_("running %s\n"), command.buf);
-		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);
+		strvec_push(&cmd.args, command.buf);
+		cmd.use_shell = 1;
+		res = run_command(&cmd);

 		/*
 		 * Exit code 126 and 127 can either come from the shell
@@ -1207,7 +1208,7 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		 * missing or non-executable script.
 		 */
 		if (is_first_run && (res == 126 || res == 127)) {
-			int rc = verify_good(terms, run_args.v);
+			int rc = verify_good(terms, command.buf);
 			is_first_run = 0;
 			if (rc < 0) {
 				error(_("unable to verify '%s' on good"
@@ -1274,7 +1275,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 	}

 	strbuf_release(&command);
-	strvec_clear(&run_args);
 	return res;
 }

diff --git a/builtin/clone.c b/builtin/clone.c
index ed8d44bb6a..219cfbd735 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -651,9 +651,9 @@ static void update_head(const struct ref *our, const struct ref *remote,

 static int git_sparse_checkout_init(const char *repo)
 {
-	struct strvec argv = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int result = 0;
-	strvec_pushl(&argv, "-C", repo, "sparse-checkout", "set", NULL);
+	strvec_pushl(&cmd.args, "-C", repo, "sparse-checkout", "set", NULL);

 	/*
 	 * We must apply the setting in the current process
@@ -661,12 +661,12 @@ static int git_sparse_checkout_init(const char *repo)
 	 */
 	core_apply_sparse_checkout = 1;

-	if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+	cmd.git_cmd = 1;
+	if (run_command(&cmd)) {
 		error(_("failed to initialize sparse-checkout"));
 		result = 1;
 	}

-	strvec_clear(&argv);
 	return result;
 }

@@ -731,37 +731,38 @@ static int checkout(int submodule_progress, int filter_submodules)
 			   oid_to_hex(&oid), "1", NULL);

 	if (!err && (option_recurse_submodules.nr > 0)) {
-		struct strvec args = STRVEC_INIT;
-		strvec_pushl(&args, "submodule", "update", "--require-init", "--recursive", NULL);
+		struct child_process cmd = CHILD_PROCESS_INIT;
+		strvec_pushl(&cmd.args, "submodule", "update", "--require-init",
+			     "--recursive", NULL);

 		if (option_shallow_submodules == 1)
-			strvec_push(&args, "--depth=1");
+			strvec_push(&cmd.args, "--depth=1");

 		if (max_jobs != -1)
-			strvec_pushf(&args, "--jobs=%d", max_jobs);
+			strvec_pushf(&cmd.args, "--jobs=%d", max_jobs);

 		if (submodule_progress)
-			strvec_push(&args, "--progress");
+			strvec_push(&cmd.args, "--progress");

 		if (option_verbosity < 0)
-			strvec_push(&args, "--quiet");
+			strvec_push(&cmd.args, "--quiet");

 		if (option_remote_submodules) {
-			strvec_push(&args, "--remote");
-			strvec_push(&args, "--no-fetch");
+			strvec_push(&cmd.args, "--remote");
+			strvec_push(&cmd.args, "--no-fetch");
 		}

 		if (filter_submodules && filter_options.choice)
-			strvec_pushf(&args, "--filter=%s",
+			strvec_pushf(&cmd.args, "--filter=%s",
 				     expand_list_objects_filter_spec(&filter_options));

 		if (option_single_branch >= 0)
-			strvec_push(&args, option_single_branch ?
+			strvec_push(&cmd.args, option_single_branch ?
 					       "--single-branch" :
 					       "--no-single-branch");

-		err = run_command_v_opt(args.v, RUN_GIT_CMD);
-		strvec_clear(&args);
+		cmd.git_cmd = 1;
+		err = run_command(&cmd);
 	}

 	return err;
@@ -862,11 +863,15 @@ static void write_refspec_config(const char *src_ref_prefix,

 static void dissociate_from_references(void)
 {
-	static const char* argv[] = { "repack", "-a", "-d", NULL };
 	char *alternates = git_pathdup("objects/info/alternates");

 	if (!access(alternates, F_OK)) {
-		if (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		strvec_pushl(&cmd.args, "repack", "-a", "-d", NULL);
+		cmd.git_cmd = 1;
+		cmd.no_stdin = 1;
+		if (run_command(&cmd))
 			die(_("cannot repack to clean up"));
 		if (unlink(alternates) && errno != ENOENT)
 			die_errno(_("cannot unlink temporary alternates file"));
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4b10ad1a36..d6c262e15f 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -44,8 +44,10 @@ static int difftool_config(const char *var, const char *value, void *cb)

 static int print_tool_help(void)
 {
-	const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
-	return run_command_v_opt(argv, RUN_GIT_CMD);
+	struct child_process cmd = CHILD_PROCESS_INIT;
+	strvec_pushl(&cmd.args, "mergetool", "--tool-help=diff", NULL);
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 static int parse_index_info(char *p, int *mode1, int *mode2,
@@ -360,8 +362,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct pair_entry *entry;
 	struct index_state wtindex;
 	struct checkout lstate, rstate;
-	int flags = RUN_GIT_CMD, err = 0;
-	const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
+	int err = 0;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct hashmap wt_modified, tmp_modified;
 	int indices_loaded = 0;

@@ -563,16 +565,18 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	}

 	strbuf_setlen(&ldir, ldir_len);
-	helper_argv[1] = ldir.buf;
 	strbuf_setlen(&rdir, rdir_len);
-	helper_argv[2] = rdir.buf;

 	if (extcmd) {
-		helper_argv[0] = extcmd;
-		flags = 0;
-	} else
+		strvec_push(&cmd.args, extcmd);
+	} else {
+		strvec_push(&cmd.args, "difftool--helper");
+		cmd.git_cmd = 1;
 		setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
-	ret = run_command_v_opt(helper_argv, flags);
+	}
+	strvec_pushl(&cmd.args, ldir.buf, rdir.buf, NULL);
+
+	ret = run_command(&cmd);

 	/* TODO: audit for interaction with sparse-index. */
 	ensure_full_index(&wtindex);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a0fca93bb6..dd060dd65a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1965,14 +1965,17 @@ static int fetch_multiple(struct string_list *list, int max_children)
 	} else
 		for (i = 0; i < list->nr; i++) {
 			const char *name = list->items[i].string;
-			strvec_push(&argv, name);
+			struct child_process cmd = CHILD_PROCESS_INIT;
+
+			strvec_pushv(&cmd.args, argv.v);
+			strvec_push(&cmd.args, name);
 			if (verbosity >= 0)
 				printf(_("Fetching %s\n"), name);
-			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+			cmd.git_cmd = 1;
+			if (run_command(&cmd)) {
 				error(_("could not fetch %s"), name);
 				result = 1;
 			}
-			strvec_pop(&argv);
 		}

 	strvec_clear(&argv);
diff --git a/builtin/gc.c b/builtin/gc.c
index 243ee85d28..b47e53c210 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -167,9 +167,11 @@ static void gc_config(void)
 struct maintenance_run_opts;
 static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)
 {
-	const char *argv[] = { "pack-refs", "--all", "--prune", NULL };
+	struct child_process cmd = CHILD_PROCESS_INIT;

-	return run_command_v_opt(argv, RUN_GIT_CMD);
+	strvec_pushl(&cmd.args, "pack-refs", "--all", "--prune", NULL);
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 static int too_many_loose_objects(void)
@@ -521,6 +523,16 @@ static int report_last_gc_error(void)
 	return ret;
 }

+static void run_git_or_die(const char **argv)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	strvec_pushv(&cmd.args, argv);
+	cmd.git_cmd = 1;
+	if (run_command(&cmd))
+		die(FAILED_RUN, argv[0]);
+}
+
 static void gc_before_repack(void)
 {
 	/*
@@ -535,8 +547,8 @@ static void gc_before_repack(void)
 	if (pack_refs && maintenance_task_pack_refs(NULL))
 		die(FAILED_RUN, "pack-refs");

-	if (prune_reflogs && run_command_v_opt(reflog.v, RUN_GIT_CMD))
-		die(FAILED_RUN, reflog.v[0]);
+	if (prune_reflogs)
+		run_git_or_die(reflog.v);
 }

 int cmd_gc(int argc, const char **argv, const char *prefix)
@@ -671,8 +683,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	gc_before_repack();

 	if (!repository_format_precious_objects) {
-		if (run_command_v_opt(repack.v,
-				      RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE))
+		struct child_process repack_cmd = CHILD_PROCESS_INIT;
+
+		strvec_pushv(&repack_cmd.args, repack.v);
+		repack_cmd.git_cmd = 1;
+		repack_cmd.close_object_store = 1;
+		if (run_command(&repack_cmd))
 			die(FAILED_RUN, repack.v[0]);

 		if (prune_expire) {
@@ -683,19 +699,16 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			if (has_promisor_remote())
 				strvec_push(&prune,
 					    "--exclude-promisor-objects");
-			if (run_command_v_opt(prune.v, RUN_GIT_CMD))
-				die(FAILED_RUN, prune.v[0]);
+			run_git_or_die(prune.v);
 		}
 	}

 	if (prune_worktrees_expire) {
 		strvec_push(&prune_worktrees, prune_worktrees_expire);
-		if (run_command_v_opt(prune_worktrees.v, RUN_GIT_CMD))
-			die(FAILED_RUN, prune_worktrees.v[0]);
+		run_git_or_die(prune_worktrees.v);
 	}

-	if (run_command_v_opt(rerere.v, RUN_GIT_CMD))
-		die(FAILED_RUN, rerere.v[0]);
+	run_git_or_die(rerere.v);

 	report_garbage = report_pack_garbage;
 	reprepare_packed_git(the_repository);
@@ -1910,20 +1923,16 @@ static char *schtasks_task_name(const char *frequency)
 static int schtasks_remove_task(enum schedule_priority schedule)
 {
 	const char *cmd = "schtasks";
-	int result;
-	struct strvec args = STRVEC_INIT;
+	struct child_process child = CHILD_PROCESS_INIT;
 	const char *frequency = get_frequency(schedule);
 	char *name = schtasks_task_name(frequency);

 	get_schedule_cmd(&cmd, NULL);
-	strvec_split(&args, cmd);
-	strvec_pushl(&args, "/delete", "/tn", name, "/f", NULL);
-
-	result = run_command_v_opt(args.v, 0);
-
-	strvec_clear(&args);
+	strvec_split(&child.args, cmd);
+	strvec_pushl(&child.args, "/delete", "/tn", name, "/f", NULL);
 	free(name);
-	return result;
+
+	return run_command(&child);
 }

 static int schtasks_remove_tasks(void)
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index c0383fe9df..012f52bd00 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -12,6 +12,7 @@ static int merge_entry(int pos, const char *path)
 	const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
 	char hexbuf[4][GIT_MAX_HEXSZ + 1];
 	char ownbuf[4][60];
+	struct child_process cmd = CHILD_PROCESS_INIT;

 	if (pos >= active_nr)
 		die("git merge-index: %s not in the cache", path);
@@ -31,7 +32,8 @@ static int merge_entry(int pos, const char *path)
 	if (!found)
 		die("git merge-index: %s not in the cache", path);

-	if (run_command_v_opt(arguments, 0)) {
+	strvec_pushv(&cmd.args, arguments);
+	if (run_command(&cmd)) {
 		if (one_shot)
 			err++;
 		else {
diff --git a/builtin/merge.c b/builtin/merge.c
index 5900b81729..5e14decc22 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -347,58 +347,52 @@ static int save_state(struct object_id *stash)

 static void read_empty(const struct object_id *oid, int verbose)
 {
-	int i = 0;
-	const char *args[7];
+	struct child_process cmd = CHILD_PROCESS_INIT;

-	args[i++] = "read-tree";
+	strvec_push(&cmd.args, "read-tree");
 	if (verbose)
-		args[i++] = "-v";
-	args[i++] = "-m";
-	args[i++] = "-u";
-	args[i++] = empty_tree_oid_hex();
-	args[i++] = oid_to_hex(oid);
-	args[i] = NULL;
+		strvec_push(&cmd.args, "-v");
+	strvec_pushl(&cmd.args, "-m", "-u", empty_tree_oid_hex(),
+		     oid_to_hex(oid), NULL);

-	if (run_command_v_opt(args, RUN_GIT_CMD))
+	cmd.git_cmd = 1;
+	if (run_command(&cmd))
 		die(_("read-tree failed"));
 }

 static void reset_hard(const struct object_id *oid, int verbose)
 {
-	int i = 0;
-	const char *args[6];
+	struct child_process cmd = CHILD_PROCESS_INIT;

-	args[i++] = "read-tree";
+	strvec_push(&cmd.args, "read-tree");
 	if (verbose)
-		args[i++] = "-v";
-	args[i++] = "--reset";
-	args[i++] = "-u";
-	args[i++] = oid_to_hex(oid);
-	args[i] = NULL;
+		strvec_push(&cmd.args, "-v");
+	strvec_pushl(&cmd.args, "--reset", "-u", oid_to_hex(oid), NULL);

-	if (run_command_v_opt(args, RUN_GIT_CMD))
+	cmd.git_cmd = 1;
+	if (run_command(&cmd))
 		die(_("read-tree failed"));
 }

 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	struct strvec args = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;

 	reset_hard(head, 1);

 	if (is_null_oid(stash))
 		goto refresh_cache;

-	strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
-	strvec_push(&args, oid_to_hex(stash));
+	strvec_pushl(&cmd.args, "stash", "apply", "--index", "--quiet", NULL);
+	strvec_push(&cmd.args, oid_to_hex(stash));

 	/*
 	 * It is OK to ignore error here, for example when there was
 	 * nothing to restore.
 	 */
-	run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
+	cmd.git_cmd = 1;
+	run_command(&cmd);

 refresh_cache:
 	if (discard_cache() < 0 || read_cache() < 0)
diff --git a/builtin/pull.c b/builtin/pull.c
index 403a24d7ca..b21edd767a 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -515,76 +515,75 @@ static void parse_repo_refspecs(int argc, const char **argv, const char **repo,
  */
 static int run_fetch(const char *repo, const char **refspecs)
 {
-	struct strvec args = STRVEC_INIT;
-	int ret;
+	struct child_process cmd = CHILD_PROCESS_INIT;

-	strvec_pushl(&args, "fetch", "--update-head-ok", NULL);
+	strvec_pushl(&cmd.args, "fetch", "--update-head-ok", NULL);

 	/* Shared options */
-	argv_push_verbosity(&args);
+	argv_push_verbosity(&cmd.args);
 	if (opt_progress)
-		strvec_push(&args, opt_progress);
+		strvec_push(&cmd.args, opt_progress);

 	/* Options passed to git-fetch */
 	if (opt_all)
-		strvec_push(&args, opt_all);
+		strvec_push(&cmd.args, opt_all);
 	if (opt_append)
-		strvec_push(&args, opt_append);
+		strvec_push(&cmd.args, opt_append);
 	if (opt_upload_pack)
-		strvec_push(&args, opt_upload_pack);
-	argv_push_force(&args);
+		strvec_push(&cmd.args, opt_upload_pack);
+	argv_push_force(&cmd.args);
 	if (opt_tags)
-		strvec_push(&args, opt_tags);
+		strvec_push(&cmd.args, opt_tags);
 	if (opt_prune)
-		strvec_push(&args, opt_prune);
+		strvec_push(&cmd.args, opt_prune);
 	if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
 		switch (recurse_submodules_cli) {
 		case RECURSE_SUBMODULES_ON:
-			strvec_push(&args, "--recurse-submodules=on");
+			strvec_push(&cmd.args, "--recurse-submodules=on");
 			break;
 		case RECURSE_SUBMODULES_OFF:
-			strvec_push(&args, "--recurse-submodules=no");
+			strvec_push(&cmd.args, "--recurse-submodules=no");
 			break;
 		case RECURSE_SUBMODULES_ON_DEMAND:
-			strvec_push(&args, "--recurse-submodules=on-demand");
+			strvec_push(&cmd.args, "--recurse-submodules=on-demand");
 			break;
 		default:
 			BUG("submodule recursion option not understood");
 		}
 	if (max_children)
-		strvec_push(&args, max_children);
+		strvec_push(&cmd.args, max_children);
 	if (opt_dry_run)
-		strvec_push(&args, "--dry-run");
+		strvec_push(&cmd.args, "--dry-run");
 	if (opt_keep)
-		strvec_push(&args, opt_keep);
+		strvec_push(&cmd.args, opt_keep);
 	if (opt_depth)
-		strvec_push(&args, opt_depth);
+		strvec_push(&cmd.args, opt_depth);
 	if (opt_unshallow)
-		strvec_push(&args, opt_unshallow);
+		strvec_push(&cmd.args, opt_unshallow);
 	if (opt_update_shallow)
-		strvec_push(&args, opt_update_shallow);
+		strvec_push(&cmd.args, opt_update_shallow);
 	if (opt_refmap)
-		strvec_push(&args, opt_refmap);
+		strvec_push(&cmd.args, opt_refmap);
 	if (opt_ipv4)
-		strvec_push(&args, opt_ipv4);
+		strvec_push(&cmd.args, opt_ipv4);
 	if (opt_ipv6)
-		strvec_push(&args, opt_ipv6);
+		strvec_push(&cmd.args, opt_ipv6);
 	if (opt_show_forced_updates > 0)
-		strvec_push(&args, "--show-forced-updates");
+		strvec_push(&cmd.args, "--show-forced-updates");
 	else if (opt_show_forced_updates == 0)
-		strvec_push(&args, "--no-show-forced-updates");
+		strvec_push(&cmd.args, "--no-show-forced-updates");
 	if (set_upstream)
-		strvec_push(&args, set_upstream);
-	strvec_pushv(&args, opt_fetch.v);
+		strvec_push(&cmd.args, set_upstream);
+	strvec_pushv(&cmd.args, opt_fetch.v);

 	if (repo) {
-		strvec_push(&args, repo);
-		strvec_pushv(&args, refspecs);
+		strvec_push(&cmd.args, repo);
+		strvec_pushv(&cmd.args, refspecs);
 	} else if (*refspecs)
 		BUG("refspecs without repo?");
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);
-	strvec_clear(&args);
-	return ret;
+	cmd.git_cmd = 1;
+	cmd.close_object_store = 1;
+	return run_command(&cmd);
 }

 /**
@@ -653,52 +652,50 @@ static int update_submodules(void)
  */
 static int run_merge(void)
 {
-	int ret;
-	struct strvec args = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;

-	strvec_pushl(&args, "merge", NULL);
+	strvec_pushl(&cmd.args, "merge", NULL);

 	/* Shared options */
-	argv_push_verbosity(&args);
+	argv_push_verbosity(&cmd.args);
 	if (opt_progress)
-		strvec_push(&args, opt_progress);
+		strvec_push(&cmd.args, opt_progress);

 	/* Options passed to git-merge */
 	if (opt_diffstat)
-		strvec_push(&args, opt_diffstat);
+		strvec_push(&cmd.args, opt_diffstat);
 	if (opt_log)
-		strvec_push(&args, opt_log);
+		strvec_push(&cmd.args, opt_log);
 	if (opt_signoff)
-		strvec_push(&args, opt_signoff);
+		strvec_push(&cmd.args, opt_signoff);
 	if (opt_squash)
-		strvec_push(&args, opt_squash);
+		strvec_push(&cmd.args, opt_squash);
 	if (opt_commit)
-		strvec_push(&args, opt_commit);
+		strvec_push(&cmd.args, opt_commit);
 	if (opt_edit)
-		strvec_push(&args, opt_edit);
+		strvec_push(&cmd.args, opt_edit);
 	if (cleanup_arg)
-		strvec_pushf(&args, "--cleanup=%s", cleanup_arg);
+		strvec_pushf(&cmd.args, "--cleanup=%s", cleanup_arg);
 	if (opt_ff)
-		strvec_push(&args, opt_ff);
+		strvec_push(&cmd.args, opt_ff);
 	if (opt_verify)
-		strvec_push(&args, opt_verify);
+		strvec_push(&cmd.args, opt_verify);
 	if (opt_verify_signatures)
-		strvec_push(&args, opt_verify_signatures);
-	strvec_pushv(&args, opt_strategies.v);
-	strvec_pushv(&args, opt_strategy_opts.v);
+		strvec_push(&cmd.args, opt_verify_signatures);
+	strvec_pushv(&cmd.args, opt_strategies.v);
+	strvec_pushv(&cmd.args, opt_strategy_opts.v);
 	if (opt_gpg_sign)
-		strvec_push(&args, opt_gpg_sign);
+		strvec_push(&cmd.args, opt_gpg_sign);
 	if (opt_autostash == 0)
-		strvec_push(&args, "--no-autostash");
+		strvec_push(&cmd.args, "--no-autostash");
 	else if (opt_autostash == 1)
-		strvec_push(&args, "--autostash");
+		strvec_push(&cmd.args, "--autostash");
 	if (opt_allow_unrelated_histories > 0)
-		strvec_push(&args, "--allow-unrelated-histories");
+		strvec_push(&cmd.args, "--allow-unrelated-histories");

-	strvec_push(&args, "FETCH_HEAD");
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
-	return ret;
+	strvec_push(&cmd.args, "FETCH_HEAD");
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 /**
@@ -879,43 +876,41 @@ static int get_rebase_newbase_and_upstream(struct object_id *newbase,
 static int run_rebase(const struct object_id *newbase,
 		const struct object_id *upstream)
 {
-	int ret;
-	struct strvec args = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;

-	strvec_push(&args, "rebase");
+	strvec_push(&cmd.args, "rebase");

 	/* Shared options */
-	argv_push_verbosity(&args);
+	argv_push_verbosity(&cmd.args);

 	/* Options passed to git-rebase */
 	if (opt_rebase == REBASE_MERGES)
-		strvec_push(&args, "--rebase-merges");
+		strvec_push(&cmd.args, "--rebase-merges");
 	else if (opt_rebase == REBASE_INTERACTIVE)
-		strvec_push(&args, "--interactive");
+		strvec_push(&cmd.args, "--interactive");
 	if (opt_diffstat)
-		strvec_push(&args, opt_diffstat);
-	strvec_pushv(&args, opt_strategies.v);
-	strvec_pushv(&args, opt_strategy_opts.v);
+		strvec_push(&cmd.args, opt_diffstat);
+	strvec_pushv(&cmd.args, opt_strategies.v);
+	strvec_pushv(&cmd.args, opt_strategy_opts.v);
 	if (opt_gpg_sign)
-		strvec_push(&args, opt_gpg_sign);
+		strvec_push(&cmd.args, opt_gpg_sign);
 	if (opt_signoff)
-		strvec_push(&args, opt_signoff);
+		strvec_push(&cmd.args, opt_signoff);
 	if (opt_autostash == 0)
-		strvec_push(&args, "--no-autostash");
+		strvec_push(&cmd.args, "--no-autostash");
 	else if (opt_autostash == 1)
-		strvec_push(&args, "--autostash");
+		strvec_push(&cmd.args, "--autostash");
 	if (opt_verify_signatures &&
 	    !strcmp(opt_verify_signatures, "--verify-signatures"))
 		warning(_("ignoring --verify-signatures for rebase"));

-	strvec_push(&args, "--onto");
-	strvec_push(&args, oid_to_hex(newbase));
+	strvec_push(&cmd.args, "--onto");
+	strvec_push(&cmd.args, oid_to_hex(newbase));

-	strvec_push(&args, oid_to_hex(upstream));
+	strvec_push(&cmd.args, oid_to_hex(upstream));

-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
-	return ret;
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 static int get_can_ff(struct object_id *orig_head,
diff --git a/builtin/remote.c b/builtin/remote.c
index 910f7b9316..0cde2e244a 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -92,13 +92,15 @@ static int verbose;

 static int fetch_remote(const char *name)
 {
-	const char *argv[] = { "fetch", name, NULL, NULL };
-	if (verbose) {
-		argv[1] = "-v";
-		argv[2] = name;
-	}
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	strvec_push(&cmd.args, "fetch");
+	if (verbose)
+		strvec_push(&cmd.args, "-v");
+	strvec_push(&cmd.args, name);
 	printf_ln(_("Updating %s"), name);
-	if (run_command_v_opt(argv, RUN_GIT_CMD))
+	cmd.git_cmd = 1;
+	if (run_command(&cmd))
 		return error(_("Could not fetch %s"), name);
 	return 0;
 }
@@ -1508,37 +1510,35 @@ static int update(int argc, const char **argv, const char *prefix)
 			 N_("prune remotes after fetching")),
 		OPT_END()
 	};
-	struct strvec fetch_argv = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int default_defined = 0;
-	int retval;

 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_remote_update_usage,
 			     PARSE_OPT_KEEP_ARGV0);

-	strvec_push(&fetch_argv, "fetch");
+	strvec_push(&cmd.args, "fetch");

 	if (prune != -1)
-		strvec_push(&fetch_argv, prune ? "--prune" : "--no-prune");
+		strvec_push(&cmd.args, prune ? "--prune" : "--no-prune");
 	if (verbose)
-		strvec_push(&fetch_argv, "-v");
-	strvec_push(&fetch_argv, "--multiple");
+		strvec_push(&cmd.args, "-v");
+	strvec_push(&cmd.args, "--multiple");
 	if (argc < 2)
-		strvec_push(&fetch_argv, "default");
+		strvec_push(&cmd.args, "default");
 	for (i = 1; i < argc; i++)
-		strvec_push(&fetch_argv, argv[i]);
+		strvec_push(&cmd.args, argv[i]);

-	if (strcmp(fetch_argv.v[fetch_argv.nr-1], "default") == 0) {
+	if (strcmp(cmd.args.v[cmd.args.nr-1], "default") == 0) {
 		git_config(get_remote_default, &default_defined);
 		if (!default_defined) {
-			strvec_pop(&fetch_argv);
-			strvec_push(&fetch_argv, "--all");
+			strvec_pop(&cmd.args);
+			strvec_push(&cmd.args, "--all");
 		}
 	}

-	retval = run_command_v_opt(fetch_argv.v, RUN_GIT_CMD);
-	strvec_clear(&fetch_argv);
-	return retval;
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 static int remove_all_fetch_refspecs(const char *key)
diff --git a/compat/mingw.c b/compat/mingw.c
index 901375d584..d614f156df 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -196,16 +196,19 @@ static int read_yes_no_answer(void)
 static int ask_yes_no_if_possible(const char *format, ...)
 {
 	char question[4096];
-	const char *retry_hook[] = { NULL, NULL, NULL };
+	const char *retry_hook;
 	va_list args;

 	va_start(args, format);
 	vsnprintf(question, sizeof(question), format, args);
 	va_end(args);

-	if ((retry_hook[0] = mingw_getenv("GIT_ASK_YESNO"))) {
-		retry_hook[1] = question;
-		return !run_command_v_opt(retry_hook, 0);
+	retry_hook = mingw_getenv("GIT_ASK_YESNO");
+	if (retry_hook) {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		strvec_pushl(&cmd.args, retry_hook, question, NULL);
+		return !run_command(&cmd);
 	}

 	if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
diff --git a/diff.c b/diff.c
index 648f6717a5..8451c230d9 100644
--- a/diff.c
+++ b/diff.c
@@ -4278,35 +4278,34 @@ static void run_external_diff(const char *pgm,
 			      const char *xfrm_msg,
 			      struct diff_options *o)
 {
-	struct strvec argv = STRVEC_INIT;
-	struct strvec env = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;

-	strvec_push(&argv, pgm);
-	strvec_push(&argv, name);
+	strvec_push(&cmd.args, pgm);
+	strvec_push(&cmd.args, name);

 	if (one && two) {
-		add_external_diff_name(o->repo, &argv, name, one);
+		add_external_diff_name(o->repo, &cmd.args, name, one);
 		if (!other)
-			add_external_diff_name(o->repo, &argv, name, two);
+			add_external_diff_name(o->repo, &cmd.args, name, two);
 		else {
-			add_external_diff_name(o->repo, &argv, other, two);
-			strvec_push(&argv, other);
-			strvec_push(&argv, xfrm_msg);
+			add_external_diff_name(o->repo, &cmd.args, other, two);
+			strvec_push(&cmd.args, other);
+			strvec_push(&cmd.args, xfrm_msg);
 		}
 	}

-	strvec_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
-	strvec_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
+	strvec_pushf(&cmd.env, "GIT_DIFF_PATH_COUNTER=%d",
+		     ++o->diff_path_counter);
+	strvec_pushf(&cmd.env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);

 	diff_free_filespec_data(one);
 	diff_free_filespec_data(two);
-	if (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))
+	cmd.use_shell = 1;
+	if (run_command(&cmd))
 		die(_("external diff died, stopping at %s"), name);

 	remove_tempfile();
-	strvec_clear(&argv);
-	strvec_clear(&env);
 }

 static int similarity_index(struct diff_filepair *p)
diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
index 789e7397ba..8d0b0b157c 100644
--- a/fsmonitor-ipc.c
+++ b/fsmonitor-ipc.c
@@ -56,10 +56,14 @@ enum ipc_active_state fsmonitor_ipc__get_state(void)

 static int spawn_daemon(void)
 {
-	const char *args[] = { "fsmonitor--daemon", "start", NULL };
+	struct child_process cmd = CHILD_PROCESS_INIT;

-	return run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,
-				    "fsmonitor");
+	strvec_pushl(&cmd.args, "fsmonitor--daemon", "start", NULL);
+
+	cmd.git_cmd = 1;
+	cmd.no_stdin = 1;
+	cmd.trace2_child_class = "fsmonitor";
+	return run_command(&cmd);
 }

 int fsmonitor_ipc__send_query(const char *since_token,
diff --git a/git.c b/git.c
index da411c5382..ccf444417b 100644
--- a/git.c
+++ b/git.c
@@ -787,7 +787,7 @@ static int run_argv(int *argcp, const char ***argv)
 		if (!done_alias)
 			handle_builtin(*argcp, *argv);
 		else if (get_builtin(**argv)) {
-			struct strvec args = STRVEC_INIT;
+			struct child_process cmd = CHILD_PROCESS_INIT;
 			int i;

 			/*
@@ -804,18 +804,21 @@ static int run_argv(int *argcp, const char ***argv)

 			commit_pager_choice();

-			strvec_push(&args, "git");
+			strvec_push(&cmd.args, "git");
 			for (i = 0; i < *argcp; i++)
-				strvec_push(&args, (*argv)[i]);
+				strvec_push(&cmd.args, (*argv)[i]);

-			trace_argv_printf(args.v, "trace: exec:");
+			trace_argv_printf(cmd.args.v, "trace: exec:");

 			/*
 			 * if we fail because the command is not found, it is
 			 * OK to return. Otherwise, we just pass along the status code.
 			 */
-			i = run_command_v_opt_tr2(args.v, RUN_SILENT_EXEC_FAILURE |
-						  RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, "git_alias");
+			cmd.silent_exec_failure = 1;
+			cmd.clean_on_exit = 1;
+			cmd.wait_after_clean = 1;
+			cmd.trace2_child_class = "git_alias";
+			i = run_command(&cmd);
 			if (i >= 0 || errno != ENOENT)
 				exit(i);
 			die("could not execute builtin %s", **argv);
diff --git a/ll-merge.c b/ll-merge.c
index 8955d7e1f6..b20491043e 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -193,7 +193,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf_expand_dict_entry dict[6];
 	struct strbuf path_sq = STRBUF_INIT;
-	const char *args[] = { NULL, NULL };
+	struct child_process child = CHILD_PROCESS_INIT;
 	int status, fd, i;
 	struct stat st;
 	enum ll_merge_result ret;
@@ -219,8 +219,9 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,

 	strbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);

-	args[0] = cmd.buf;
-	status = run_command_v_opt(args, RUN_USING_SHELL);
+	strvec_push(&child.args, cmd.buf);
+	child.use_shell = 1;
+	status = run_command(&child);
 	fd = open(temp[1], O_RDONLY);
 	if (fd < 0)
 		goto bad;
diff --git a/merge.c b/merge.c
index 2382ff66d3..445b4f19aa 100644
--- a/merge.c
+++ b/merge.c
@@ -19,22 +19,22 @@ int try_merge_command(struct repository *r,
 		      const char **xopts, struct commit_list *common,
 		      const char *head_arg, struct commit_list *remotes)
 {
-	struct strvec args = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int i, ret;
 	struct commit_list *j;

-	strvec_pushf(&args, "merge-%s", strategy);
+	strvec_pushf(&cmd.args, "merge-%s", strategy);
 	for (i = 0; i < xopts_nr; i++)
-		strvec_pushf(&args, "--%s", xopts[i]);
+		strvec_pushf(&cmd.args, "--%s", xopts[i]);
 	for (j = common; j; j = j->next)
-		strvec_push(&args, merge_argument(j->item));
-	strvec_push(&args, "--");
-	strvec_push(&args, head_arg);
+		strvec_push(&cmd.args, merge_argument(j->item));
+	strvec_push(&cmd.args, "--");
+	strvec_push(&cmd.args, head_arg);
 	for (j = remotes; j; j = j->next)
-		strvec_push(&args, merge_argument(j->item));
+		strvec_push(&cmd.args, merge_argument(j->item));

-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
+	cmd.git_cmd = 1;
+	ret = run_command(&cmd);

 	discard_index(r->index);
 	if (repo_read_index(r) < 0)
diff --git a/run-command.c b/run-command.c
index 5ec3a46dcc..23e100dffc 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1004,41 +1004,6 @@ int run_command(struct child_process *cmd)
 	return finish_command(cmd);
 }

-int run_command_v_opt(const char **argv, int opt)
-{
-	return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
-}
-
-int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)
-{
-	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);
-}
-
-int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env)
-{
-	return run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);
-}
-
-int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
-				 const char *const *env, const char *tr2_class)
-{
-	struct child_process cmd = CHILD_PROCESS_INIT;
-	strvec_pushv(&cmd.args, argv);
-	cmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
-	cmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
-	cmd.stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
-	cmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
-	cmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;
-	cmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
-	cmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
-	cmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
-	cmd.dir = dir;
-	if (env)
-		strvec_pushv(&cmd.env, (const char **)env);
-	cmd.trace2_child_class = tr2_class;
-	return run_command(&cmd);
-}
-
 #ifndef NO_PTHREADS
 static pthread_t main_thread;
 static int main_thread_set;
diff --git a/run-command.h b/run-command.h
index 0e85e5846a..57354fb7df 100644
--- a/run-command.h
+++ b/run-command.h
@@ -224,36 +224,6 @@ int run_command(struct child_process *);
  */
 int run_auto_maintenance(int quiet);

-#define RUN_COMMAND_NO_STDIN		(1<<0)
-#define RUN_GIT_CMD			(1<<1)
-#define RUN_COMMAND_STDOUT_TO_STDERR	(1<<2)
-#define RUN_SILENT_EXEC_FAILURE		(1<<3)
-#define RUN_USING_SHELL			(1<<4)
-#define RUN_CLEAN_ON_EXIT		(1<<5)
-#define RUN_WAIT_AFTER_CLEAN		(1<<6)
-#define RUN_CLOSE_OBJECT_STORE		(1<<7)
-
-/**
- * Convenience functions that encapsulate a sequence of
- * start_command() followed by finish_command(). The argument argv
- * specifies the program and its arguments. The argument opt is zero
- * or more of the flags `RUN_COMMAND_NO_STDIN`, `RUN_GIT_CMD`,
- * `RUN_COMMAND_STDOUT_TO_STDERR`, or `RUN_SILENT_EXEC_FAILURE`
- * that correspond to the members .no_stdin, .git_cmd,
- * .stdout_to_stderr, .silent_exec_failure of `struct child_process`.
- * The argument dir corresponds the member .dir. The argument env
- * corresponds to the member .env.
- */
-int run_command_v_opt(const char **argv, int opt);
-int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class);
-/*
- * env (the environment) is to be formatted like environ: "VAR=VALUE".
- * To unset an environment variable use just "VAR".
- */
-int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);
-int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
-				 const char *const *env, const char *tr2_class);
-
 /**
  * Execute the given command, sending "in" to its stdin, and capturing its
  * stdout and stderr in the "out" and "err" strbufs. Any of the three may
diff --git a/scalar.c b/scalar.c
index 6de9c0ee52..03f9e480dd 100644
--- a/scalar.c
+++ b/scalar.c
@@ -69,21 +69,18 @@ static void setup_enlistment_directory(int argc, const char **argv,

 static int run_git(const char *arg, ...)
 {
-	struct strvec argv = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	va_list args;
 	const char *p;
-	int res;

 	va_start(args, arg);
-	strvec_push(&argv, arg);
+	strvec_push(&cmd.args, arg);
 	while ((p = va_arg(args, const char *)))
-		strvec_push(&argv, p);
+		strvec_push(&cmd.args, p);
 	va_end(args);

-	res = run_command_v_opt(argv.v, RUN_GIT_CMD);
-
-	strvec_clear(&argv);
-	return res;
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 struct scalar_config {
diff --git a/sequencer.c b/sequencer.c
index debb2ecbaf..9b9d6a5582 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3183,18 +3183,15 @@ static int rollback_is_safe(void)

 static int reset_merge(const struct object_id *oid)
 {
-	int ret;
-	struct strvec argv = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;

-	strvec_pushl(&argv, "reset", "--merge", NULL);
+	strvec_pushl(&cmd.args, "reset", "--merge", NULL);

 	if (!is_null_oid(oid))
-		strvec_push(&argv, oid_to_hex(oid));
-
-	ret = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
+		strvec_push(&cmd.args, oid_to_hex(oid));

-	return ret;
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 static int rollback_single_pick(struct repository *r)
@@ -3558,12 +3555,13 @@ static int error_failed_squash(struct repository *r,

 static int do_exec(struct repository *r, const char *command_line)
 {
-	const char *child_argv[] = { NULL, NULL };
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int dirty, status;

 	fprintf(stderr, _("Executing: %s\n"), command_line);
-	child_argv[0] = command_line;
-	status = run_command_v_opt(child_argv, RUN_USING_SHELL);
+	strvec_push(&cmd.args, command_line);
+	cmd.use_shell = 1;
+	status = run_command(&cmd);

 	/* force re-reading of the cache */
 	if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
@@ -4867,14 +4865,13 @@ static int pick_commits(struct repository *r,

 static int continue_single_pick(struct repository *r, struct replay_opts *opts)
 {
-	struct strvec argv = STRVEC_INIT;
-	int ret;
+	struct child_process cmd = CHILD_PROCESS_INIT;

 	if (!refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
 	    !refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD"))
 		return error(_("no cherry-pick or revert in progress"));

-	strvec_push(&argv, "commit");
+	strvec_push(&cmd.args, "commit");

 	/*
 	 * continue_single_pick() handles the case of recovering from a
@@ -4887,11 +4884,10 @@ static int continue_single_pick(struct repository *r, struct replay_opts *opts)
 		 * Include --cleanup=strip as well because we don't want the
 		 * "# Conflicts:" messages.
 		 */
-		strvec_pushl(&argv, "--no-edit", "--cleanup=strip", NULL);
+		strvec_pushl(&cmd.args, "--no-edit", "--cleanup=strip", NULL);

-	ret = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-	return ret;
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 static int commit_staged_changes(struct repository *r,
diff --git a/shell.c b/shell.c
index 811e13b9c9..4abc14eeee 100644
--- a/shell.c
+++ b/shell.c
@@ -50,21 +50,24 @@ static void cd_to_homedir(void)
 static void run_shell(void)
 {
 	int done = 0;
-	static const char *help_argv[] = { HELP_COMMAND, NULL };
+	struct child_process help_cmd = CHILD_PROCESS_INIT;

 	if (!access(NOLOGIN_COMMAND, F_OK)) {
 		/* Interactive login disabled. */
-		const char *argv[] = { NOLOGIN_COMMAND, NULL };
+		struct child_process nologin_cmd = CHILD_PROCESS_INIT;
 		int status;

-		status = run_command_v_opt(argv, 0);
+		strvec_push(&nologin_cmd.args, NOLOGIN_COMMAND);
+		status = run_command(&nologin_cmd);
 		if (status < 0)
 			exit(127);
 		exit(status);
 	}

 	/* Print help if enabled */
-	run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+	strvec_push(&help_cmd.args, HELP_COMMAND);
+	help_cmd.silent_exec_failure = 1;
+	run_command(&help_cmd);

 	do {
 		struct strbuf line = STRBUF_INIT;
@@ -99,9 +102,13 @@ static void run_shell(void)
 			   !strcmp(prog, "exit") || !strcmp(prog, "bye")) {
 			done = 1;
 		} else if (is_valid_cmd_name(prog)) {
+			struct child_process cmd = CHILD_PROCESS_INIT;
+
 			full_cmd = make_cmd(prog);
 			argv[0] = full_cmd;
-			code = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);
+			strvec_pushv(&cmd.args, argv);
+			cmd.silent_exec_failure = 1;
+			code = run_command(&cmd);
 			if (code == -1 && errno == ENOENT) {
 				fprintf(stderr, "unrecognized command '%s'\n", prog);
 			}
diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c
index 12beee99ad..42185f3fc0 100644
--- a/t/helper/test-fake-ssh.c
+++ b/t/helper/test-fake-ssh.c
@@ -8,7 +8,7 @@ int cmd_main(int argc, const char **argv)
 	struct strbuf buf = STRBUF_INIT;
 	FILE *f;
 	int i;
-	const char *child_argv[] = { NULL, NULL };
+	struct child_process cmd = CHILD_PROCESS_INIT;

 	/* First, print all parameters into $TRASH_DIRECTORY/ssh-output */
 	if (!trash_directory)
@@ -25,6 +25,7 @@ int cmd_main(int argc, const char **argv)
 	/* Now, evaluate the *last* parameter */
 	if (argc < 2)
 		return 0;
-	child_argv[0] = argv[argc - 1];
-	return run_command_v_opt(child_argv, RUN_USING_SHELL);
+	strvec_push(&cmd.args, argv[argc - 1]);
+	cmd.use_shell = 1;
+	return run_command(&cmd);
 }
diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index a714130ece..94fd8ccf51 100644
--- a/t/helper/test-trace2.c
+++ b/t/helper/test-trace2.c
@@ -132,6 +132,7 @@ static int ut_003error(int argc, const char **argv)
  */
 static int ut_004child(int argc, const char **argv)
 {
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int result;

 	/*
@@ -141,7 +142,8 @@ static int ut_004child(int argc, const char **argv)
 	if (!argc)
 		return 0;

-	result = run_command_v_opt(argv, 0);
+	strvec_pushv(&cmd.args, argv);
+	result = run_command(&cmd);
 	exit(result);
 }

diff --git a/tmp-objdir.h b/tmp-objdir.h
index 76efc7edee..615b188573 100644
--- a/tmp-objdir.h
+++ b/tmp-objdir.h
@@ -11,8 +11,8 @@
  * Example:
  *
  *	struct tmp_objdir *t = tmp_objdir_create("incoming");
- *	if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&
- *	    !tmp_objdir_migrate(t))
+ *	strvec_pushv(&cmd.env, tmp_objdir_env(t));
+ *	if (!run_command(&cmd)) && !tmp_objdir_migrate(t))
  *		printf("success!\n");
  *	else
  *		die("failed...tmp_objdir will clean up for us");
--
2.38.0

Re: [PATCH v2] bisect--helper: plug strvec leak

[Jeff King]

On Sat, Oct 15, 2022 at 08:51:34AM +0200, René Scharfe wrote:

> Am 14.10.22 um 21:44 schrieb Jeff King:
> > And I'd be happy to see all of the run_command_v() variants go away
> > entirely. It does save a few lines, but with modern niceties, it's
> > not very many, and it's much less flexible.
> That would look something like below.
>
> [...]
>  27 files changed, 330 insertions(+), 374 deletions(-)

I was a little surprised we ended up with fewer lines, but I guess most
of that is not in the callers themselves, but deleting the function and
flag definitions.

Most call sites seem to be about as long. IMHO the result is quite
readable, though of course any big change like this carries risk of
regression.

> diff --git a/add-interactive.c b/add-interactive.c
> index f071b2a1b4..ecc5ae1b24 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -997,18 +997,17 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
>  	count = list_and_choose(s, files, opts);
>  	opts->flags = 0;
>  	if (count > 0) {
> -		struct strvec args = STRVEC_INIT;
> +		struct child_process cmd = CHILD_PROCESS_INIT;
> 
> -		strvec_pushl(&args, "git", "diff", "-p", "--cached",
> +		strvec_pushl(&cmd.args, "git", "diff", "-p", "--cached",
>  			     oid_to_hex(!is_initial ? &oid :
>  					s->r->hash_algo->empty_tree),
>  			     "--", NULL);
>  		for (i = 0; i < files->items.nr; i++)
>  			if (files->selected[i])
> -				strvec_push(&args,
> +				strvec_push(&cmd.args,
>  					    files->items.items[i].string);
> -		res = run_command_v_opt(args.v, 0);
> -		strvec_clear(&args);
> +		res = run_command(&cmd);
>  	}

So ones like this are just swapping "args" for "cmd.args". It's nice
that we don't have to remember to free (and in some cases, that lets us
return directly rather than stashing the result of run_command(),
cleaning up, and then returning it).

> -static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
> -
>  static const char *term_bad;
>  static const char *term_good;
> 
> @@ -729,20 +727,22 @@ static int is_expected_rev(const struct object_id *oid)
>  enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
>  				  int no_checkout)
>  {
> -	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
>  	struct commit *commit;
>  	struct pretty_print_context pp = {0};
>  	struct strbuf commit_msg = STRBUF_INIT;
> 
> -	oid_to_hex_r(bisect_rev_hex, bisect_rev);
>  	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
> 
> -	argv_checkout[2] = bisect_rev_hex;
>  	if (no_checkout) {
>  		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
>  			   UPDATE_REFS_DIE_ON_ERR);
>  	} else {
> -		if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
> +		struct child_process cmd = CHILD_PROCESS_INIT;
> +
> +		cmd.git_cmd = 1;
> +		strvec_pushl(&cmd.args, "checkout", "-q",
> +			     oid_to_hex(bisect_rev), "--", NULL);
> +		if (run_command(&cmd))

This one is a little longer, but IMHO worth it regardless to avoid the
magic "2" that must match the NULL stuffed into argv_checkout.

That is mostly coming from using a strvec at all, though. It _could_ use
one and then run_command_v_opt() or whatever, though I like the full
conversion.

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 5900b81729..5e14decc22 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -347,58 +347,52 @@ static int save_state(struct object_id *stash)
> 
>  static void read_empty(const struct object_id *oid, int verbose)
>  {
> -	int i = 0;
> -	const char *args[7];
> +	struct child_process cmd = CHILD_PROCESS_INIT;
> 
> -	args[i++] = "read-tree";
> +	strvec_push(&cmd.args, "read-tree");
>  	if (verbose)
> -		args[i++] = "-v";
> -	args[i++] = "-m";
> -	args[i++] = "-u";
> -	args[i++] = empty_tree_oid_hex();
> -	args[i++] = oid_to_hex(oid);
> -	args[i] = NULL;
> +		strvec_push(&cmd.args, "-v");
> +	strvec_pushl(&cmd.args, "-m", "-u", empty_tree_oid_hex(),
> +		     oid_to_hex(oid), NULL);
> 
> -	if (run_command_v_opt(args, RUN_GIT_CMD))
> +	cmd.git_cmd = 1;
> +	if (run_command(&cmd))
>  		die(_("read-tree failed"));
>  }

Likewise this one, which is even worse, as that "7" must match the
number of "i++" lines. So regardless of any run_command_v() conversion,
I am happy to see this move to a strvec (and again, IMHO you might as
well go straight to using cmd.args rather than a separate strvec).

-Peff