| Message ID | 5db4e17db9b9b991e0dbe56d7043b56e5d161097.1727687410.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 9d4855eef3644ac00d62e08d797ff7db554ca65d |
| Headers | show |
| Series | Memory leak fixes (pt.8) | expand |
On Mon, Sep 30, 2024 at 11:14:01AM +0200, Patrick Steinhardt wrote: > The buffer used to compute the final MIDX name is never released. Plug > this memory leak. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > midx-write.c | 2 ++ > t/t5334-incremental-multi-pack-index.sh | 2 ++ > 2 files changed, 4 insertions(+) > > diff --git a/midx-write.c b/midx-write.c > index 1ef62c4f4b..625c7d3137 100644 > --- a/midx-write.c > +++ b/midx-write.c > @@ -1445,6 +1445,8 @@ static int write_midx_internal(const char *object_dir, > return -1; Would you also want to strbuf_release() the final_midx_name buffer here as well? I guess the point is moot since there are a number of other finalization steps that we likewise skip here by returning -1 directly instead of jumping to the cleanup sub-routine. In the above sense I'm OK with it as-is, but it would be nice to verify that this portion of the code is leak-free even when we return early (e.g., because we couldn't rename() the tempfile into place). Of course, because final_midx_name is local to the body of this conditional, I think you'd need to do something like: if (ctx.incremntal) { struct strbuf final_midx_name = STRBUF_INIT; /* ... */ get_split_midx_filename_ext(&final_midx_name, object_dir, midx_hash, MIDX_EXT_MIDX); if (rename_tempfile(&incr, final_midx_name.buf) < 0) { result = error_errno(_("unable to rename new multi-pack-index layer")); strbuf_release(&final_midx_name); /* <- here */ goto cleanup; } strbuf_release(&final_midx_name); /* ... <- and here */ keep_hashes[ctx.num_multi_pack_indexes_before] = xstrdup(hash_to_hex(midx_hash)); } Thanks, Taylor
On Mon, Sep 30, 2024 at 05:27:09PM -0400, Taylor Blau wrote: > On Mon, Sep 30, 2024 at 11:14:01AM +0200, Patrick Steinhardt wrote: > > The buffer used to compute the final MIDX name is never released. Plug > > this memory leak. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > midx-write.c | 2 ++ > > t/t5334-incremental-multi-pack-index.sh | 2 ++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/midx-write.c b/midx-write.c > > index 1ef62c4f4b..625c7d3137 100644 > > --- a/midx-write.c > > +++ b/midx-write.c > > @@ -1445,6 +1445,8 @@ static int write_midx_internal(const char *object_dir, > > return -1; > > Would you also want to strbuf_release() the final_midx_name buffer here > as well? > > I guess the point is moot since there are a number of other finalization > steps that we likewise skip here by returning -1 directly instead of > jumping to the cleanup sub-routine. > > In the above sense I'm OK with it as-is, but it would be nice to verify > that this portion of the code is leak-free even when we return early > (e.g., because we couldn't rename() the tempfile into place). > > Of course, because final_midx_name is local to the body of this > conditional, I think you'd need to do something like: > > if (ctx.incremntal) { > struct strbuf final_midx_name = STRBUF_INIT; > > /* ... */ > > get_split_midx_filename_ext(&final_midx_name, object_dir, > midx_hash, MIDX_EXT_MIDX); > > if (rename_tempfile(&incr, final_midx_name.buf) < 0) { > result = error_errno(_("unable to rename new multi-pack-index layer")); > strbuf_release(&final_midx_name); /* <- here */ > goto cleanup; > } > > strbuf_release(&final_midx_name); /* ... <- and here */ > > keep_hashes[ctx.num_multi_pack_indexes_before] = > xstrdup(hash_to_hex(midx_hash)); > } Potentially. Until now we never really cared all that much about releasing resources in error paths, and for all I can see this leak isn't hit in our test suite. But it is an easy fix to make, so I can include this in case I'll have to reroll this series due to other reasons. Patrick
diff --git a/midx-write.c b/midx-write.c index 1ef62c4f4b..625c7d3137 100644 --- a/midx-write.c +++ b/midx-write.c @@ -1445,6 +1445,8 @@ static int write_midx_internal(const char *object_dir, return -1; } + strbuf_release(&final_midx_name); + keep_hashes[ctx.num_multi_pack_indexes_before] = xstrdup(hash_to_hex(midx_hash)); diff --git a/t/t5334-incremental-multi-pack-index.sh b/t/t5334-incremental-multi-pack-index.sh index c3b08acc73..471994c4bc 100755 --- a/t/t5334-incremental-multi-pack-index.sh +++ b/t/t5334-incremental-multi-pack-index.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='incremental multi-pack-index' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-midx.sh
The buffer used to compute the final MIDX name is never released. Plug this memory leak. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- midx-write.c | 2 ++ t/t5334-incremental-multi-pack-index.sh | 2 ++ 2 files changed, 4 insertions(+)