Re: [PATCH] diffcore-rename: fix BUG when break detection and --follow used together

From: Jeff King <peff@peff.net>
Date: Mar 14, 2025, 17:24 UTC
Message-ID: <20250314172437.GA16311@coredump.intra.peff.net>
In-Reply-To: <pull.1876.git.1741395615315.gitgitgadget@gmail.com>

On Sat, Mar 08, 2025 at 01:00:15AM +0000, Elijah Newren via GitGitGadget wrote:

Show 8 quoted lines

> It turns out that making a testcase to trigger this is a bit challenging
> too.  I added a simple testcase which tickles the necessary area, but
> running it normally actually passes for me.  However, running it under
> valgrind shows that it is depending upon uninitialized memory.  I
> suspect that to get a reliable reproduction case, I might need to have
> several more paths involved, but that might make the testcase more
> difficult to understand.  So, I instead just embedded a warning within
> the testname that the test triggered uninitialized memory use.

I think it's OK for a test case to require extra memory checks to fail; after all, these kinds of bugs are usually non-deterministic without those checks anyway.

I did verify that it reproduces for me with "--valgrind". I was surprised (and a little disappointed) that it doesn't seem to trigger with ASan/UBSan. We do run those routinely in CI, but I doubt that --valgrind gets used regularly for the whole test suite by anyone these days, just because it's so much slower.

I'm puzzled, though, why the test case at the beginning of this thread[1] yields the BUG() so readily, but your test case doesn't.

So maybe this is the best we can do, but it feels like we should be able to at least trigger the existing BUG() reliably. I couldn't seem to figure it out, though. :(

> In short, when these two rare options are used together, fix the
> accidental find of the wrong dst entry (which would often be
> uninitialized memory just past the end of the array), by adding a little
> more care around the recorded indices for break_idx.

Your description of the problem and the solution both seemed sensible to me (though I'm not all that familiar with the ins and outs of the rename code these days).

-Peff

[1] The simplest I came up with is:

      git clone --bare https://github.com/intel/linux-sgx.git tmp.git
      cd tmp.git
      git --no-pager log -B --follow 63d0e65cfa49bb46a8dbe8745bb15aaf226faa97 -- external/ippcp_internal/inc

    Curiously, that pathspec is actually a directory, but it only has a
    single file in it. Feeding the actual file in it _doesn't_ trigger
    the bug:

      git --no-pager log -B --follow 63d0e65cfa49bb46a8dbe8745bb15aaf226faa97 -- external/ippcp_internal/inc/ippcp20u3.patch

    That just shows the single commit (even though there is a single
    file before and after that commit, it is not similar enough to find
    a rename).

    The file that hits the break detection is unrelated. It looks like
    it's psw/ae/data/prebuilt/le_prod_css.bin.

    I tried variants with break files, subdirs, etc, but I couldn't seem
    to make anything work.