| Message ID | 5f042ce5098563aa0662026006c356c278dad0b8.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: > When writing the MIDX file we first create the `struct hashfile` used to > write the trailer hash, and then afterwards we verify whether we can > actually write the MIDX in the first place. When we decide that we > can't, this leads to a memory leak because we never free the hash file > contents. > > We could fix this by freeing the hashfile on the exit path. There is a > better option though: we can simply move the checks for the error > condition earlier. As there is no early exit between creating the > hashfile and finalizing it anymore this is sufficient to fix the memory > leak. The above is a good explanation why "are we dropping everything" block was moved up, but does not explain why the other "if there is no objects" block has to move (it is however easy to see that the latter block can be moved without any bad side effect). In any case, the struct hashfile hashfd() gives us is now associated with the struct chunkfile cf immediately after it is instanciated, and there is no early exit while the chunkfile is in use, which is great.
On Tue, Aug 20, 2024 at 04:19:13PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > When writing the MIDX file we first create the `struct hashfile` used to > > write the trailer hash, and then afterwards we verify whether we can > > actually write the MIDX in the first place. When we decide that we > > can't, this leads to a memory leak because we never free the hash file > > contents. > > > > We could fix this by freeing the hashfile on the exit path. There is a > > better option though: we can simply move the checks for the error > > condition earlier. As there is no early exit between creating the > > hashfile and finalizing it anymore this is sufficient to fix the memory > > leak. > > The above is a good explanation why "are we dropping everything" > block was moved up, but does not explain why the other "if there is > no objects" block has to move (it is however easy to see that the > latter block can be moved without any bad side effect). > > In any case, the struct hashfile hashfd() gives us is now associated > with the struct chunkfile cf immediately after it is instanciated, > and there is no early exit while the chunkfile is in use, which is > great. There basically is no reason why the other block needs to move. It just felt more natural to keep massaging of parameters closly together before we go off and actually do "the thing". I'll document this accordingly. Patrick
diff --git a/midx-write.c b/midx-write.c index e3fa33203fa..07d98d494aa 100644 --- a/midx-write.c +++ b/midx-write.c @@ -1308,6 +1308,18 @@ static int write_midx_internal(const char *object_dir, pack_name_concat_len += MIDX_CHUNK_ALIGNMENT - (pack_name_concat_len % MIDX_CHUNK_ALIGNMENT); + if (ctx.nr - dropped_packs == 0) { + error(_("no pack files to index.")); + result = 1; + goto cleanup; + } + + if (!ctx.entries_nr) { + if (flags & MIDX_WRITE_BITMAP) + warning(_("refusing to write multi-pack .bitmap without any objects")); + flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP); + } + if (ctx.incremental) { struct strbuf lock_name = STRBUF_INIT; @@ -1333,18 +1345,6 @@ static int write_midx_internal(const char *object_dir, f = hashfd(get_lock_file_fd(&lk), get_lock_file_path(&lk)); } - if (ctx.nr - dropped_packs == 0) { - error(_("no pack files to index.")); - result = 1; - goto cleanup; - } - - if (!ctx.entries_nr) { - if (flags & MIDX_WRITE_BITMAP) - warning(_("refusing to write multi-pack .bitmap without any objects")); - flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP); - } - cf = init_chunkfile(f); add_chunk(cf, MIDX_CHUNKID_PACKNAMES, pack_name_concat_len,
When writing the MIDX file we first create the `struct hashfile` used to write the trailer hash, and then afterwards we verify whether we can actually write the MIDX in the first place. When we decide that we can't, this leads to a memory leak because we never free the hash file contents. We could fix this by freeing the hashfile on the exit path. There is a better option though: we can simply move the checks for the error condition earlier. As there is no early exit between creating the hashfile and finalizing it anymore this is sufficient to fix the memory leak. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- midx-write.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)