Bug: git grep --no-index 123 /dev/stdin crashes with SIGABRT

Thank you for filling out a Git bug report! Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue) `git grep --no-index 123 /dev/stdin` outside of git repository

What did you expect to happen? (Expected behavior) Ability to grep input from stdin

What happened instead? (Actual behavior) `git grep` crashed with SIGABRT

$ git grep --no-index 123 /dev/stdin
BUG: environment.c:215: git environment hasn't been setup
Aborted (core dumped)

What's different between what you expected and what actually happened? Crash, no grep result

Anything else you want to add: Plain grep can do that, so I suppose `git grep` could also:

$ echo -e "123\n456\n789" | grep 456 /dev/stdin 456

Please review the rest of the bug report below. You can delete any lines you don't wish to share.

[System Info] git version: git version 2.41.0 cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh uname: Linux 6.5.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Oct 6 19:02:35 UTC 2023 x86_64 compiler info: gnuc: 13.1 libc info: glibc: 2.37 $SHELL (typically, interactive shell): /bin/bash

[Enabled Hooks] not run from a git repository - no hooks to show

Hi ks1322 

On Sat, Oct 14, 2023, at 17:42, ks1322 ks1322 wrote:

It looks like `setup.c:verify_filename` fails to deny paths that are not transitive children (or whatever the term) of the directory that git(1) is running in:

    $ git -C ~/IdeaProjects/ grep --no-index dfddf ~
    BUG: environment.c:213: git environment hasn't been setup
    Aborted (core dumped)

So `builtin/grep.c` goes past that check, into `pathspec.c:init_pathspec_item` and dies at line 472 since that function assumes that we are in a Git repository.

On Sat, Oct 14, 2023, at 20:12, Kristoffer Haugsbakk wrote:

No, that's not it. I don't think that's the responsibility of that function. Compare to a successful run, that is when run in a Git repository:

    $ git grep --no-index dfddf /home
    fatal: /home: '/home' is outside repository at '/home/kristoffer/programming/git'

And that error happens at `pathspec.c:473`.

So going down into that file is not wrong.

Die gracefully when `git grep --no-index` is run outside of a Git repository and the path is outside the directory tree.

If you are not in a Git repository and say:

    git grep --no-index search ..

You trigger a `BUG`:

    BUG: environment.c:213: git environment hasn't been setup
    Aborted (core dumped)

Because `..` is a valid path which is treated as a pathspec. Then `pathspec` figures out that it is not in the current directory tree. The `BUG` is triggered when `pathspec` tries to advice the user about the path to the (non-existing) repository.

Reported-by: ks1322 ks1322 <ks1322@gmail.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 pathspec.c      |  3 +++
 t/t7810-grep.sh | 13 +++++++++++++
 2 files changed, 16 insertions(+)

On Sat, Oct 14, 2023 at 11:02:38PM +0200, Kristoffer Haugsbakk wrote:

Is it even reasonable for "grep --no-index" to care about leaving the tree in the first place? That is, is there a reason we should not allow:

  git grep --no-index foo ../bar

? And if we do want to care, there is a weirdness here that even with your patch, we check to see if the file exists:

  $ git grep --no-index foo ../does-exist
  fatal: '../does-exist' is outside the directory tree

  $ git grep --no-index foo ../does-not-exist
  fatal: ../does-not-exist: no such path in the working tree.

If we want to avoid leaving the current directory, then I think we need to be checking much sooner (but again, I would argue that it is not worth caring about in no-index mode).

I do think your patch does not make anything worse (and indeed makes the error output much better). So I do not mind it in the meantime. But I have a feeling that we'd end up reverting it as part of the fix for the larger issue.

-Peff

On Sun, Oct 15, 2023, at 05:26, Jeff King wrote:

On second thought yeah, it doesn't make sense. We are outside of any repository already so what does it matter where the file is relative to the current working directory?

It seems that `pathspec.c:init_pathspec_item` should let you through in this case.

Jeff King <peff@peff.net> writes:

A huge difference between the bare "grep" and "git grep" is that we know the scope of the project tree, so it goes recursive by default. Should the above command line recursively go below ../bar? Would we allow "/" to be given?

I actually do not think these "we are allowing Git tools to be used on random garbage" is a good idea to begin with X-<. If we invented something nice for our variant in "git grep" and wish we can use it outside the repository, contributing the feature to implementations of "grep" would have been the right way to move forward, instead of contaminating the codebase with things that are not related to Git. Whoever did 59332d13 (Resurrect "git grep --no-index", 2010-02-06) should be punished X-<.

Anyway.

2e48fcdb (grep docs: document --no-index option, 2010-02-25) seems to have wanted to explicitly limit the search within the "current directory", and I am fine to keep the search space limited by the cwd. On the other hand, of course, the users can shoot themselves in the foot with "grep -r foo /", so letting them use "git grep" the same way is perhaps OK. Especially if it simplifies the code if we lift the limitation, that is a very tempting thing to do.

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

It is curious that the original has two sources of hint_path (i.e., get_git_dir() is used as a fallback for get_git_work_tree()). Are we certain that the check is at the right place? If we do not have a repository, then both would fail by returning NULL, so it should not matter if we add the new check before we check either or both, or even after we checked both before dying.

I wonder if

	const char *hint_path = get_git_work_tree();

	if (!hint_path)
	        hint_path = get_git_dir();
	if (hint_path)
		die(_("%s: '%s' is outside repository at '%s'"),
		    elt, copyfrom, absolute_path(hint_path));
	else
		die(_("%s: '%s' is outside the directory tree"),
		    elt, copyfrom);

makes the intent of the code clearer. We want to hint the location of the repository by computing hint_path, and if we can compute it, we use it in the error message, but otherwise we don't add hint. And we apply that conditional whether we have repository or not---what we care about is the NULL-ness of the hint string we computed.

Excellent. This is a very good use of the GIT_CEILING_DIRECTORIES facility.

On Tue, Oct 17, 2023, at 18:42, Junio C Hamano wrote:

That doesn't work since `get_git_dir()` triggers `BUG` instead of returning `NULL`.

The `hint_path` declaration has to be at the start because of style rules. But we can initialize it after.

I can also have a second look at the test since I am using `grep` to test the failure output and not the translation string variant.

-- >8 --
Subject: [PATCH] fixup! grep: die gracefully when outside repository

---
 pathspec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

Ah, interesting.

Yes, what you have below (but please leave a blank line between the last line of decl and the first line of statement for readablility) looks very readable and sensible.

That is not necessary, as we no longer run under phoney i18n that required us to use test_i18ngrep. It is OK to assume that the tests are run under "C" locale.

Thanks.

Kristoffer Haugsbakk <code@khaugsbakk.name> writes:

So you create non/git/sub, go to non/git (so there is sub/ directory), and try running "..".

If you had a directory non/tig next to non/git and used ../tig instead of .. as the path given to "git grep", it would also correctly fail. Searching in a non-existing path, ../non, dies in a different error, with an error message that is not technically wrong, but it probably can be improved. It has been a while since I looked at the pathspec matching code, but if we are lucky, it might be just the matter of swapping the order of checking (in other words, check "is it outside" first and then "does it exist" next, or something like that)?

 t/t7810-grep.sh | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Die gracefully when `git grep --no-index` is run outside of a Git repository and the path is outside the directory tree.

If you are not in a Git repository and say:

    git grep --no-index search ..

You trigger a `BUG`:

    BUG: environment.c:213: git environment hasn't been setup
    Aborted (core dumped)

Because `..` is a valid path which is treated as a pathspec. Then `pathspec` figures out that it is not in the current directory tree. The `BUG` is triggered when `pathspec` tries to advice the user about how the path is not in the current (non-existing) repository.

Reported-by: ks1322 ks1322 <ks1322@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2:
    - Initialize `hint_path` after we know that we are in a Git repository
    - Apply Junio's suggestion for the test: https://lore.kernel.org/git/xmqqzg0hf0g8.fsf@gitster.g/

 pathspec.c      |  7 ++++++-
 t/t7810-grep.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)

On Fri, Oct 20, 2023 at 12:40 PM Kristoffer Haugsbakk <code@khaugsbakk.name> wrote:

s/advice/advise/

(probably not worth a reroll)

Eric Sunshine <sunshine@sunshineco.com> writes:

The only remaining niggle I have is that the effect of this change would be much wider than just "grep", but in "git shortlog" output it may appear that this is specific to it, making later developers' life a bit harder when they are hunting for the cause of a behaviour change that is outside "grep", but still caused by this patch.

But I think I am worried too much in this particular case. Once this codepath is entered, the code will die no matter what, and we are merely making it die a bit more nicely.

There still is the "we say different things depending on the path outside the hierarchy exists or not" raised by Peff remaining, but for now, let's declare a victory and merge it to 'next'.

Thanks.

Die gracefully when `git grep --no-index` is run outside of a Git repository and the path is outside the directory tree.

If you are not in a Git repository and say:

    git grep --no-index search ..

You trigger a `BUG`:

    BUG: environment.c:213: git environment hasn't been setup
    Aborted (core dumped)

Because `..` is a valid path which is treated as a pathspec. Then `pathspec` figures out that it is not in the current directory tree. The `BUG` is triggered when `pathspec` tries to advise the user about how the path is not in the current (non-existing) repository.

Reported-by: ks1322 ks1322 <ks1322@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2:
    - Initialize `hint_path` after we know that we are in a Git repository
    - Apply Junio's suggestion for the test: https://lore.kernel.org/git/xmqqzg0hf0g8.fsf@gitster.g/
    v3:
    - commit message: correct to “advise” (“advice” is a noun)

 pathspec.c      |  7 ++++++-
 t/t7810-grep.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 1 deletion(-)