Re: [PATCH] builtin/gc.c: fix use-after-free in maintenance_unregister()
From: Derrick Stolee <hidden>
Date: 2022-11-15 19:00:59
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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
} 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