From: Jeff King <peff@peff.net>
To: "René Scharfe" <l.s.r@web.de>
Cc: "Junio C Hamano" <gitster@pobox.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Git List" <git@vger.kernel.org>
Subject: Re: [PATCH v2] bisect--helper: plug strvec leak
Date: Sat, 15 Oct 2022 14:21:33 -0400 [thread overview]
Message-ID: <Y0r6LaxDLjpiRNqS@coredump.intra.peff.net> (raw)
In-Reply-To: <9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de>
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
next prev parent reply other threads:[~2022-10-15 18:21 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-04 16:06 PATCH] bisect--helper: plug strvec leak in bisect_start() René Scharfe
2022-10-05 7:29 ` Ævar Arnfjörð Bjarmason
2022-10-05 15:43 ` René Scharfe
2022-10-05 19:44 ` Junio C Hamano
2022-10-06 21:35 ` Ævar Arnfjörð Bjarmason
2022-10-06 21:53 ` Junio C Hamano
2022-10-07 15:08 ` [PATCH v2] bisect--helper: plug strvec leak René Scharfe
2022-10-07 17:21 ` Junio C Hamano
2022-10-11 2:39 ` Jeff King
2022-10-11 5:42 ` Junio C Hamano
2022-10-11 7:29 ` Ævar Arnfjörð Bjarmason
2022-10-11 13:21 ` Jeff King
2022-10-11 13:20 ` Jeff King
2022-10-11 17:11 ` Junio C Hamano
2022-10-11 18:13 ` Ævar Arnfjörð Bjarmason
2022-10-11 21:43 ` Junio C Hamano
2022-10-14 19:44 ` Jeff King
2022-10-14 20:23 ` Junio C Hamano
2022-10-15 6:51 ` René Scharfe
2022-10-15 18:21 ` Jeff King [this message]
2022-10-05 19:41 ` PATCH] bisect--helper: plug strvec leak in bisect_start() Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y0r6LaxDLjpiRNqS@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=l.s.r@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).