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
