{"thread":{"id":"63077","subject":"[PATCH] diffcore-rename: fix BUG when break detection and --follow used together","startedAt":"2025-03-08T01:00:18Z","lastAt":"2025-03-17T17:52:24Z","messageCount":6,"participants":["Elijah Newren via GitGitGadget","Jeff King","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"513824","messageId":"pull.1876.git.1741395615315.gitgitgadget@gmail.com","threadId":"63077","inReplyTo":null,"subject":"[PATCH] diffcore-rename: fix BUG when break detection and --follow used together","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-08T01:00:15Z","receivedAt":"2025-03-08T01:00:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nPrior to commit 9db2ac56168e (diffcore-rename: accelerate rename_dst\nsetup, 2020-12-11), the function add_rename_dst() resulted in quadratic\nruntime since each call inserted the new entry into the array in sorted\norder.  The reason for the sorted order requirement was so that\nlocate_rename_dst(), used when break detection is turned on, could find\nthe appropriate entry in logarithmic time via bisection on string\ncomparisons.  (It's better to be quadratic in moving pointers than\nquadratic in string comparisons, so this made some sense.)  However,\nsince break detection always sticks the broken pairs adjacent to each\nother, that commit decided to simply append entries to rename_dst, and\nrecord the mapping of (filename) -> (index within rename_dst) via a\nstrintmap.  Doing this relied on the fact that when adding the source of\na broken pair via register_rename_src(), that the next item we'd process\nwas the other half of the same broken pair and would be added to\nrename_dst via add_rename_dst().  This assumption was fine under break\ndetection alone, but the combination of break detection and\nsingle_follow violated that assumption because of this code:\n\n\t\telse if (options->single_follow &&\n\t\t\t strcmp(options->single_follow, p->two->path))\n\t\t\tcontinue; /* not interested */\n\nwhich would end up skipping calling add_rename_dst() below that point.\nSince I knew I was assuming that the dst pair of a break would always be\nadded right after the src pair of a break, I added a new BUG() directive\nas part of that commit later on at time of use that would check my\nassumptions held.  That BUG() didn't trip for nearly 4 years...which\nsadly meant I had long since forgotten the related details.  Anyway...\n\nWhen the dst half of a broken pair is skipped like this, it means that\nnot only could my recorded index be invalid (just past the end of the\narray), it could also point to some unrelated dst that just happened to\nbe the next one added to the array.  So, to fix this, we need to add a\nlittle more safety around the checks for the recorded break_idx.\n\nIt turns out that making a testcase to trigger this is a bit challenging\ntoo.  I added a simple testcase which tickles the necessary area, but\nrunning it normally actually passes for me.  However, running it under\nvalgrind shows that it is depending upon uninitialized memory.  I\nsuspect that to get a reliable reproduction case, I might need to have\nseveral more paths involved, but that might make the testcase more\ndifficult to understand.  So, I instead just embedded a warning within\nthe testname that the test triggered uninitialized memory use.\n\nIn short, when these two rare options are used together, fix the\naccidental find of the wrong dst entry (which would often be\nuninitialized memory just past the end of the array), by adding a little\nmore care around the recorded indices for break_idx.\n\nReported-by: Olaf Hering <olaf@aepfle.de>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    diffcore-rename: fix BUG when break detection and --follow used together\n    \n    Bug dates back to Git v2.31.0, and was discovered and reported about\n    four years later over at\n    https://lore.kernel.org/git/20240920112228.3d1130f5.olaf@aepfle.de/.\n    Sadly, took me about half a year to get back to this one...\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1876%2Fnewren%2Ffix-break-follow-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1876/newren/fix-break-follow-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1876\n\n diffcore-rename.c                   |  9 +++++----\n t/t4206-log-follow-harder-copies.sh | 18 ++++++++++++++++++\n 2 files changed, 23 insertions(+), 4 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 10bb0321b10..cb4be5be63c 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -33,7 +33,7 @@ static struct diff_rename_dst *locate_rename_dst(struct diff_filepair *p)\n {\n \t/* Lookup by p->ONE->path */\n \tint idx = break_idx ? strintmap_get(break_idx, p->one->path) : -1;\n-\treturn (idx == -1) ? NULL : &rename_dst[idx];\n+\treturn (idx == -1 || idx == rename_dst_nr) ? NULL : &rename_dst[idx];\n }\n \n /*\n@@ -1668,9 +1668,10 @@ void diffcore_rename_extended(struct diff_options *options,\n \t\t\tif (DIFF_PAIR_BROKEN(p)) {\n \t\t\t\t/* broken delete */\n \t\t\t\tstruct diff_rename_dst *dst = locate_rename_dst(p);\n-\t\t\t\tif (!dst)\n-\t\t\t\t\tBUG(\"tracking failed somehow; failed to find associated dst for broken pair\");\n-\t\t\t\tif (dst->is_rename)\n+\t\t\t\tif (options->single_follow && dst &&\n+\t\t\t\t    strcmp(dst->p->two->path, p->two->path))\n+\t\t\t\t\tdst = NULL;\n+\t\t\t\tif (dst && dst->is_rename)\n \t\t\t\t\t/* counterpart is now rename/copy */\n \t\t\t\t\tpair_to_free = p;\n \t\t\t}\ndiff --git a/t/t4206-log-follow-harder-copies.sh b/t/t4206-log-follow-harder-copies.sh\nindex bcab71c8e84..9e00f33a425 100755\n--- a/t/t4206-log-follow-harder-copies.sh\n+++ b/t/t4206-log-follow-harder-copies.sh\n@@ -54,4 +54,22 @@ test_expect_success 'validate the output.' '\n \tcompare_diff_patch current expected\n '\n \n+test_expect_success 'log --follow -B does not die or use uninitialized memory' '\n+\tgit switch --orphan break_and_follow_are_icky_so_use_both &&\n+\tprintf \"%s\\n\" A B C D E F G H I J K L M N O P Q R S T U V W X Y Z >z &&\n+\tgit add z &&\n+\tgit commit -m \"Initial\" &&\n+\n+\ttest_seq 1 130 >z &&\n+\techo lame >somefile &&\n+\tgit add z somefile &&\n+\tgit commit -m \"Rewrite z, introduce lame somefile\" &&\n+\n+\techo Content >somefile &&\n+\tgit add somefile &&\n+\tgit commit -m \"Rewrite somefile\" &&\n+\n+\tgit log -B --follow somefile\n+'\n+\n test_done\n\nbase-commit: f93ff170b93a1782659637824b25923245ac9dd1\n-- \ngitgitgadget\n"},{"id":"514276","messageId":"20250314172437.GA16311@coredump.intra.peff.net","threadId":"63077","inReplyTo":"pull.1876.git.1741395615315.gitgitgadget@gmail.com","subject":"Re: [PATCH] diffcore-rename: fix BUG when break detection and --follow used together","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-14T17:24:37Z","receivedAt":"2025-03-14T17:24:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 08, 2025 at 01:00:15AM +0000, Elijah Newren via GitGitGadget wrote:\n\n> It turns out that making a testcase to trigger this is a bit challenging\n> too.  I added a simple testcase which tickles the necessary area, but\n> running it normally actually passes for me.  However, running it under\n> valgrind shows that it is depending upon uninitialized memory.  I\n> suspect that to get a reliable reproduction case, I might need to have\n> several more paths involved, but that might make the testcase more\n> difficult to understand.  So, I instead just embedded a warning within\n> the testname that the test triggered uninitialized memory use.\n\nI think it's OK for a test case to require extra memory checks to fail;\nafter all, these kinds of bugs are usually non-deterministic without\nthose checks anyway.\n\nI did verify that it reproduces for me with \"--valgrind\". I was\nsurprised (and a little disappointed) that it doesn't seem to trigger\nwith ASan/UBSan. We do run those routinely in CI, but I doubt that\n--valgrind gets used regularly for the whole test suite by anyone these\ndays, just because it's so much slower.\n\nI'm puzzled, though, why the test case at the beginning of this\nthread[1] yields the BUG() so readily, but your test case doesn't.\n\nSo maybe this is the best we can do, but it feels like we should be able\nto at least trigger the existing BUG() reliably. I couldn't seem to\nfigure it out, though. :(\n\n> In short, when these two rare options are used together, fix the\n> accidental find of the wrong dst entry (which would often be\n> uninitialized memory just past the end of the array), by adding a little\n> more care around the recorded indices for break_idx.\n\nYour description of the problem and the solution both seemed sensible to\nme (though I'm not all that familiar with the ins and outs of the rename\ncode these days).\n\n-Peff\n\n[1] The simplest I came up with is:\n\n      git clone --bare https://github.com/intel/linux-sgx.git tmp.git\n      cd tmp.git\n      git --no-pager log -B --follow 63d0e65cfa49bb46a8dbe8745bb15aaf226faa97 -- external/ippcp_internal/inc\n\n    Curiously, that pathspec is actually a directory, but it only has a\n    single file in it. Feeding the actual file in it _doesn't_ trigger\n    the bug:\n\n      git --no-pager log -B --follow 63d0e65cfa49bb46a8dbe8745bb15aaf226faa97 -- external/ippcp_internal/inc/ippcp20u3.patch\n\n    That just shows the single commit (even though there is a single\n    file before and after that commit, it is not similar enough to find\n    a rename).\n\n    The file that hits the break detection is unrelated. It looks like\n    it's psw/ae/data/prebuilt/le_prod_css.bin.\n\n    I tried variants with break files, subdirs, etc, but I couldn't seem\n    to make anything work.\n"},{"id":"514326","messageId":"CABPp-BF5R_ob5uAS=m9Vt775-Er2fFqpH7jBjm_=MRmnRVbeew@mail.gmail.com","threadId":"63077","inReplyTo":"20250314172437.GA16311@coredump.intra.peff.net","subject":"Re: [PATCH] diffcore-rename: fix BUG when break detection and --follow used together","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-14T22:28:09Z","receivedAt":"2025-03-14T22:28:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Mar 14, 2025 at 10:24 AM Jeff King <peff@peff.net> wrote:\n>\n> On Sat, Mar 08, 2025 at 01:00:15AM +0000, Elijah Newren via GitGitGadget wrote:\n>\n> > It turns out that making a testcase to trigger this is a bit challenging\n> > too.  I added a simple testcase which tickles the necessary area, but\n> > running it normally actually passes for me.  However, running it under\n> > valgrind shows that it is depending upon uninitialized memory.  I\n> > suspect that to get a reliable reproduction case, I might need to have\n> > several more paths involved, but that might make the testcase more\n> > difficult to understand.  So, I instead just embedded a warning within\n> > the testname that the test triggered uninitialized memory use.\n\nMaybe I should have been a little more clear about \"a bit\nchallenging\".  I spent hours on it.  And I suspected that the only way\nto trigger the reporter's particular manifestation of the issue was to\nuse an invalid command.  I didn't want to spend any more hours on it,\nbut...\n\n> I think it's OK for a test case to require extra memory checks to fail;\n> after all, these kinds of bugs are usually non-deterministic without\n> those checks anyway.\n>\n> I did verify that it reproduces for me with \"--valgrind\". I was\n> surprised (and a little disappointed) that it doesn't seem to trigger\n> with ASan/UBSan. We do run those routinely in CI, but I doubt that\n> --valgrind gets used regularly for the whole test suite by anyone these\n> days, just because it's so much slower.\n>\n> I'm puzzled, though, why the test case at the beginning of this\n> thread[1] yields the BUG() so readily, but your test case doesn't.\n>\n> So maybe this is the best we can do, but it feels like we should be able\n> to at least trigger the existing BUG() reliably. I couldn't seem to\n> figure it out, though. :(\n\nSo, after _another_ 7 hours or so on it today...  The BUG() the\nreporter triggered only happens when there is no uninitialized memory\nuse, and is only triggerable when you use invalid flags.  For the\nreporter, they passed a directory name for their pathspec along with\n--follow, despite the fact that --follow only works when given a\nsingle pathspec that names an individual file.  You can also trigger\ntheir particular manifestation of the issue here when using a glob\npathspec together with --follow.  In either event, the --follow\nbecomes useless: when the follow logic checks whether filenames are\nequal to the given pathspec to see if it might be a relevant rename,\nno filename is exactly equal to the pathspec, so it never finds any\nrelevant files to follow or to include in the rename detection.  The\nupshot is the command basically behaves the same as if you hadn't\ngiven --follow, other than the fact that the presence of --follow\nmakes diffcore_rename throw away rename_dst pairs in a way that\nhappens to trigger this particular BUG().\n\nThe testcase I found is:\n    seq 1 127 >numbers &&\n    git add numbers &&\n    git commit -m \"numbers\" &&\n\n    printf \"%s\\n\" A B C D E F G H I J K L M N O Q R S T U V W X Y Z >pool &&\n    seq 1 10 >numbers &&\n    git add pool numbers &&\n    git commit -m \"pool\" &&\n\n    git log -1 -B --raw --follow -- \"p*\"\n\nThe BUG will still be triggered if\n  * you change the content of pool (you can even make it as similar as\nyou want to numbers)\n  * you change the content of numbers in the second commit while\nkeeping it sufficiently different from the first commit\nThe BUG will _not_ be triggered if:\n  * you change the log's pathspec to match numbers\n  * you change the log's pathspec to not match pool\n  * you change the log's pathspec to be exactly \"pool\"\n  * you remove any of the three files (two versions of number and one\nof pool) from the testcase\n  * you reduce 127 (even if only to 126)\n  * you make numbers from the second commit too similar to numbers\nfrom the first commit\n\nIn all of these cases (and this is also true for the reporter's\noriginal case), in locate_rename_dst(), idx will be computed as 0, but\nrename_dst is NULL, so &rename_dst[idx] is NULL as well.\n\nHowever, I think the fact that rename_dst == NULL implies\n&rename_dst[0] == NULL should raise alarm bells about the risks of\nusing memory improperly, even if it doesn't directly use uninitialized\nmemory in this case.  Which brings us to the bigger more encompassing\nissue, which is what I reported in the commit message:\n\n> > In short, when these two rare options are used together, fix the\n> > accidental find of the wrong dst entry (which would often be\n> > uninitialized memory just past the end of the array), by adding a little\n> > more care around the recorded indices for break_idx.\n\nIt's just the special case when rename_dst is empty and, in fact,\nNULL, that you trigger the BUG() call.\n\n> Your description of the problem and the solution both seemed sensible to\n> me (though I'm not all that familiar with the ins and outs of the rename\n> code these days).\n\nThanks.\n\n>\n> -Peff\n>\n> [1] The simplest I came up with is:\n>\n>       git clone --bare https://github.com/intel/linux-sgx.git tmp.git\n>       cd tmp.git\n>       git --no-pager log -B --follow 63d0e65cfa49bb46a8dbe8745bb15aaf226faa97 -- external/ippcp_internal/inc\n\nYeah, that was the same I was using last week.\n\n>     Curiously, that pathspec is actually a directory, but it only has a\n>     single file in it. Feeding the actual file in it _doesn't_ trigger\n>     the bug:\n>\n>       git --no-pager log -B --follow 63d0e65cfa49bb46a8dbe8745bb15aaf226faa97 -- external/ippcp_internal/inc/ippcp20u3.patch\n\nYeah, I was doing the same thing last week when fixing this bug.\nSorry, I guess I should have mentioned this to avoid you attempting\nthe same.\n\n>     That just shows the single commit (even though there is a single\n>     file before and after that commit, it is not similar enough to find\n>     a rename).\n>\n>     The file that hits the break detection is unrelated. It looks like\n>     it's psw/ae/data/prebuilt/le_prod_css.bin.\n>\n>     I tried variants with break files, subdirs, etc, but I couldn't seem\n>     to make anything work.\n\nMaybe what I wrote above helps.  Is this enough information to satisfy\nyour curiosity?\n\nI suspect adding this second test to the commit makes sense.  Which\nparts of my explanation in this email would you like to see added to\nthe commit message as well, or is it fine as-is?\n"},{"id":"514334","messageId":"pull.1876.v2.git.1742000894041.gitgitgadget@gmail.com","threadId":"63077","inReplyTo":"pull.1876.git.1741395615315.gitgitgadget@gmail.com","subject":"[PATCH v2] diffcore-rename: fix BUG when break detection and --follow used together","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-15T01:08:13Z","receivedAt":"2025-03-15T01:08:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren <newren@gmail.com>\n\nPrior to commit 9db2ac56168e (diffcore-rename: accelerate rename_dst\nsetup, 2020-12-11), the function add_rename_dst() resulted in quadratic\nruntime since each call inserted the new entry into the array in sorted\norder.  The reason for the sorted order requirement was so that\nlocate_rename_dst(), used when break detection is turned on, could find\nthe appropriate entry in logarithmic time via bisection on string\ncomparisons.  (It's better to be quadratic in moving pointers than\nquadratic in string comparisons, so this made some sense.)  However,\nsince break detection always sticks the broken pairs adjacent to each\nother, that commit decided to simply append entries to rename_dst, and\nrecord the mapping of (filename) -> (index within rename_dst) via a\nstrintmap.  Doing this relied on the fact that when adding the source of\na broken pair via register_rename_src(), that the next item we'd process\nwas the other half of the same broken pair and would be added to\nrename_dst via add_rename_dst().  This assumption was fine under break\ndetection alone, but the combination of break detection and\nsingle_follow violated that assumption because of this code:\n\n\t\telse if (options->single_follow &&\n\t\t\t strcmp(options->single_follow, p->two->path))\n\t\t\tcontinue; /* not interested */\n\nwhich would end up skipping calling add_rename_dst() below that point.\nSince I knew I was assuming that the dst pair of a break would always be\nadded right after the src pair of a break, I added a new BUG() directive\nas part of that commit later on at time of use that would check my\nassumptions held.  That BUG() didn't trip for nearly 4 years...which\nsadly meant I had long since forgotten the related details.  Anyway...\n\nWhen the dst half of a broken pair is skipped like this, it means that\nnot only could my recorded index be invalid (just past the end of the\narray), it could also point to some unrelated dst that just happened to\nbe the next one added to the array.  So, to fix this, we need to add a\nlittle more safety around the checks for the recorded break_idx.\n\nIt turns out that making a testcase to trigger this is quite the\nchallenge.  I actually added two testscases:\n  * One testcase which uses --follow incorrectly (it uses its single\n    pathspec to specifying something other than a single filename), and\n    which triggers the same bug reported-by Olaf.  This triggers a\n    special case within locate_rename_dst() where idx evaluates to 0\n    and rename_dst is NULL, meaning that our return value of\n    &rename_dst[idx] happens to evaluate to NULL as well.  This\n    addressing of an index into a NULL array hints at deeper problems,\n    which are raised in the next testcase...\n  * A second testcase which when run under valgrind shows that the code\n    actually depends upon unintialized memory, in particular the entry\n    just after the end of the rename_dst array.\n\nIn short, when the two rare options -B and --follow are used together,\nfix the accidental find of the wrong dst entry (which would often be\nuninitialized memory just past the end of the array, but also could\nhave just been a dst for an unrelated path if no dst was recorded for\nthe expected path).  Do so by adding a little more care around checking\nthe recorded indices in break_idx.\n\nReported-by: Olaf Hering <olaf@aepfle.de>\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n    diffcore-rename: fix BUG when break detection and --follow used together\n    \n    Bug dates back to Git v2.31.0, and was discovered and reported about\n    four years later over at\n    https://lore.kernel.org/git/20240920112228.3d1130f5.olaf@aepfle.de/.\n    Sadly, took me about half a year to get back to this one...\n    \n    Changes since v1:\n    \n     * Added a testcase, and extended the commit message slightly\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1876%2Fnewren%2Ffix-break-follow-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1876/newren/fix-break-follow-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1876\n\nRange-diff vs v1:\n\n 1:  e14c1193905 ! 1:  a68e7fe19f7 diffcore-rename: fix BUG when break detection and --follow used together\n     @@ Commit message\n          be the next one added to the array.  So, to fix this, we need to add a\n          little more safety around the checks for the recorded break_idx.\n      \n     -    It turns out that making a testcase to trigger this is a bit challenging\n     -    too.  I added a simple testcase which tickles the necessary area, but\n     -    running it normally actually passes for me.  However, running it under\n     -    valgrind shows that it is depending upon uninitialized memory.  I\n     -    suspect that to get a reliable reproduction case, I might need to have\n     -    several more paths involved, but that might make the testcase more\n     -    difficult to understand.  So, I instead just embedded a warning within\n     -    the testname that the test triggered uninitialized memory use.\n     +    It turns out that making a testcase to trigger this is quite the\n     +    challenge.  I actually added two testscases:\n     +      * One testcase which uses --follow incorrectly (it uses its single\n     +        pathspec to specifying something other than a single filename), and\n     +        which triggers the same bug reported-by Olaf.  This triggers a\n     +        special case within locate_rename_dst() where idx evaluates to 0\n     +        and rename_dst is NULL, meaning that our return value of\n     +        &rename_dst[idx] happens to evaluate to NULL as well.  This\n     +        addressing of an index into a NULL array hints at deeper problems,\n     +        which are raised in the next testcase...\n     +      * A second testcase which when run under valgrind shows that the code\n     +        actually depends upon unintialized memory, in particular the entry\n     +        just after the end of the rename_dst array.\n      \n     -    In short, when these two rare options are used together, fix the\n     -    accidental find of the wrong dst entry (which would often be\n     -    uninitialized memory just past the end of the array), by adding a little\n     -    more care around the recorded indices for break_idx.\n     +    In short, when the two rare options -B and --follow are used together,\n     +    fix the accidental find of the wrong dst entry (which would often be\n     +    uninitialized memory just past the end of the array, but also could\n     +    have just been a dst for an unrelated path if no dst was recorded for\n     +    the expected path).  Do so by adding a little more care around checking\n     +    the recorded indices in break_idx.\n      \n          Reported-by: Olaf Hering <olaf@aepfle.de>\n          Signed-off-by: Elijah Newren <newren@gmail.com>\n     @@ t/t4206-log-follow-harder-copies.sh: test_expect_success 'validate the output.'\n       \tcompare_diff_patch current expected\n       '\n       \n     -+test_expect_success 'log --follow -B does not die or use uninitialized memory' '\n     ++test_expect_success 'log --follow -B does not BUG' '\n      +\tgit switch --orphan break_and_follow_are_icky_so_use_both &&\n     ++\n     ++\ttest_seq 1 127 >numbers &&\n     ++\tgit add numbers &&\n     ++\tgit commit -m \"numbers\" &&\n     ++\n     ++\tprintf \"%s\\n\" A B C D E F G H I J K L M N O Q R S T U V W X Y Z >pool &&\n     ++\techo changed >numbers &&\n     ++\tgit add pool numbers &&\n     ++\tgit commit -m \"pool\" &&\n     ++\n     ++\tgit log -1 -B --raw --follow -- \"p*\"\n     ++'\n     ++\n     ++test_expect_success 'log --follow -B does not die or use uninitialized memory' '\n      +\tprintf \"%s\\n\" A B C D E F G H I J K L M N O P Q R S T U V W X Y Z >z &&\n      +\tgit add z &&\n      +\tgit commit -m \"Initial\" &&\n\n\n diffcore-rename.c                   |  9 ++++----\n t/t4206-log-follow-harder-copies.sh | 32 +++++++++++++++++++++++++++++\n 2 files changed, 37 insertions(+), 4 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 10bb0321b10..cb4be5be63c 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -33,7 +33,7 @@ static struct diff_rename_dst *locate_rename_dst(struct diff_filepair *p)\n {\n \t/* Lookup by p->ONE->path */\n \tint idx = break_idx ? strintmap_get(break_idx, p->one->path) : -1;\n-\treturn (idx == -1) ? NULL : &rename_dst[idx];\n+\treturn (idx == -1 || idx == rename_dst_nr) ? NULL : &rename_dst[idx];\n }\n \n /*\n@@ -1668,9 +1668,10 @@ void diffcore_rename_extended(struct diff_options *options,\n \t\t\tif (DIFF_PAIR_BROKEN(p)) {\n \t\t\t\t/* broken delete */\n \t\t\t\tstruct diff_rename_dst *dst = locate_rename_dst(p);\n-\t\t\t\tif (!dst)\n-\t\t\t\t\tBUG(\"tracking failed somehow; failed to find associated dst for broken pair\");\n-\t\t\t\tif (dst->is_rename)\n+\t\t\t\tif (options->single_follow && dst &&\n+\t\t\t\t    strcmp(dst->p->two->path, p->two->path))\n+\t\t\t\t\tdst = NULL;\n+\t\t\t\tif (dst && dst->is_rename)\n \t\t\t\t\t/* counterpart is now rename/copy */\n \t\t\t\t\tpair_to_free = p;\n \t\t\t}\ndiff --git a/t/t4206-log-follow-harder-copies.sh b/t/t4206-log-follow-harder-copies.sh\nindex bcab71c8e84..190c4843211 100755\n--- a/t/t4206-log-follow-harder-copies.sh\n+++ b/t/t4206-log-follow-harder-copies.sh\n@@ -54,4 +54,36 @@ test_expect_success 'validate the output.' '\n \tcompare_diff_patch current expected\n '\n \n+test_expect_success 'log --follow -B does not BUG' '\n+\tgit switch --orphan break_and_follow_are_icky_so_use_both &&\n+\n+\ttest_seq 1 127 >numbers &&\n+\tgit add numbers &&\n+\tgit commit -m \"numbers\" &&\n+\n+\tprintf \"%s\\n\" A B C D E F G H I J K L M N O Q R S T U V W X Y Z >pool &&\n+\techo changed >numbers &&\n+\tgit add pool numbers &&\n+\tgit commit -m \"pool\" &&\n+\n+\tgit log -1 -B --raw --follow -- \"p*\"\n+'\n+\n+test_expect_success 'log --follow -B does not die or use uninitialized memory' '\n+\tprintf \"%s\\n\" A B C D E F G H I J K L M N O P Q R S T U V W X Y Z >z &&\n+\tgit add z &&\n+\tgit commit -m \"Initial\" &&\n+\n+\ttest_seq 1 130 >z &&\n+\techo lame >somefile &&\n+\tgit add z somefile &&\n+\tgit commit -m \"Rewrite z, introduce lame somefile\" &&\n+\n+\techo Content >somefile &&\n+\tgit add somefile &&\n+\tgit commit -m \"Rewrite somefile\" &&\n+\n+\tgit log -B --follow somefile\n+'\n+\n test_done\n\nbase-commit: f93ff170b93a1782659637824b25923245ac9dd1\n-- \ngitgitgadget\n"},{"id":"514424","messageId":"20250317175135.GB704253@coredump.intra.peff.net","threadId":"63077","inReplyTo":"CABPp-BF5R_ob5uAS=m9Vt775-Er2fFqpH7jBjm_=MRmnRVbeew@mail.gmail.com","subject":"Re: [PATCH] diffcore-rename: fix BUG when break detection and --follow used together","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-17T17:51:35Z","receivedAt":"2025-03-17T17:51:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 14, 2025 at 03:28:09PM -0700, Elijah Newren wrote:\n\n> > So maybe this is the best we can do, but it feels like we should be able\n> > to at least trigger the existing BUG() reliably. I couldn't seem to\n> > figure it out, though. :(\n> \n> So, after _another_ 7 hours or so on it today...  The BUG() the\n> reporter triggered only happens when there is no uninitialized memory\n> use, and is only triggerable when you use invalid flags.  For the\n> reporter, they passed a directory name for their pathspec along with\n> --follow, despite the fact that --follow only works when given a\n> single pathspec that names an individual file.  You can also trigger\n> their particular manifestation of the issue here when using a glob\n> pathspec together with --follow.  In either event, the --follow\n> becomes useless: when the follow logic checks whether filenames are\n> equal to the given pathspec to see if it might be a relevant rename,\n> no filename is exactly equal to the pathspec, so it never finds any\n> relevant files to follow or to include in the rename detection.  The\n> upshot is the command basically behaves the same as if you hadn't\n> given --follow, other than the fact that the presence of --follow\n> makes diffcore_rename throw away rename_dst pairs in a way that\n> happens to trigger this particular BUG().\n\nOK, that all makes sense (and what I was trying to reduce the case, too,\nbut somehow it eluded me).\n\nSorry, I didn't mean for you to spend all day on it. I was just\nwondering if there was low-hanging fruit we could convince to trigger\nwith ASan (which would much better protect against regression). It does\nlook like we at least got some benefit, though. ;)\n\n> The testcase I found is:\n>     seq 1 127 >numbers &&\n>     git add numbers &&\n>     git commit -m \"numbers\" &&\n> \n>     printf \"%s\\n\" A B C D E F G H I J K L M N O Q R S T U V W X Y Z >pool &&\n>     seq 1 10 >numbers &&\n>     git add pool numbers &&\n>     git commit -m \"pool\" &&\n> \n>     git log -1 -B --raw --follow -- \"p*\"\n\nYeah, that's similar to what I was trying, but I didn't try the globbing\npathspec. I stuck everything in a subdirectory and use subdir/. But I\nthink I got caught up on...\n\n> The BUG will _not_ be triggered if:\n>   * you change the log's pathspec to match numbers\n\nI put both files into the subdir/, so they both matched.\n\n> In all of these cases (and this is also true for the reporter's\n> original case), in locate_rename_dst(), idx will be computed as 0, but\n> rename_dst is NULL, so &rename_dst[idx] is NULL as well.\n\nMakes sense.\n\n> However, I think the fact that rename_dst == NULL implies\n> &rename_dst[0] == NULL should raise alarm bells about the risks of\n> using memory improperly, even if it doesn't directly use uninitialized\n> memory in this case.  Which brings us to the bigger more encompassing\n> issue, which is what I reported in the commit message:\n> \n> > > In short, when these two rare options are used together, fix the\n> > > accidental find of the wrong dst entry (which would often be\n> > > uninitialized memory just past the end of the array), by adding a little\n> > > more care around the recorded indices for break_idx.\n> \n> It's just the special case when rename_dst is empty and, in fact,\n> NULL, that you trigger the BUG() call.\n\nOh, absolutely. The uninitialized memory is the bigger problem, and the\nfact that it BUG()-ed at all was mostly lucky.\n\n> Maybe what I wrote above helps.  Is this enough information to satisfy\n> your curiosity?\n\nDefinitely.\n\n> I suspect adding this second test to the commit makes sense.  Which\n> parts of my explanation in this email would you like to see added to\n> the commit message as well, or is it fine as-is?\n\nI read over your v2, and I think the explanation there is good. In the\nlong run we might eventually disallow --follow on the glob like this\n(since as you note, it's invalid and doesn't actually work). In which\ncase I guess we'd end up deleting that test case. But in the meantime, I\nthink there's some benefit to having it.\n\n-Peff\n"},{"id":"514425","messageId":"20250317175223.GC704253@coredump.intra.peff.net","threadId":"63077","inReplyTo":"pull.1876.v2.git.1742000894041.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] diffcore-rename: fix BUG when break detection and --follow used together","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-17T17:52:23Z","receivedAt":"2025-03-17T17:52:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 15, 2025 at 01:08:13AM +0000, Elijah Newren via GitGitGadget wrote:\n\n>     Changes since v1:\n>     \n>      * Added a testcase, and extended the commit message slightly\n\nI already left a bigger response in the thread on v1, but I wanted to\nadd here that this version looks great to me. Thanks!\n\n-Peff\n"}]}