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

From: Jeff King <peff@peff.net>
Date: Mar 17, 2025, 17:51 UTC
Message-ID: <20250317175135.GB704253@coredump.intra.peff.net>
In-Reply-To: <CABPp-BF5R_ob5uAS=m9Vt775-Er2fFqpH7jBjm_=MRmnRVbeew@mail.gmail.com>

On Fri, Mar 14, 2025 at 03:28:09PM -0700, Elijah Newren wrote:

Show 20 quoted lines

> > 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. :(
> 
> So, after _another_ 7 hours or so on it today...  The BUG() the
> reporter triggered only happens when there is no uninitialized memory
> use, and is only triggerable when you use invalid flags.  For the
> reporter, they passed a directory name for their pathspec along with
> --follow, despite the fact that --follow only works when given a
> single pathspec that names an individual file.  You can also trigger
> their particular manifestation of the issue here when using a glob
> pathspec together with --follow.  In either event, the --follow
> becomes useless: when the follow logic checks whether filenames are
> equal to the given pathspec to see if it might be a relevant rename,
> no filename is exactly equal to the pathspec, so it never finds any
> relevant files to follow or to include in the rename detection.  The
> upshot is the command basically behaves the same as if you hadn't
> given --follow, other than the fact that the presence of --follow
> makes diffcore_rename throw away rename_dst pairs in a way that
> happens to trigger this particular BUG().

OK, that all makes sense (and what I was trying to reduce the case, too, but somehow it eluded me).

Sorry, I didn't mean for you to spend all day on it. I was just wondering if there was low-hanging fruit we could convince to trigger with ASan (which would much better protect against regression). It does look like we at least got some benefit, though. ;)

Show 11 quoted lines

> The testcase I found is:
>     seq 1 127 >numbers &&
>     git add numbers &&
>     git commit -m "numbers" &&
> 
>     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 &&
>     seq 1 10 >numbers &&
>     git add pool numbers &&
>     git commit -m "pool" &&
> 
>     git log -1 -B --raw --follow -- "p*"

Yeah, that's similar to what I was trying, but I didn't try the globbing pathspec. I stuck everything in a subdirectory and use subdir/. But I think I got caught up on...

> The BUG will _not_ be triggered if:
>   * you change the log's pathspec to match numbers

I put both files into the subdir/, so they both matched.

> In all of these cases (and this is also true for the reporter's
> original case), in locate_rename_dst(), idx will be computed as 0, but
> rename_dst is NULL, so &rename_dst[idx] is NULL as well.

Makes sense.

Show 13 quoted lines

> However, I think the fact that rename_dst == NULL implies
> &rename_dst[0] == NULL should raise alarm bells about the risks of
> using memory improperly, even if it doesn't directly use uninitialized
> memory in this case.  Which brings us to the bigger more encompassing
> issue, which is what I reported in the commit message:
> 
> > > 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.
> 
> It's just the special case when rename_dst is empty and, in fact,
> NULL, that you trigger the BUG() call.

Oh, absolutely. The uninitialized memory is the bigger problem, and the fact that it BUG()-ed at all was mostly lucky.

> Maybe what I wrote above helps.  Is this enough information to satisfy
> your curiosity?

Definitely.

> I suspect adding this second test to the commit makes sense.  Which
> parts of my explanation in this email would you like to see added to
> the commit message as well, or is it fine as-is?

I read over your v2, and I think the explanation there is good. In the long run we might eventually disallow --follow on the glob like this (since as you note, it's invalid and doesn't actually work). In which case I guess we'd end up deleting that test case. But in the meantime, I think there's some benefit to having it.

-Peff