Re: segfault on invalid 'git grep' invocation

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 28, 2021, 18:53 UTC
Message-ID: <87bl7phl41.fsf@evledraar.gmail.com>
In-Reply-To: <20210628121748.f3yrc72v4mynknl3@debianXPS.lan>

On Mon, Jun 28 2021, Matthew Hughes wrote:

> Hi all,
>
> It's my first time writing to this mailing list, so apologies in advance
> for any mistakes/faux pas.

Thanks a lot for the report, and welcome.

Show 29 quoted lines

> Summary
> =======
>
> Segfault seen when running:
>
>     $ ./git grep --and -e 'pattern' -- not_a_path
>
> Expected Behaviour
> ==================
>
> The above invocation doesn't appear particularly valid to me, I
> accidentally ran it while editing a command. Not sure if this needs
> addressing, but perhaps the expected behaviour would be an error message
> about it not being valid?
>
> Compare with, running without '--':
>
>     $ ./git grep --and -e 'pattern' not_a_path
>     fatal: ambiguous argument 'not_a_path': unknown revision or path not in the working tree.
>     Use '--' to separate paths from revisions, like this:
>     'git <command> [<revision>...] -- [<file>...]'
>
> Running with a valid path:
>
>     $ ./git grep --and -e 'pattern' -- common-main.c
>     fatal: Not a valid grep expression
>
>     $ ./git grep --and -e 'pattern' common-main.c
>     fatal: Not a valid grep expression

The exact segfault you've found is found at least back to Git version 2.0.0, so it's been with us for a while.

The problem is that you're trying to --and two things, but did not provide two things. We apparently never checked for this, and then segfault in the cleanup, thinking an --and must imply that we have two sides.

I haven't looked deeply, but think that the reason it segfaults just with not_a_path is that we then proceed to die() in something that should probably be a BUG() instead as we can't grep in the expected-but-not-there pattern at runtime.

Giving it a pathspec that doesn't exist means we never run the "real" grep, but we do run the cleanup, hence the segfault.