From f98c2f7e53062a59f67914337c0b45c82393e11f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 26 Feb 2015 20:39:48 -0500 Subject: diffcore-rename: split locate_rename_dst into two functions This function manages the mapping of destination pathnames to filepairs, and it handles both insertion and lookup. This makes the return value a bit confusing, as we return a newly created entry (even though no caller cares), and have no room to indicate to the caller that an entry already existed. Instead, let's break this up into two distinct functions, both backed by a common binary search. The binary search will use our normal "return the index if we found something, or negative index minus one to show where it would have gone" semantics. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diffcore-rename.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) (limited to 'diffcore-rename.c') diff --git a/diffcore-rename.c b/diffcore-rename.c index 749a35d2c2..0afe903de9 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -15,8 +15,7 @@ static struct diff_rename_dst { } *rename_dst; static int rename_dst_nr, rename_dst_alloc; -static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two, - int insert_ok) +static int find_rename_dst(struct diff_filespec *two) { int first, last; @@ -27,16 +26,33 @@ static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two, struct diff_rename_dst *dst = &(rename_dst[next]); int cmp = strcmp(two->path, dst->two->path); if (!cmp) - return dst; + return next; if (cmp < 0) { last = next; continue; } first = next+1; } - /* not found */ - if (!insert_ok) - return NULL; + return -first - 1; +} + +static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two) +{ + int ofs = find_rename_dst(two); + return ofs < 0 ? NULL : &rename_dst[ofs]; +} + +/* + * Returns 0 on success, -1 if we found a duplicate. + */ +static int add_rename_dst(struct diff_filespec *two) +{ + int first = find_rename_dst(two); + + if (first >= 0) + return -1; + first = -first - 1; + /* insert to make it at "first" */ ALLOC_GROW(rename_dst, rename_dst_nr + 1, rename_dst_alloc); rename_dst_nr++; @@ -46,7 +62,7 @@ static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two, rename_dst[first].two = alloc_filespec(two->path); fill_filespec(rename_dst[first].two, two->sha1, two->sha1_valid, two->mode); rename_dst[first].pair = NULL; - return &(rename_dst[first]); + return 0; } /* Table of rename/copy src files */ @@ -452,7 +468,7 @@ void diffcore_rename(struct diff_options *options) is_empty_blob_sha1(p->two->sha1)) continue; else - locate_rename_dst(p->two, 1); + add_rename_dst(p->two); } else if (!DIFF_OPT_TST(options, RENAME_EMPTY) && is_empty_blob_sha1(p->one->sha1)) @@ -583,8 +599,7 @@ void diffcore_rename(struct diff_options *options) * We would output this create record if it has * not been turned into a rename/copy already. */ - struct diff_rename_dst *dst = - locate_rename_dst(p->two, 0); + struct diff_rename_dst *dst = locate_rename_dst(p->two); if (dst && dst->pair) { diff_q(&outq, dst->pair); pair_to_free = p; @@ -614,8 +629,7 @@ void diffcore_rename(struct diff_options *options) */ if (DIFF_PAIR_BROKEN(p)) { /* broken delete */ - struct diff_rename_dst *dst = - locate_rename_dst(p->one, 0); + struct diff_rename_dst *dst = locate_rename_dst(p->one); if (dst && dst->pair) /* counterpart is now rename/copy */ pair_to_free = p; -- cgit v1.2.3 From 4d6be03b95c3db21db1bb8fee01128c1b13f70e7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 26 Feb 2015 20:42:27 -0500 Subject: diffcore-rename: avoid processing duplicate destinations The rename code cannot handle an input where we have duplicate destinations (i.e., more than one diff_filepair in the queue with the same string in its pair->two->path). We end up allocating only one slot in the rename_dst mapping. If we fill in the diff_filepair for that slot, when we re-queue the results, we may queue that filepair multiple times. When the diff is finally flushed, the filepair is processed and free()d multiple times, leading to heap corruption. This situation should only happen when a tree diff sees duplicates in one of the trees (see the added test for a detailed example). Rather than handle it, the sanest thing is just to turn off rename detection altogether for the diff. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diffcore-rename.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'diffcore-rename.c') diff --git a/diffcore-rename.c b/diffcore-rename.c index 0afe903de9..361eed9fbc 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -467,8 +467,12 @@ void diffcore_rename(struct diff_options *options) else if (!DIFF_OPT_TST(options, RENAME_EMPTY) && is_empty_blob_sha1(p->two->sha1)) continue; - else - add_rename_dst(p->two); + else if (add_rename_dst(p->two) < 0) { + warning("skipping rename detection, detected" + " duplicate destination '%s'", + p->two->path); + goto cleanup; + } } else if (!DIFF_OPT_TST(options, RENAME_EMPTY) && is_empty_blob_sha1(p->one->sha1)) -- cgit v1.2.3