Bug: Segfault when doing "git diff"

> 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

>
>
> 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

> 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

> 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;

> > 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.