Jeff King <p...@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 <p...@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; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
