| Message ID | b72f7d1ee394291a018061c91222ea17a99c2e53.1724159575.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| 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. Fix this 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> > --- > transport.c | 2 ++ > 1 file changed, 2 insertions(+) > > 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; > } Hmph. unbundle() has this: if (extra_index_pack_args) { strvec_pushv(&ip.args, extra_index_pack_args->v); strvec_clear(extra_index_pack_args); } so while I think this patch would not hurt at all, do we need this patch? The other caller of unbundle() that passes the extra_index_pack_args is cmd_bundle_unbundle() and it does not do anything after it is done.
On Wed, Aug 21, 2024 at 11:07:39AM -0700, Junio C Hamano wrote: > 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. Fix this 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> > > --- > > transport.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > 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; > > } > > Hmph. unbundle() has this: > > if (extra_index_pack_args) { > strvec_pushv(&ip.args, extra_index_pack_args->v); > strvec_clear(extra_index_pack_args); > } > > so while I think this patch would not hurt at all, do we need this > patch? Yes we do, because there is an early return in case `verify_bundle()` fails. I didn't notice that we have it in `unbundle()` though. > The other caller of unbundle() that passes the extra_index_pack_args > is cmd_bundle_unbundle() and it does not do anything after it is > done. I'd argue it's bad practice to have `unbundle()` clear the caller provided args for us and somewhat surprising. While one way to fix this would be to have a common exit path where we always free them. But I'd much rather make it the responsibility of the caller itself to free them. I'll adapt the code in this spirit. Patrick
Patrick Steinhardt <ps@pks.im> writes: > Yes we do, because there is an early return in case `verify_bundle()` > fails. I didn't notice that we have it in `unbundle()` though. Ah, I missed that one. > I'd argue it's bad practice to have `unbundle()` clear the caller > provided args for us and somewhat surprising. I was surprised so it makes it at least two of us. In some call patterns, having the callee _consume_ what the caller fed it may be natural, but not in this code path. Thanks.
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. Fix this 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> --- transport.c | 2 ++ 1 file changed, 2 insertions(+)