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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 11, 2022, 17:11 UTC
Message-ID: <xmqqpmeyuvxt.fsf@gitster.g>
In-Reply-To: <Y0VtkmNwjKcXcemP@coredump.intra.peff.net>

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"));
 }