| Message ID | 4c5740afe43c3bb619a9cba0c1634097c27ed33f.1724315484.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7720460ccf8d5a8fcc5cf37bad97b26d799a5644 |
| Headers | show |
| Series | Memory leak fixes (pt.5) | expand |
Patrick Steinhardt <ps@pks.im> writes: > In `fetch_refs_from_bundle()` we assemble a vector of arguments to pass > to `unbundle()`, but never free it. And in theory we wouldn't have to > because `unbundle()` already knows to free the vector for us. But it > fails to do so when it exits early due to `verify_bundle()` failing. > > The calling convention that the arguments are freed by the callee and > not the caller feels somewhat weird. Refactor the code such that it is > instead the responsibility of the caller to free the vector, adapting > the only two callsites where we pass extra arguments. This also fixes > the memory leak. > > This memory leak gets hit in t5510, but fixing it isn't sufficient to > make the whole test suite pass. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > builtin/bundle.c | 2 ++ > bundle.c | 4 +--- > transport.c | 2 ++ > 3 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/builtin/bundle.c b/builtin/bundle.c > index d5d41a8f672..df97f399019 100644 > --- a/builtin/bundle.c > +++ b/builtin/bundle.c > @@ -220,7 +220,9 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) > &extra_index_pack_args, 0) || > list_bundle_refs(&header, argc, argv); > bundle_header_release(&header); > + > cleanup: > + strvec_clear(&extra_index_pack_args); > free(bundle_file); > return ret; > } > diff --git a/bundle.c b/bundle.c > index ce164c37bc8..0f6c7a71ef1 100644 > --- a/bundle.c > +++ b/bundle.c > @@ -639,10 +639,8 @@ int unbundle(struct repository *r, struct bundle_header *header, > if (flags & VERIFY_BUNDLE_FSCK) > strvec_push(&ip.args, "--fsck-objects"); > > - if (extra_index_pack_args) { > + if (extra_index_pack_args) > strvec_pushv(&ip.args, extra_index_pack_args->v); > - strvec_clear(extra_index_pack_args); > - } > > ip.in = bundle_fd; > ip.no_stdout = 1; > diff --git a/transport.c b/transport.c > index f0672fdc505..da639d3bff0 100644 > --- a/transport.c > +++ b/transport.c > @@ -189,6 +189,8 @@ static int fetch_refs_from_bundle(struct transport *transport, > &extra_index_pack_args, > fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0); > transport->hash_algo = data->header.hash_algo; > + > + strvec_clear(&extra_index_pack_args); > return ret; > } Looks much better. Will queue. Thanks.
diff --git a/builtin/bundle.c b/builtin/bundle.c index d5d41a8f672..df97f399019 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -220,7 +220,9 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) &extra_index_pack_args, 0) || list_bundle_refs(&header, argc, argv); bundle_header_release(&header); + cleanup: + strvec_clear(&extra_index_pack_args); free(bundle_file); return ret; } diff --git a/bundle.c b/bundle.c index ce164c37bc8..0f6c7a71ef1 100644 --- a/bundle.c +++ b/bundle.c @@ -639,10 +639,8 @@ int unbundle(struct repository *r, struct bundle_header *header, if (flags & VERIFY_BUNDLE_FSCK) strvec_push(&ip.args, "--fsck-objects"); - if (extra_index_pack_args) { + if (extra_index_pack_args) strvec_pushv(&ip.args, extra_index_pack_args->v); - strvec_clear(extra_index_pack_args); - } ip.in = bundle_fd; ip.no_stdout = 1; diff --git a/transport.c b/transport.c index f0672fdc505..da639d3bff0 100644 --- a/transport.c +++ b/transport.c @@ -189,6 +189,8 @@ static int fetch_refs_from_bundle(struct transport *transport, &extra_index_pack_args, fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0); transport->hash_algo = data->header.hash_algo; + + strvec_clear(&extra_index_pack_args); return ret; }
In `fetch_refs_from_bundle()` we assemble a vector of arguments to pass to `unbundle()`, but never free it. And in theory we wouldn't have to because `unbundle()` already knows to free the vector for us. But it fails to do so when it exits early due to `verify_bundle()` failing. The calling convention that the arguments are freed by the callee and not the caller feels somewhat weird. Refactor the code such that it is instead the responsibility of the caller to free the vector, adapting the only two callsites where we pass extra arguments. This also fixes the memory leak. This memory leak gets hit in t5510, but fixing it isn't sufficient to make the whole test suite pass. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/bundle.c | 2 ++ bundle.c | 4 +--- transport.c | 2 ++ 3 files changed, 5 insertions(+), 3 deletions(-)