Message ID | 20190130085855.GA24387@genre.crustytoothpaste.net (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | t5702 failing under ASan on master | expand |
On Wed, Jan 30, 2019 at 3:59 PM brian m. carlson <sandals@crustytoothpaste.net> 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.
On Wed, Jan 30, 2019 at 05:07:20PM +0700, Duy Nguyen wrote: > 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. Ah, I think I was missing the fact that we're doing a tag backfill. That explains a lot. > 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. Yeah, that sounds like a better choice. I'll send a complete patch which does this. > PS. No idea how ASan blames your series for this. Yeah maybe memory > layout and stuff. But it does spot a real problem. I don't doubt this is a problem. We'll definitely want to fix it before the release, since if I see it in development, somebody will likely see it in production.
"brian m. carlson" <sandals@crustytoothpaste.net> writes: > On Wed, Jan 30, 2019 at 05:07:20PM +0700, Duy Nguyen wrote: >> 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. > > Ah, I think I was missing the fact that we're doing a tag backfill. That > explains a lot. > >> 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. > > Yeah, that sounds like a better choice. I'll send a complete patch which > does this. Thanks, both.
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; }