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

[Derrick Stolee <derrickstolee@github.com>]

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