Bug: Segfault when doing "git diff"

Bug: Segfault when doing "git diff"

[Mathias L. Baumann]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

Hello dear git devs,

I just stumbled upon a segfault when doing just "git diff" in my repo.

I managed to create a minimal repo setup where the bug is reproducable.

The problem seems to be a mix of having an untracked submodule and 
having set an alternates file for one submodule.

Attached you'll find my setup that will reproduce the problem. Simply 
run  'git diff' in bugtest1.

In case the attachment is refused, I also uploaded it here:

http://supraverse.net/bugdemo.tar.gz

cheers,

     --Marenz

[-- Attachment #2: bugdemo.tar.gz --]
[-- Type: application/x-gzip, Size: 25120 bytes --]

Re: Bug: Segfault when doing "git diff"

[Victor Leschuk]

On 10/28/2015 02:58 PM, Mathias L. Baumann wrote:
> Hello dear git devs,
>
> I just stumbled upon a segfault when doing just "git diff" in my repo.
>
> I managed to create a minimal repo setup where the bug is reproducable.
>
> The problem seems to be a mix of having an untracked submodule and 
> having set an alternates file for one submodule.
>
> Attached you'll find my setup that will reproduce the problem. Simply 
> run  'git diff' in bugtest1.
>
> In case the attachment is refused, I also uploaded it here:
>
> http://supraverse.net/bugdemo.tar.gz
>
> cheers,
>
>     --Marenz
Hello Marenz,

I have just tried to reproduce segfault with the provided archive:

[del@del-debian bugtest1 (master)]$ git diff
diff --git a/submodules/bugtest2 b/submodules/bugtest2
--- a/submodules/bugtest2
+++ b/submodules/bugtest2
@@ -1 +1 @@
-Subproject commit cd0b9ee2946d2df3626943347332a4d86f93b126
+Subproject commit cd0b9ee2946d2df3626943347332a4d86f93b126-dirty

No segfault occured. I am using

git version 2.6.2.308.g3b8f10c

Could you please specify which version of git you are using and also try 
to reproduce it with latest 2.6.2?

--
Victor

Re: Bug: Segfault when doing "git diff"

[Mathias L. Baumann]

I was using the latest git version 2.6.2 already.
I suspect it is due to a .gitconfig. This is what is probably required:

➜  ~  cat .gitconfig
[diff]
     submodule = log


On 28/10/15 13:24, Victor Leschuk wrote:
>
>
> On 10/28/2015 02:58 PM, Mathias L. Baumann wrote:
>> Hello dear git devs,
>>
>> I just stumbled upon a segfault when doing just "git diff" in my repo.
>>
>> I managed to create a minimal repo setup where the bug is reproducable.
>>
>> The problem seems to be a mix of having an untracked submodule and
>> having set an alternates file for one submodule.
>>
>> Attached you'll find my setup that will reproduce the problem. Simply
>> run  'git diff' in bugtest1.
>>
>> In case the attachment is refused, I also uploaded it here:
>>
>> http://supraverse.net/bugdemo.tar.gz
>>
>> cheers,
>>
>>     --Marenz
> Hello Marenz,
>
> I have just tried to reproduce segfault with the provided archive:
>
> [del@del-debian bugtest1 (master)]$ git diff
> diff --git a/submodules/bugtest2 b/submodules/bugtest2
> --- a/submodules/bugtest2
> +++ b/submodules/bugtest2
> @@ -1 +1 @@
> -Subproject commit cd0b9ee2946d2df3626943347332a4d86f93b126
> +Subproject commit cd0b9ee2946d2df3626943347332a4d86f93b126-dirty
>
> No segfault occured. I am using
>
> git version 2.6.2.308.g3b8f10c
>
> Could you please specify which version of git you are using and also try
> to reproduce it with latest 2.6.2?
>
> --
> Victor

Re: Bug: Segfault when doing "git diff"

[Victor Leschuk]

On 10/28/2015 04:35 PM, Mathias L. Baumann wrote:
> I was using the latest git version 2.6.2 already.
> I suspect it is due to a .gitconfig. This is what is probably required:
>
> ➜  ~  cat .gitconfig
> [diff]
>     submodule = log
Yep, that did the trick.

The segfault is from

sha1_file.c:

/* add the alternate entry */
  *alt_odb_tail = ent; /* <===== alt_obd_tail is NULL here */
alt_odb_tail = &(ent->next);
ent->next = NULL;

Will try to take a closer look at it.

--
Victor

[PATCH] add_submodule_odb: initialize alt_odb list earlier

[Jeff King]

On Wed, Oct 28, 2015 at 02:35:23PM +0100, Mathias L. Baumann wrote:

> I was using the latest git version 2.6.2 already.
> I suspect it is due to a .gitconfig. This is what is probably required:
> 
> ➜  ~  cat .gitconfig
> [diff]
>     submodule = log

Yeah, I can reproduce it easily with that. Thanks for providing the
repository. It takes a rather convoluted set of conditions to trigger
the bug. :)

Here's the fix:

-- >8 --
Subject: add_submodule_odb: initialize alt_odb list earlier

The add_submodule_odb function tries to add a submodule's
object store as an "alternate". It needs the existing list
to be initialized (from the objects/info/alternates file)
for two reasons:

  1. We look for duplicates with the existing alternate
     stores, but obviously this doesn't work if we haven't
     loaded any yet.

  2. We link our new entry into the list by prepending it to
     alt_odb_list. But we do _not_ modify alt_odb_tail.
     This variable starts as NULL, and is a signal to the
     alt_odb code that the list has not yet been
     initialized.

     We then call read_info_alternates on the submodule (to
     recursively load its alternates), which will try to
     append to that tail, assuming it has been initialized.
     This causes us to segfault if it is NULL.

This rarely comes up in practice, because we will have
initialized the alt_odb any time we do an object lookup. So
you can trigger this only when:

  - you try to access a submodule (e.g., a diff with
    diff.submodule=log)

  - the access happens before any other object has been
    accessed (e.g., because the diff is between the working
    tree and the index)

  - the submodule contains an alternates file (so we try to
    add an entry to the NULL alt_odb_tail)

To fix this, we just need to call prepare_alt_odb at the
start of the function (and if we have already initialized,
it is a noop).

Note that we can remove the prepare_alt_odb call from the
end. It is guaranteed to be a noop, since we will have
called it earlier.

Signed-off-by: Jeff King <peff@peff.net>
---
 submodule.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index 5879cfb..88af54c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -130,6 +130,7 @@ static int add_submodule_odb(const char *path)
 		goto done;
 	}
 	/* avoid adding it twice */
+	prepare_alt_odb();
 	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
 		if (alt_odb->name - alt_odb->base == objects_directory.len &&
 				!strncmp(alt_odb->base, objects_directory.buf,
@@ -148,7 +149,6 @@ static int add_submodule_odb(const char *path)
 
 	/* add possible alternates from the submodule */
 	read_info_alternates(objects_directory.buf, 0);
-	prepare_alt_odb();
 done:
 	strbuf_release(&objects_directory);
 	return ret;
-- 
2.6.2.572.g6ed22dd

Re: [PATCH] add_submodule_odb: initialize alt_odb list earlier

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I can reproduce it easily with that. Thanks for providing the
> repository. It takes a rather convoluted set of conditions to trigger
> the bug. :)
>
> Here's the fix:
>
> -- >8 --
> Subject: add_submodule_odb: initialize alt_odb list earlier
>
> The add_submodule_odb function tries to add a submodule's
> object store as an "alternate". It needs the existing list
> to be initialized (from the objects/info/alternates file)
> for two reasons:
>
>   1. We look for duplicates with the existing alternate
>      stores, but obviously this doesn't work if we haven't
>      loaded any yet.
>
>   2. We link our new entry into the list by prepending it to
>      alt_odb_list. But we do _not_ modify alt_odb_tail.
>      This variable starts as NULL, and is a signal to the
>      alt_odb code that the list has not yet been
>      initialized.
>
>      We then call read_info_alternates on the submodule (to
>      recursively load its alternates), which will try to
>      append to that tail, assuming it has been initialized.
>      This causes us to segfault if it is NULL.
>
> This rarely comes up in practice, because we will have
> initialized the alt_odb any time we do an object lookup. So
> you can trigger this only when:
>
>   - you try to access a submodule (e.g., a diff with
>     diff.submodule=log)
>
>   - the access happens before any other object has been
>     accessed (e.g., because the diff is between the working
>     tree and the index)
>
>   - the submodule contains an alternates file (so we try to
>     add an entry to the NULL alt_odb_tail)
>
> To fix this, we just need to call prepare_alt_odb at the
> start of the function (and if we have already initialized,
> it is a noop).
>
> Note that we can remove the prepare_alt_odb call from the
> end. It is guaranteed to be a noop, since we will have
> called it earlier.

Thanks for a quick and detailed diagnosis and a fix.

The removal is correct, but even without this fix, the order of
calls in the original should have screamed "bug" loudly at us, I
think.  We shouldn't be reading data from alternates file without
first preparing the place we read data into.

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  submodule.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/submodule.c b/submodule.c
> index 5879cfb..88af54c 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -130,6 +130,7 @@ static int add_submodule_odb(const char *path)
>  		goto done;
>  	}
>  	/* avoid adding it twice */
> +	prepare_alt_odb();
>  	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
>  		if (alt_odb->name - alt_odb->base == objects_directory.len &&
>  				!strncmp(alt_odb->base, objects_directory.buf,
> @@ -148,7 +149,6 @@ static int add_submodule_odb(const char *path)
>  
>  	/* add possible alternates from the submodule */
>  	read_info_alternates(objects_directory.buf, 0);
> -	prepare_alt_odb();
>  done:
>  	strbuf_release(&objects_directory);
>  	return ret;

Re: [PATCH] add_submodule_odb: initialize alt_odb list earlier

[Jeff King]

On Wed, Oct 28, 2015 at 08:24:17AM -0700, Junio C Hamano wrote:

> > Note that we can remove the prepare_alt_odb call from the
> > end. It is guaranteed to be a noop, since we will have
> > called it earlier.
> 
> Thanks for a quick and detailed diagnosis and a fix.
> 
> The removal is correct, but even without this fix, the order of
> calls in the original should have screamed "bug" loudly at us, I
> think.  We shouldn't be reading data from alternates file without
> first preparing the place we read data into.

Yeah, I agree. I spent a long time trying to figure out if that
prepare_alt_odb was actually doing something useful (like if it was
needed to somehow "cement" the new alt into place).

But I don't think it was.

In the majority of cases, it was a noop (we had already prepared when we
looked up the first object). But for other cases...

  - if read_info_alternates actually did something, we segfaulted (i.e.,
    this bug)

  - otherwise, we would prepare on _top_ of what we just added to the
    list, which was probably buggy (I didn't dig far enough to see if
    prepare_alt_odb() would overwrite what we just added to the list).

So some pretty dark corners of the code. :)

-Peff