git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Oct 11, 2022, 18:13 UTC
Message-ID
<221011.86czayns5x.gmgdl@evledraar.gmail.com>
In-Reply-To
<xmqqpmeyuvxt.fsf@gitster.g>
On Tue, Oct 11 2022, Junio C Hamano wrote:
Show 83 quoted lines
> 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;
Previous: Junio C HamanoNext: Junio C Hamano
Message 15 of 21 in “PATCH] bisect--helper: plug strvec leak in bisect_start()”
  1. René ScharfeOct 4, 2022
  2. Ævar Arnfjörð BjarmasonOct 5, 2022
  3. René ScharfeOct 5, 2022
  4. Junio C HamanoOct 5, 2022
  5. Ævar Arnfjörð BjarmasonOct 6, 2022
  6. Junio C HamanoOct 6, 2022
  7. bisect--helper: plug strvec leakRené Scharfe, Oct 7, 2022
  8. Junio C HamanoOct 7, 2022
  9. Jeff KingOct 11, 2022
  10. Junio C HamanoOct 11, 2022
  11. Ævar Arnfjörð BjarmasonOct 11, 2022
  12. Jeff KingOct 11, 2022
  13. Jeff KingOct 11, 2022
  14. Junio C HamanoOct 11, 2022
  15. Ævar Arnfjörð BjarmasonOct 11, 2022
  16. Junio C HamanoOct 11, 2022
  17. Jeff KingOct 14, 2022
  18. Junio C HamanoOct 14, 2022
  19. René ScharfeOct 15, 2022
  20. Jeff KingOct 15, 2022
  21. Junio C HamanoOct 5, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.