[BUG] segmentation fault in git-diff

Hey there!

I have found a bug in the git-diff utility, which is reproducible in the next branch. In any repository, if I run

    git diff :/any/path/

(The important part is the trailing forward slash. No slash will generate either a valid diff or an error message about the path not being known. ":/" also works without issue)

it will trigger a SIGSEV. I have traced that back to the refs_read_raw_ref() function, where it seems the ref_store parameter passed to it is 0x0 (according to GDB). It's always possible to include a null-check in that function to fix the issue, but I don't think that'd be the best solution. I can attempt to fix it, but I don't know what (and where) the proper solution would be, because I don't know what the expected behavior is here, nor where exactly it should fail. Do you think this could also warrant the creation of a test?

I don't know what the best debug resources (valgrind output, core file from gdb) would be, but I can provide them if necessary.

Thanks,

Érico Nogueira

Érico Rolim <erico.erc@gmail.com> writes:

This is 'next' running for me:

    $ git diff :/any/path
    fatal: ambiguous argument ':/any/path': unknown revision or path not in the working tree.
    Use '--' to separate paths from revisions, like this:
    'git <command> [<revision>...] -- [<file>...]'

If looking for a commit that has a string "any/path" in its commit message may be consuming stack resources that are proportional to the depth of the history, however, I can imagine that it might run out of the memory and die, though. Another thing that may be interesting to try is to use "git rev-parse :/any/path" in the same repository you are having trouble with. If the "string :/any/path to a revision" logic is where it is dying, it would die the same way.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Scratch all that---sorry, but I did see the note about trailing slash, but somehow managed to forget adding it when I tried it.

Junio C Hamano <gitster@pobox.com> writes:

$ git checkout v2.22.0 && make && ./git-rev-parse :/any/path/

segfaults, while

$ git checkout v2.21.0 && make && ./git-rev-parse :/any/path/

is OK. We should be able to bisect this fairly straightforward between these two.

The problem bisects down to c931ba4e (sha1-name.c: remove the_repo from handle_one_ref(), 2019-04-16), which did this:

- for_each_ref(handle_one_ref, &list); - head_ref(handle_one_ref, &list); + cb.repo = repo; + cb.list = &list; + refs_for_each_ref(repo->refs, handle_one_ref, &cb); + refs_head_ref(repo->refs, handle_one_ref, &cb);

The old code used the helper for_each_ref(). This is a thin wrapper around refs_for_each_ref() and allows the default repository object to be used implicitly by the caller. It is understandable that the code wanted to work on arbitrary repository object, and replaced the for_each_ref() helper with refs_for_each_ref() helper that takes any ref store object.

But there is a small mistake. for_each_ref() makes sure that the ref store is initialized; the new code blindly assumes it has already been initialized.

    int for_each_ref(each_ref_fn fn, void *cb_data)
    {
            return refs_for_each_ref(get_main_ref_store(the_repository), fn, cb_data);
    }

So, I think the fix is simple. With the attached one liner on top of c931ba4e (sha1-name.c: remove the_repo from handle_one_ref(), 2019-04-16),

	$ git rev-parse :/any/path/

no longer segfaults. I think it would also work just fine when merged to more modern codebase, but I haven't tried it (yet).

 sha1-name.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Thu, Apr 09, 2020 at 03:57:00PM -0700, Junio C Hamano wrote:

Indeed. The culprit is c931ba4e78 (sha1-name.c: remove the_repo from handle_one_ref(), 2019-04-16), which swapped out for_each_ref() for refs_for_each_ref(repo->refs). But it misses the access method for repo->refs which lazy-initializes the pointer.

So the immediate fix is:

Jeff King <peff@peff.net> writes:

Here is the final version that I am going to apply and merge to 'jch' branch. This is an ancient regression in Git timescale, so its fix is not all that urgent, though.

-- >8 --
Subject: [PATCH] sha1-name: do not assume that the ref store is initialized

c931ba4e (sha1-name.c: remove the_repo from handle_one_ref(), 2019-04-16) replaced the use of for_each_ref() helper, which works with the main ref store of the default repository instance, with refs_for_each_ref(), which can work on any ref store instance, by assuming that the repository instance the function is given has its ref store already initialized.

But it is possible that nobody has initialized it, in which case, the code ends up dereferencing a NULL pointer.

Reported-by: Érico Rolim <erico.erc@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sha1-name.c                   | 2 +-
 t/t4208-log-magic-pathspec.sh | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

You guys sure are fast! Happy it could be solved somewhat easily.

Thanks!

On Thu, Apr 09, 2020 at 05:03:45PM -0700, Junio C Hamano wrote:

Agreed. The patch looks good to me.

I prepared the patch below on top. I mostly wanted it as an auditing tool to find similar cases, but there were none. It may still be worth applying to protect ourselves in the future.

-- >8 --
Subject: [PATCH] repository: mark the "refs" pointer as private

The "refs" pointer in a struct repository starts life as NULL, but then is lazily initialized when it is accessed via get_main_ref_store(). However, it's easy for calling code to forget this and access it directly, leading to code which works _some_ of the time, but fails if it is called before anybody else accesses the refs.

This was the cause of the bug fixed by 5ff4b920eb (sha1-name: do not assume that the ref store is initialized, 2020-04-09). In order to prevent similar bugs, let's more clearly mark the "refs" field as private.

In addition to helping future code, the name change will help us audit any existing direct uses. Besides get_main_ref_store() itself, it turns out there is only one. But we know it's OK as it is on the line directly after the fix from 5ff4b920eb, which will have initialized the pointer. However it's still a good idea for it to model the proper use of the accessing function, so we'll convert it.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c       | 8 ++++----
 repository.h | 8 ++++++--
 sha1-name.c  | 2 +-
 3 files changed, 11 insertions(+), 7 deletions(-)

Jeff King <peff@peff.net> writes:

Makes perfect sense.

Will queue.  Thanks.