On Tue, Nov 15 2022, Derrick Stolee wrote:
> 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.
Yeah, the config API's full of foot-guns, although here we return a
"const struct string_list *", not a "struct string_list *", so in
retrospect this should be rather obvious...
But still, we should probably as #leftoverbits make it behave
consistently wrt naming. I.e. in this case it's
git_configset_get_value_multi() really behaves like a
git_configset_get_string_tmp(), and there's no equivalent of a
git_configset_get_string() (i.e. xstrdup()'d) for *_multi().
>> + 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.
Yes, and it's probably worth keeping that, but I haven't benchmarked
etc. This is only a problem in practice if you're constructing your own
configset, e.g. here because we have a custom config file. So for most
users this API is safe in general, i.e. we free() it, but it's the
config that's in "the_repository" normally, so it outlives any "normal"
code.
>> } 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.
On the related subject of config API foot-guns, it would be great if you
could look over the in-flight series I have to make related parts of the
config API safe by default [1].
8/9 there fixes 6 segfaults, 3 of which are git blame'd to you :), and
9/9 a foot-gun-y interaction with the strvec API, which you'll also
probably find interesting...
1. https://lore.kernel.org/git/cover-v2-0.9-00000000000-20221101T225822Z-avarab@xxxxxxxxx/
