Re: What's cooking in git.git (Oct 2017, #01; Wed, 4)

[Jeff King <peff@peff.net>]

On Wed, Oct 04, 2017 at 04:02:12AM -0400, Jeff King wrote:

> On Wed, Oct 04, 2017 at 04:19:52PM +0900, Junio C Hamano wrote:
> 
> > * jt/partial-clone-lazy-fetch (2017-10-02) 18 commits
> [...]
> 
> The merge of this topic into jch (at 766f92478b0) causes the test suite
> to fail when compiled with ASan/UBSan. The simplest reproduction I could
> come up with is:
> 
>    $ make SANITIZE=address,undefined BLK_SHA1=1 &&
>      GIT_DIR=nope ./git shortlog </dev/null >/dev/null
>    repository.c:69:31: runtime error: index 1869098813 out of bounds for type 'git_hash_algo [1]'
> 
> Note that the series is fine by itself, it's only the merge which fails.
> Which implies to me it's some funny interaction with bc/hash-algo (which
> introduces the hash_algo concept). But I didn't dig further.
> 
> +cc brian and Jonathan.

Actually, I take it back. Bisecting between "master" and "pu" turned up
that commit (and I verified that it fails at that commit but not at its
first parent).

But if I go directly to the tip of bc/hash-algo, I can see the failure
there. And it bisects to 67a9dfcc00 (hash-algo: integrate hash algorithm
support with repo setup, 2017-08-21).

So I think it's a read of uninitialized data, and it may come and go as
various topics tickle the compiler differently. And that would make
jt/partial-clone-lazy-fetch a red herring.

This seems to make it go away (on top of 67a9dfcc00):

diff --git a/setup.c b/setup.c
index 289e24811c..310afe9736 100644
--- a/setup.c
+++ b/setup.c
@@ -1126,7 +1126,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			repo_set_gitdir(the_repository, gitdir);
 			setup_git_env();
 		}
-		repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
+		/* This is only valid if we actually found a repository */
+		if (startup_info->have_repository)
+			repo_set_hash_algo(the_repository, repo_fmt.hash_algo);
 	}
 
 	strbuf_release(&dir);

The problem is that we may not have a real repository at all, but we
still hit this code path, which tries to setup repository properties, if
there's a $GIT_DIR set.

I actually suspect this repo_set_hash_algo() line should go into
check_repository_format_gently(), where we set other globals (and which
should eventually take a pointer to struct repository rather than
touching the_repository).

But I'm sufficiently convinced now that this is just a bug in the
RFC-marked bc/hash-algo topic, and in no danger of graduating to master
soon.

-Peff