reflog-walk: duplicate strings in complete_reflogs list

As part of the add_reflog_to_walk() function, we keep a string_list mapping refnames to their reflog contents. This serves as a cache so that accessing the same reflog twice requires only a single copy of the log in memory. The string_list is initialized via xcalloc, meaning its strdup_strings field is set to 0. But after inserting a string into the list, we unconditionally call free() on the string, leaving the list pointing to freed memory. If another reflog is added (e.g., "git log -g HEAD HEAD"), then the second one may have unpredictable results. The extra free was added by 5026b47175 (add_reflog_for_walk: avoid memory leak, 2017-05-04). Though if you look carefully, you can see that the code was buggy even before then. If we tried to read the reflogs by time but came up with no entries, we exited with an error, freeing the string in that code path. So the bug was harder to trigger, but still there. We can fix it by just asking the string list to make a copy of the string. Technically we could fix the problem by not calling free() on our string (and just handing over ownership to the string list), but there are enough conditionals that it's quite hard to figure out which code paths need the free and which do not. Simpler is better here. The new test reliably shows the problem when run with --valgrind or ASAN. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Jul 7, 2017 at 04:39 UTC 75afe7ac8728128873c001d82b02ada4a3aa8181
2 files changed +7
reflog-walk.c
+1
@@ -136,6 +136,7 @@ struct reflog_walk_info {
136 void init_reflog_walk(struct reflog_walk_info **info)
137 {
138 *info = xcalloc(1, sizeof(struct reflog_walk_info));
139 + (*info)->complete_reflogs.strdup_strings = 1;
140 }
141
142 int add_reflog_for_walk(struct reflog_walk_info *info,
t/t1411-reflog-show.sh
+6
@@ -171,4 +171,10 @@ test_expect_success 'reflog exists works' '
171 ! git reflog exists refs/heads/nonexistent
172 '
173
174 +# The behavior with two reflogs is buggy and the output is in flux; for now
175 +# we're just checking that the program works at all without segfaulting.
176 +test_expect_success 'showing multiple reflogs works' '
177 + git log -g HEAD HEAD >actual
178 +'
179 +
180 test_done