run-command API: remove "argv" member, always use "args"
Remove the "argv" member from the run-command API, ever since "args" was added in c460c0ecdca (run-command: store an optional argv_array, 2014-05-15) being able to provide either "argv" or "args" has led to some confusion and bugs. If we hadn't gone in that direction and only had an "argv" our problems wouldn't have been solved either, as noted in [1] (and in the documentation amended here) it comes with inherent memory management issues: The caller would have to hang on to the "argv" until the run-command API was finished. If the "argv" was an argument to main() this wasn't an issue, but if it it was manually constructed using the API might be painful. We also have a recent report[2] of a user of the API segfaulting, which is a direct result of it being complex to use. This commit addresses the root cause of that bug. This change is larger than I'd like, but there's no easy way to avoid it that wouldn't involve even more verbose intermediate steps. We use the "argv" as the source of truth over the "args", so we need to change all parts of run-command.[ch] itself, as well as the trace2 logging at the same time. The resulting Windows-specific code in start_command() is a bit nasty, as we're now assigning to a strvec's "v" member, instead of to our own "argv". There was a suggestion of some alternate approaches in reply to an earlier version of this commit[3], but let's leave larger a larger and needless refactoring of this code for now. 1. http://lore.kernel.org/git/YT6BnnXeAWn8BycF@coredump.intra.peff.net 2. https://lore.kernel.org/git/20211120194048.12125-1-ematsumiya@suse.de/ 3. https://lore.kernel.org/git/patch-5.5-ea1011f7473-20211122T153605Z-avarab@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Ævar Arnfjörð Bjarmason committed
Nov 25, 2021 at 23:52 UTC
d3b2159712019a06f1f495d3e42bd6aa6e76e848
6 files changed
+33
-39
run-command.c
+20
-22
@@ -380,7 +380,7 @@ static void child_err_spew(struct child_process *cmd, struct child_err *cerr)
380
switch (cerr->err) {
381
case CHILD_ERR_CHDIR:
382
error_errno("exec '%s': cd to '%s' failed",
383
- cmd->argv[0], cmd->dir);
383
+ cmd->args.v[0], cmd->dir);
384
break;
385
case CHILD_ERR_DUP2:
386
error_errno("dup2() in child failed");
@@ -392,12 +392,12 @@ static void child_err_spew(struct child_process *cmd, struct child_err *cerr)
392
error_errno("sigprocmask failed restoring signals");
393
break;
394
case CHILD_ERR_ENOENT:
395
- error_errno("cannot run %s", cmd->argv[0]);
395
+ error_errno("cannot run %s", cmd->args.v[0]);
396
break;
397
case CHILD_ERR_SILENT:
398
break;
399
case CHILD_ERR_ERRNO:
400
- error_errno("cannot exec '%s'", cmd->argv[0]);
400
+ error_errno("cannot exec '%s'", cmd->args.v[0]);
401
break;
402
}
403
set_error_routine(old_errfn);
@@ -405,7 +405,7 @@ static void child_err_spew(struct child_process *cmd, struct child_err *cerr)
405
406
static int prepare_cmd(struct strvec *out, const struct child_process *cmd)
407
{
408
- if (!cmd->argv[0])
408
+ if (!cmd->args.v[0])
409
BUG("command is empty");
410
411
/*
@@ -415,11 +415,11 @@ static int prepare_cmd(struct strvec *out, const struct child_process *cmd)
415
strvec_push(out, SHELL_PATH);
416
417
if (cmd->git_cmd) {
418
- prepare_git_cmd(out, cmd->argv);
418
+ prepare_git_cmd(out, cmd->args.v);
419
} else if (cmd->use_shell) {
420
- prepare_shell_cmd(out, cmd->argv);
420
+ prepare_shell_cmd(out, cmd->args.v);
421
} else {
422
- strvec_pushv(out, cmd->argv);
422
+ strvec_pushv(out, cmd->args.v);
423
}
424
425
/*
@@ -663,7 +663,7 @@ static void trace_run_command(const struct child_process *cp)
663
trace_add_env(&buf, cp->env);
664
if (cp->git_cmd)
665
strbuf_addstr(&buf, " git");
666
- sq_quote_argv_pretty(&buf, cp->argv);
666
+ sq_quote_argv_pretty(&buf, cp->args.v);
667
668
trace_printf("%s", buf.buf);
669
strbuf_release(&buf);
@@ -676,8 +676,6 @@ int start_command(struct child_process *cmd)
676
int failed_errno;
677
char *str;
678
679
- if (!cmd->argv)
680
- cmd->argv = cmd->args.v;
679
if (!cmd->env)
680
cmd->env = cmd->env_array.v;
681
@@ -729,7 +727,7 @@ int start_command(struct child_process *cmd)
727
str = "standard error";
728
fail_pipe:
729
error("cannot create %s pipe for %s: %s",
732
- str, cmd->argv[0], strerror(failed_errno));
730
+ str, cmd->args.v[0], strerror(failed_errno));
731
child_process_clear(cmd);
732
errno = failed_errno;
733
return -1;
@@ -758,7 +756,7 @@ fail_pipe:
756
failed_errno = errno;
757
cmd->pid = -1;
758
if (!cmd->silent_exec_failure)
761
- error_errno("cannot run %s", cmd->argv[0]);
759
+ error_errno("cannot run %s", cmd->args.v[0]);
760
goto end_of_spawn;
761
}
762
@@ -868,7 +866,7 @@ fail_pipe:
866
}
867
atfork_parent(&as);
868
if (cmd->pid < 0)
871
- error_errno("cannot fork() for %s", cmd->argv[0]);
869
+ error_errno("cannot fork() for %s", cmd->args.v[0]);
870
else if (cmd->clean_on_exit)
871
mark_child_for_cleanup(cmd->pid, cmd);
872
@@ -885,7 +883,7 @@ fail_pipe:
883
* At this point we know that fork() succeeded, but exec()
884
* failed. Errors have been reported to our stderr.
885
*/
888
- wait_or_whine(cmd->pid, cmd->argv[0], 0);
886
+ wait_or_whine(cmd->pid, cmd->args.v[0], 0);
887
child_err_spew(cmd, &cerr);
888
failed_errno = errno;
889
cmd->pid = -1;
@@ -902,7 +900,7 @@ end_of_spawn:
900
#else
901
{
902
int fhin = 0, fhout = 1, fherr = 2;
905
- const char **sargv = cmd->argv;
903
+ const char **sargv = cmd->args.v;
904
struct strvec nargv = STRVEC_INIT;
905
906
if (cmd->no_stdin)
@@ -929,20 +927,20 @@ end_of_spawn:
927
fhout = dup(cmd->out);
928
929
if (cmd->git_cmd)
932
- cmd->argv = prepare_git_cmd(&nargv, cmd->argv);
930
+ cmd->args.v = prepare_git_cmd(&nargv, sargv);
931
else if (cmd->use_shell)
934
- cmd->argv = prepare_shell_cmd(&nargv, cmd->argv);
932
+ cmd->args.v = prepare_shell_cmd(&nargv, sargv);
933
936
- cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, (char**) cmd->env,
934
+ cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v, (char**) cmd->env,
935
cmd->dir, fhin, fhout, fherr);
936
failed_errno = errno;
937
if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
940
- error_errno("cannot spawn %s", cmd->argv[0]);
938
+ error_errno("cannot spawn %s", cmd->args.v[0]);
939
if (cmd->clean_on_exit && cmd->pid >= 0)
940
mark_child_for_cleanup(cmd->pid, cmd);
941
942
strvec_clear(&nargv);
945
- cmd->argv = sargv;
943
+ cmd->args.v = sargv;
944
if (fhin != 0)
945
close(fhin);
946
if (fhout != 1)
@@ -992,7 +990,7 @@ end_of_spawn:
990
991
int finish_command(struct child_process *cmd)
992
{
995
- int ret = wait_or_whine(cmd->pid, cmd->argv[0], 0);
993
+ int ret = wait_or_whine(cmd->pid, cmd->args.v[0], 0);
994
trace2_child_exit(cmd, ret);
995
child_process_clear(cmd);
996
invalidate_lstat_cache();
@@ -1001,7 +999,7 @@ int finish_command(struct child_process *cmd)
999
1000
int finish_command_in_signal(struct child_process *cmd)
1001
{
1004
- int ret = wait_or_whine(cmd->pid, cmd->argv[0], 1);
1002
+ int ret = wait_or_whine(cmd->pid, cmd->args.v[0], 1);
1003
trace2_child_exit(cmd, ret);
1004
return ret;
1005
}
run-command.h
+8
-12
@@ -44,21 +44,17 @@
44
struct child_process {
45
46
/**
47
- * The .argv member is set up as an array of string pointers (NULL
48
- * terminated), of which .argv[0] is the program name to run (usually
49
- * without a path). If the command to run is a git command, set argv[0] to
50
- * the command name without the 'git-' prefix and set .git_cmd = 1.
47
+ * The .args is a `struct strvec', use that API to manipulate
48
+ * it, e.g. strvec_pushv() to add an existing "const char **"
49
+ * vector.
50
*
52
- * Note that the ownership of the memory pointed to by .argv stays with the
53
- * caller, but it should survive until `finish_command` completes. If the
54
- * .argv member is NULL, `start_command` will point it at the .args
55
- * `strvec` (so you may use one or the other, but you must use exactly
56
- * one). The memory in .args will be cleaned up automatically during
57
- * `finish_command` (or during `start_command` when it is unsuccessful).
51
+ * If the command to run is a git command, set the first
52
+ * element in the strvec to the command name without the
53
+ * 'git-' prefix and set .git_cmd = 1.
54
*
55
+ * The memory in .args will be cleaned up automatically during
56
+ * `finish_command` (or during `start_command` when it is unsuccessful).
57
*/
60
- const char **argv;
61
-
58
struct strvec args;
59
struct strvec env_array;
60
pid_t pid;
sub-process.c
+1
-1
@@ -187,7 +187,7 @@ static int handshake_capabilities(struct child_process *process,
187
*supported_capabilities |= capabilities[i].flag;
188
} else {
189
die("subprocess '%s' requested unsupported capability '%s'",
190
- process->argv[0], p);
190
+ process->args.v[0], p);
191
}
192
}
193
trace2/tr2_tgt_event.c
+1
-1
@@ -354,7 +354,7 @@ static void fn_child_start_fl(const char *file, int line,
354
jw_object_inline_begin_array(&jw, "argv");
355
if (cmd->git_cmd)
356
jw_array_string(&jw, "git");
357
- jw_array_argv(&jw, cmd->argv);
357
+ jw_array_argv(&jw, cmd->args.v);
358
jw_end(&jw);
359
jw_end(&jw);
360
trace2/tr2_tgt_normal.c
+1
-1
@@ -232,7 +232,7 @@ static void fn_child_start_fl(const char *file, int line,
232
strbuf_addch(&buf_payload, ' ');
233
if (cmd->git_cmd)
234
strbuf_addstr(&buf_payload, "git ");
235
- sq_append_quote_argv_pretty(&buf_payload, cmd->argv);
235
+ sq_append_quote_argv_pretty(&buf_payload, cmd->args.v);
236
237
normal_io_write_fl(file, line, &buf_payload);
238
strbuf_release(&buf_payload);
trace2/tr2_tgt_perf.c
+2
-2
@@ -335,10 +335,10 @@ static void fn_child_start_fl(const char *file, int line,
335
strbuf_addstr(&buf_payload, " argv:[");
336
if (cmd->git_cmd) {
337
strbuf_addstr(&buf_payload, "git");
338
- if (cmd->argv[0])
338
+ if (cmd->args.nr)
339
strbuf_addch(&buf_payload, ' ');
340
}
341
- sq_append_quote_argv_pretty(&buf_payload, cmd->argv);
341
+ sq_append_quote_argv_pretty(&buf_payload, cmd->args.v);
342
strbuf_addch(&buf_payload, ']');
343
344
perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,