On Wed, Jan 30, 2019 at 3:59 PM brian m. carlson <sandals@xxxxxxxxxxxxxxxxxxxx> wrote: > > It looks like t5702 is failing on master under ASan (output below). It > bisects to the merge of my sha-256 series, but the error makes it look > like it's unrelated. I'm wondering if we just happened to have a > different memory layout with that series that's triggering this issue in > combination with one of the protocol v2 series from Jonathan Tan, and > the correct solution is something like this: > > ----- %< ------- > diff --git a/fetch-pack.c b/fetch-pack.c > index 577faa6229..c9dda154da 100644 > --- a/fetch-pack.c > +++ b/fetch-pack.c > @@ -1489,6 +1489,7 @@ static void update_shallow(struct fetch_pack_args *args, > rollback_lock_file(&shallow_lock); > } else > commit_lock_file(&shallow_lock); > + alternate_shallow_file = ""; > return; > } > > @@ -1512,6 +1513,7 @@ static void update_shallow(struct fetch_pack_args *args, > &alternate_shallow_file, > &extra); > commit_lock_file(&shallow_lock); > + alternate_shallow_file = ""; > } > oid_array_clear(&extra); > return; > @@ -1551,6 +1553,7 @@ static void update_shallow(struct fetch_pack_args *args, > commit_lock_file(&shallow_lock); > oid_array_clear(&extra); > oid_array_clear(&ref); > + alternate_shallow_file = ""; > return; > } > > ----- %< ------- > > This does appear to pass the testsuite, but I'm unsure if it's correct. > It's also possible I've just broken something and am too dense to > realize it, so please point out if that's the case. If I understand ASan report correctly alternate_shallow_file memory is already gone after the first fetch, when we update the shallow file. But we're doing two fetches in the same process (the tag backfill thingy), the second fetch reuses the dangling alternate_shallow_file pointer and ASan caught it. Resetting the variable seems like the right way to go. But should we reset it to an empty string? We would pass "--shallow-file=" to "git index-pack", which is treated as "no shallow file" (i.e. complete repo). This sounds wrong because this is still a shallow repository. I suppose setting alternate_shallow_file to NULL would be ok. "git index-pack" will just go back to reading $GIT_DIR/info/shallow, which has been updated and contains correct info. PS. No idea how ASan blames your series for this. Yeah maybe memory layout and stuff. But it does spot a real problem. -- Duy
