| Message ID | 20240924215225.GF1143820@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c8009635785e656cfed3b814beb4eab9fd16eac3 |
| Headers | show |
| Series | leak fixes for http fetch/push | expand |
On Tue, Sep 24, 2024 at 05:52:25PM -0400, Jeff King wrote: > When we call get_remote_heads() for protocol v0, that may populate the > "shallow" oid_array, which must be cleaned up to avoid a leak at the > program exit. The same problem exists for both fetch-pack and send-pack, > but not for the usual transport.c code paths, since we already do this > cleanup in disconnect_git(). > > Fixing this lets us mark t5542 as leak-free for the send-pack side, but > fetch-pack will need some more fixes before we can do the same for > t5539. > > Signed-off-by: Jeff King <peff@peff.net> > --- > builtin/fetch-pack.c | 1 + > builtin/send-pack.c | 1 + > t/t5542-push-http-shallow.sh | 1 + > 3 files changed, 3 insertions(+) > > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c > index cfc6951d23..ef4143eef3 100644 > --- a/builtin/fetch-pack.c > +++ b/builtin/fetch-pack.c > @@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc, > free_refs(fetched_refs); > free_refs(remote_refs); > list_objects_filter_release(&args.filter_options); > + oid_array_clear(&shallow); > return ret; > } I wonder about the early exit path we have when `finish_connect()` returns non-zero. Should we make it go via the common cleanup path as well, or do we not care in the error case? > diff --git a/builtin/send-pack.c b/builtin/send-pack.c > index 81fc96d423..c49fe6c53c 100644 > --- a/builtin/send-pack.c > +++ b/builtin/send-pack.c > @@ -343,5 +343,6 @@ int cmd_send_pack(int argc, > free_refs(remote_refs); > free_refs(local_refs); > refspec_clear(&rs); > + oid_array_clear(&shallow); > return ret; > } We also have an early exit in this function when `match_push_refs()` returns non-zero. Patrick
On Thu, Sep 26, 2024 at 03:50:02PM +0200, Patrick Steinhardt wrote: > > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c > > index cfc6951d23..ef4143eef3 100644 > > --- a/builtin/fetch-pack.c > > +++ b/builtin/fetch-pack.c > > @@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc, > > free_refs(fetched_refs); > > free_refs(remote_refs); > > list_objects_filter_release(&args.filter_options); > > + oid_array_clear(&shallow); > > return ret; > > } > > I wonder about the early exit path we have when `finish_connect()` > returns non-zero. Should we make it go via the common cleanup path as > well, or do we not care in the error case? Yeah, I think it would technically be a leak if we hit that error case. But it's pretty unlikely in practice (you'd have to finish the fetch and then the "ssh" or "upload-pack" sub-process returns a non-zero exit code anyway). I'd prefer not to deal with it here for two reasons: 1. The same leak is true of all the existing cleanup. So it really is orthogonal to what this patch is doing, and should be a separate patch. 2. I think cleaning up in top-level builtins like this is mostly cosmetic to appease the leak checker. In the real world the memory is going back to the OS when we return anyway. Even in a libified world, I don't think cmd_fetch_pack() is a reasonable entry point (we already have a more lib-ish fetch_pack() that is used by the transport code). So I don't think it would be wrong to refactor the cleanup to trigger on that other early return. But I also suspect there are many such paths lurking (and will continue to lurk, even after we run clean with SANITIZE=leak) and I'm not sure how productive it is to hunt them down. > > diff --git a/builtin/send-pack.c b/builtin/send-pack.c > > index 81fc96d423..c49fe6c53c 100644 > > --- a/builtin/send-pack.c > > +++ b/builtin/send-pack.c > > @@ -343,5 +343,6 @@ int cmd_send_pack(int argc, > > free_refs(remote_refs); > > free_refs(local_refs); > > refspec_clear(&rs); > > + oid_array_clear(&shallow); > > return ret; > > } > > We also have an early exit in this function when `match_push_refs()` > returns non-zero. And I think this is in the same boat. -Peff
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c index cfc6951d23..ef4143eef3 100644 --- a/builtin/fetch-pack.c +++ b/builtin/fetch-pack.c @@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc, free_refs(fetched_refs); free_refs(remote_refs); list_objects_filter_release(&args.filter_options); + oid_array_clear(&shallow); return ret; } diff --git a/builtin/send-pack.c b/builtin/send-pack.c index 81fc96d423..c49fe6c53c 100644 --- a/builtin/send-pack.c +++ b/builtin/send-pack.c @@ -343,5 +343,6 @@ int cmd_send_pack(int argc, free_refs(remote_refs); free_refs(local_refs); refspec_clear(&rs); + oid_array_clear(&shallow); return ret; } diff --git a/t/t5542-push-http-shallow.sh b/t/t5542-push-http-shallow.sh index c2cc83182f..07624a1d7f 100755 --- a/t/t5542-push-http-shallow.sh +++ b/t/t5542-push-http-shallow.sh @@ -5,6 +5,7 @@ test_description='push from/to a shallow clone over http' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-httpd.sh start_httpd
When we call get_remote_heads() for protocol v0, that may populate the "shallow" oid_array, which must be cleaned up to avoid a leak at the program exit. The same problem exists for both fetch-pack and send-pack, but not for the usual transport.c code paths, since we already do this cleanup in disconnect_git(). Fixing this lets us mark t5542 as leak-free for the send-pack side, but fetch-pack will need some more fixes before we can do the same for t5539. Signed-off-by: Jeff King <peff@peff.net> --- builtin/fetch-pack.c | 1 + builtin/send-pack.c | 1 + t/t5542-push-http-shallow.sh | 1 + 3 files changed, 3 insertions(+)