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

Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()

From
Derrick Stolee <derrickstolee@github.com>
Date
Nov 15, 2022, 19:00 UTC
Message-ID
<d8b732c9-ae41-bf1b-3bb1-6682493bee70@github.com>
In-Reply-To
<2cbead254b77cb02d219bca8f628dc4362c045b0.1668538355.git.me@ttaylorr.com>
On 11/15/2022 1:53 PM, Taylor Blau wrote:
Show 14 quoted lines
> While trying to fix a move based on an uninitialized value (along with a
> declaration after the first statement), be0fd57228
> (maintenance --unregister: fix uninit'd data use &
> -Wdeclaration-after-statement, 2022-11-15) unintentionally introduced a
> use-after-free.
> 
> The problem arises when `maintenance_unregister()` sees a non-NULL
> `config_file` string and thus tries to call
> git_configset_get_value_multi() to lookup the corresponding values.
> 
> We store the result off, and then call git_configset_clear(), which
> frees the pointer that we just stored. We then try to read that
> now-freed pointer a few lines below, and there we have our
> use-after-free:

Makes sense why this needs to be pulled out to a larger scope, but also why it's so easy to make this mistake.

Show 14 quoted lines
> +	struct config_set cs = { { 0 } };
> 
>  	argc = parse_options(argc, argv, prefix, options,
>  			     builtin_maintenance_unregister_usage, 0);
> @@ -1551,12 +1552,9 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>  				   options);
> 
>  	if (config_file) {
> -		struct config_set cs;
> -
>  		git_configset_init(&cs);
>  		git_configset_add_file(&cs, config_file);
>  		list = git_configset_get_value_multi(&cs, key);
> -		git_configset_clear(&cs);

That the list depends on the configset and not exist as an independent entity is non-obvious, but I'm sure is rooted in some kind of memory-saving optimization.

Show 11 quoted lines
>  	} else {
>  		list = git_config_get_value_multi(key);
>  	}
> @@ -1592,6 +1590,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>  		die(_("repository '%s' is not registered"), maintpath);
>  	}
> 
> +	git_configset_clear(&cs);
>  	free(maintpath);
>  	return 0;
>  }
Thanks for drilling down on this. LGTM.
-Stolee
Previous: Taylor BlauNext: Ævar Arnfjörð Bjarmason
Message 2 of 8 in “builtin/gc.c: fix use-after-free in maintenance_unregister()”
  1. builtin/gc.c: fix use-after-free in maintenance_unregister()Taylor Blau, Nov 15, 2022
  2. Derrick StoleeNov 15, 2022
  3. Ævar Arnfjörð BjarmasonNov 15, 2022
  4. Ævar Arnfjörð BjarmasonNov 15, 2022
  5. Taylor BlauNov 15, 2022
  6. Derrick StoleeNov 16, 2022
  7. Ævar Arnfjörð BjarmasonNov 16, 2022
  8. Taylor BlauNov 16, 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.