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