| Message ID | 20190204000650.686175-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fetch-pack: clear alternate shallow when complete | expand |
On Mon, Feb 4, 2019 at 7:06 AM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > When we write an alternate shallow file in update_shallow, we write it > into the lock file. The string stored in alternate_shallow_file is > copied from the lock file path, but it is freed the moment that the lock > file is closed, since we call strbuf_release to free that path. > > This used to work, since we did not invoke git index-pack more than > once, but now that we do, we reuse the freed memory. Ensure we reset the > value to NULL to avoid using freed memory. git index-pack will read the > repository's shallow file, which will have been updated with the correct > information. It may be worth mentioning bd0b42aed3 (fetch-pack: do not take shallow lock unnecessarily - 2019-01-10). I believe this is the same problem and a full solution was suggested but not implemented in that commit. The problem with dangling alternate_shallow_file is also from that commit. When line_received is false at the end of receive_shallow_info(), we should clear alternate_shallow_file. I'm still debating myself whether we should clear alternate_shallow_file in receive_shallow_info() in addition to your changes (which is good hygiene anyway) to keep the setup steps of do_fetch_pack() and do_fetch_pack_v2() aligned. pack protocol v1 does this clearing alternate_shallow_file (near the end of do_fetch_pack()) so it will not have the same dangling pointer problem even if we do two fetches in the same process. But from the Tan's commit description, I thought v1 probably faces the same "lock twice not supported". But luckily it's never triggered in practice. When backfill_tags() is called, if I read it correctly, we either disable shallow (TRANS_OPT_DEPTH set to zero) or we start a second process. In either case, the lock will not be acquired twice by the same process. > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > --- > fetch-pack.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fetch-pack.c b/fetch-pack.c > index 577faa6229..2d76287674 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 = NULL; > 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 = NULL; > } > 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 = NULL; > return; > } >
> On Mon, Feb 4, 2019 at 7:06 AM brian m. carlson > <sandals@crustytoothpaste.net> wrote: > > > > When we write an alternate shallow file in update_shallow, we write it > > into the lock file. The string stored in alternate_shallow_file is > > copied from the lock file path, but it is freed the moment that the lock > > file is closed, since we call strbuf_release to free that path. > > > > This used to work, since we did not invoke git index-pack more than > > once, but now that we do, we reuse the freed memory. Ensure we reset the > > value to NULL to avoid using freed memory. git index-pack will read the > > repository's shallow file, which will have been updated with the correct > > information. > > It may be worth mentioning bd0b42aed3 (fetch-pack: do not take shallow > lock unnecessarily - 2019-01-10). I believe this is the same problem > and a full solution was suggested but not implemented in that commit. For reference, the full solution is [1], linked from that commit's email [2]. (Looking back, I probably should have included all the information below the "---" in the commit message proper.) The full solution is more related to shallow locks, though, not alternate_shallow_file. [1] https://public-inbox.org/git/20181218010811.143608-1-jonathantanmy@google.com/ [2] https://public-inbox.org/git/20190110193645.34080-1-jonathantanmy@google.com/ > The problem with dangling alternate_shallow_file is also from that > commit. You're right - thanks for noticing this. > When line_received is false at the end of > receive_shallow_info(), we should clear alternate_shallow_file. I'm > still debating myself whether we should clear alternate_shallow_file > in receive_shallow_info() in addition to your changes (which is good > hygiene anyway) to keep the setup steps of do_fetch_pack() and > do_fetch_pack_v2() aligned. Clearing alternate_shallow_file when line_received is false at the end of receive_shallow_info() sounds like a good idea to me.
On Tue, Feb 05, 2019 at 08:26:36AM -0800, Jonathan Tan wrote: > > It may be worth mentioning bd0b42aed3 (fetch-pack: do not take shallow > > lock unnecessarily - 2019-01-10). I believe this is the same problem > > and a full solution was suggested but not implemented in that commit. > > For reference, the full solution is [1], linked from that commit's email > [2]. (Looking back, I probably should have included all the information > below the "---" in the commit message proper.) The full solution is more > related to shallow locks, though, not alternate_shallow_file. > > [1] https://public-inbox.org/git/20181218010811.143608-1-jonathantanmy@google.com/ > [2] https://public-inbox.org/git/20190110193645.34080-1-jonathantanmy@google.com/ > > > The problem with dangling alternate_shallow_file is also from that > > commit. > > You're right - thanks for noticing this. > > > When line_received is false at the end of > > receive_shallow_info(), we should clear alternate_shallow_file. I'm > > still debating myself whether we should clear alternate_shallow_file > > in receive_shallow_info() in addition to your changes (which is good > > hygiene anyway) to keep the setup steps of do_fetch_pack() and > > do_fetch_pack_v2() aligned. > > Clearing alternate_shallow_file when line_received is false at the end > of receive_shallow_info() sounds like a good idea to me. I'll reroll with that change and a commit message update. I think it's important that we get this issue fixed for the release, and then we can choose to implement a more comprehensive solution later.
diff --git a/fetch-pack.c b/fetch-pack.c index 577faa6229..2d76287674 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 = NULL; 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 = NULL; } 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 = NULL; return; }
When we write an alternate shallow file in update_shallow, we write it into the lock file. The string stored in alternate_shallow_file is copied from the lock file path, but it is freed the moment that the lock file is closed, since we call strbuf_release to free that path. This used to work, since we did not invoke git index-pack more than once, but now that we do, we reuse the freed memory. Ensure we reset the value to NULL to avoid using freed memory. git index-pack will read the repository's shallow file, which will have been updated with the correct information. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- fetch-pack.c | 3 +++ 1 file changed, 3 insertions(+)