Skip to content

Commit 03744bb

Browse files
committed
builtin/gc.c: fix use-after-free in maintenance_unregister()
While trying to fix a move based on an uninitialized value (along with a declaration after the first statement), be0fd57 (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: $ ./t7900-maintenance.sh -vxi --run=23 --valgrind [...] + git maintenance unregister --config-file ./other ==3048727== Invalid read of size 8 ==3048727== at 0x1869CA: maintenance_unregister (gc.c:1590) ==3048727== by 0x188F42: cmd_maintenance (gc.c:2651) ==3048727== by 0x128C62: run_builtin (git.c:466) ==3048727== by 0x12907E: handle_builtin (git.c:721) ==3048727== by 0x1292EC: run_argv (git.c:788) ==3048727== by 0x12988E: cmd_main (git.c:926) ==3048727== by 0x21ED39: main (common-main.c:57) ==3048727== Address 0x4b38bc8 is 24 bytes inside a block of size 64 free'd ==3048727== at 0x484617B: free (vg_replace_malloc.c:872) ==3048727== by 0x2D207E: free_individual_entries (hashmap.c:188) ==3048727== by 0x2D2153: hashmap_clear_ (hashmap.c:207) ==3048727== by 0x270B5C: git_configset_clear (config.c:2375) ==3048727== by 0x1869AC: maintenance_unregister (gc.c:1585) ==3048727== by 0x188F42: cmd_maintenance (gc.c:2651) ==3048727== by 0x128C62: run_builtin (git.c:466) ==3048727== by 0x12907E: handle_builtin (git.c:721) ==3048727== by 0x1292EC: run_argv (git.c:788) ==3048727== by 0x12988E: cmd_main (git.c:926) ==3048727== by 0x21ED39: main (common-main.c:57) [...] Resolve this via a partial-revert of be0fd57. The config_set struct now gets a zero initialization, which makes free()-ing it a noop even without calling git_configset_init(). When we do initialize it to a non-zero value, it is only free()'d after our last read of `list`. Signed-off-by: Taylor Blau <[email protected]>
1 parent be0fd57 commit 03744bb

File tree

1 file changed

+2
-3
lines changed

1 file changed

+2
-3
lines changed

builtin/gc.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1543,6 +1543,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
15431543
int found = 0;
15441544
struct string_list_item *item;
15451545
const struct string_list *list;
1546+
struct config_set cs = { { 0 } };
15461547

15471548
argc = parse_options(argc, argv, prefix, options,
15481549
builtin_maintenance_unregister_usage, 0);
@@ -1551,12 +1552,9 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
15511552
options);
15521553

15531554
if (config_file) {
1554-
struct config_set cs;
1555-
15561555
git_configset_init(&cs);
15571556
git_configset_add_file(&cs, config_file);
15581557
list = git_configset_get_value_multi(&cs, key);
1559-
git_configset_clear(&cs);
15601558
} else {
15611559
list = git_config_get_value_multi(key);
15621560
}
@@ -1592,6 +1590,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
15921590
die(_("repository '%s' is not registered"), maintpath);
15931591
}
15941592

1593+
git_configset_clear(&cs);
15951594
free(maintpath);
15961595
return 0;
15971596
}

0 commit comments

Comments
 (0)