From 2272d3e5426a65f3fdf456f8e1bfbea40d037a59 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 5 Jul 2017 03:57:37 -0400 Subject: [PATCH 1/4] reflog-walk: skip over double-null oid due to HEAD rename Since 39ee4c6c2f (branch: record creation of renamed branch in HEAD's log, 2017-02-20), a rename on the currently checked out branch will create two entries in the HEAD reflog: one where the branch goes away (switching to the null oid), and one where it comes back (switching away from the null oid). This confuses the reflog-walk code. When walking backwards, it first sees the null oid in the "old" field of the second entry. Thanks to the "root commit" logic added by 71abeb753f (reflog: continue walking the reflog past root commits, 2016-06-03), we keep looking for the next entry by scanning the "new" field from the previous entry. But that field is also null! We need to go just a tiny bit further, and look at its "old" field. But with the current code, we decide the reflog has nothing else to show and just give up. To the user this looks like the reflog was truncated by the rename operation, when in fact those entries are still there. This patch does the absolute minimal fix, which is to look back that one extra level and keep traversing. The resulting behavior may not be the _best_ thing to do in the long run (for example, we show both reflog entries each with the same commit id), but it's a simple way to fix the problem without risking further regressions. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- reflog-walk.c | 2 ++ t/t3200-branch.sh | 11 +++++++++++ 2 files changed, 13 insertions(+) diff --git a/reflog-walk.c b/reflog-walk.c index c63eb1a3fd..f7ffd1caa3 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -259,6 +259,8 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit) /* a root commit, but there are still more entries to show */ reflog = &commit_reflog->reflogs->items[commit_reflog->recno]; logobj = parse_object(reflog->noid.hash); + if (!logobj) + logobj = parse_object(reflog->ooid.hash); } if (!logobj || logobj->type != OBJ_COMMIT) { diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index 48d152b9a9..dd37ac47c5 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -162,6 +162,17 @@ test_expect_success 'git branch -M baz bam should add entries to .git/logs/HEAD' grep "^0\{40\}.*$msg$" .git/logs/HEAD ' +test_expect_success 'resulting reflog can be shown by log -g' ' + oid=$(git rev-parse HEAD) && + cat >expect <<-EOF && + HEAD@{0} $oid $msg + HEAD@{1} $oid $msg + HEAD@{2} $oid checkout: moving from foo to baz + EOF + git log -g --format="%gd %H %gs" -3 HEAD >actual && + test_cmp expect actual +' + test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' ' git checkout master && git worktree add -b baz bazdir && From 75afe7ac8728128873c001d82b02ada4a3aa8181 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 7 Jul 2017 04:39:50 -0400 Subject: [PATCH 2/4] 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 Signed-off-by: Junio C Hamano --- reflog-walk.c | 1 + t/t1411-reflog-show.sh | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/reflog-walk.c b/reflog-walk.c index f7ffd1caa3..aec718beba 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -136,6 +136,7 @@ struct reflog_walk_info { void init_reflog_walk(struct reflog_walk_info **info) { *info = xcalloc(1, sizeof(struct reflog_walk_info)); + (*info)->complete_reflogs.strdup_strings = 1; } int add_reflog_for_walk(struct reflog_walk_info *info, diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh index 6ac7734d79..ae96eeb66a 100755 --- a/t/t1411-reflog-show.sh +++ b/t/t1411-reflog-show.sh @@ -171,4 +171,10 @@ test_expect_success 'reflog exists works' ' ! git reflog exists refs/heads/nonexistent ' +# The behavior with two reflogs is buggy and the output is in flux; for now +# we're just checking that the program works at all without segfaulting. +test_expect_success 'showing multiple reflogs works' ' + git log -g HEAD HEAD >actual +' + test_done From 8aae3cf7558f00cf3c05636f888cb11e5e548e51 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 7 Jul 2017 04:41:49 -0400 Subject: [PATCH 3/4] reflog-walk: don't free reflogs added to cache The add_reflog_for_walk() function keeps a cache mapping refnames to their reflog contents. We use a cached reflog entry if available, and otherwise allocate and store a new one. Since 5026b47175 (add_reflog_for_walk: avoid memory leak, 2017-05-04), when we hit an error parsing a date-based reflog spec, we free the reflog memory but leave the cache entry pointing to the now-freed memory. We can fix this by just leaving the memory intact once it has made it into the cache. This may leave an unused entry in the cache, but that's OK. And it means we also catch a similar situation: we may not have allocated at all in this invocation, but simply be pointing to a cached entry from a previous invocation (which is relying on that entry being present). The new test in t1411 exercises this case and fails when run with --valgrind or ASan. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- reflog-walk.c | 4 ---- t/t1411-reflog-show.sh | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/reflog-walk.c b/reflog-walk.c index aec718beba..2a43537326 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -215,10 +215,6 @@ int add_reflog_for_walk(struct reflog_walk_info *info, if (recno < 0) { commit_reflog->recno = get_reflog_recno_by_time(reflogs, timestamp); if (commit_reflog->recno < 0) { - if (reflogs) { - free(reflogs->ref); - free(reflogs); - } free(commit_reflog); return -1; } diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh index ae96eeb66a..b9cb76654b 100755 --- a/t/t1411-reflog-show.sh +++ b/t/t1411-reflog-show.sh @@ -177,4 +177,8 @@ test_expect_success 'showing multiple reflogs works' ' git log -g HEAD HEAD >actual ' +test_expect_success 'showing multiple reflogs with an old date' ' + git log -g HEAD@{1979-01-01} HEAD >actual +' + test_done From e30d463d450286ffafb442ecc7686df4004c06ff Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 7 Jul 2017 04:43:16 -0400 Subject: [PATCH 4/4] reflog-walk: include all fields when freeing complete_reflogs When we encounter an error adding reflogs for a walk, we try to free any logs we have read. But we didn't free all fields, meaning that we could in theory leak all of the "items" array (which would consitute the bulk of the allocated memory). This patch adds a helper which frees all of the entries and uses it as appropriate. As it turns out, the leak seems impossible to trigger with the current code. Of the three error paths that free the complete_reflogs struct, two only kick in when the items array is empty, and the third was removed entirely in the previous commit. So this patch should be a noop in terms of behavior, but it fixes a potential maintenance headache should anybody add a new error path and copy the partial-free code. Which is what happened in 5026b47175 (add_reflog_for_walk: avoid memory leak, 2017-05-04), though its leaky call was the third one that was recently removed. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- reflog-walk.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/reflog-walk.c b/reflog-walk.c index 2a43537326..ba72020fc3 100644 --- a/reflog-walk.c +++ b/reflog-walk.c @@ -38,6 +38,22 @@ static int read_one_reflog(struct object_id *ooid, struct object_id *noid, return 0; } +static void free_complete_reflog(struct complete_reflogs *array) +{ + int i; + + if (!array) + return; + + for (i = 0; i < array->nr; i++) { + free(array->items[i].email); + free(array->items[i].message); + } + free(array->items); + free(array->ref); + free(array); +} + static struct complete_reflogs *read_complete_reflog(const char *ref) { struct complete_reflogs *reflogs = @@ -189,20 +205,14 @@ int add_reflog_for_walk(struct reflog_walk_info *info, if (ret > 1) free(b); else if (ret == 1) { - if (reflogs) { - free(reflogs->ref); - free(reflogs); - } + free_complete_reflog(reflogs); free(branch); branch = b; reflogs = read_complete_reflog(branch); } } if (!reflogs || reflogs->nr == 0) { - if (reflogs) { - free(reflogs->ref); - free(reflogs); - } + free_complete_reflog(reflogs); free(branch); return -1; }