| Message ID | 5c6a4c30-d454-51b6-ec57-9af036b9c4e0@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PATCH] bisect--helper: plug strvec leak in bisect_start() | expand |
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).
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!
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
Æ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.
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.
Æ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.
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>'."),
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(-) -- 2.37.3