On 11/15/2022 1:53 PM, Taylor Blau wrote:
> 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.
> + 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.
> } 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
