Re: [PATCH v6 1/6] object-file.c: release strbuf in write_loose_object()
From: René Scharfe <hidden>
Date: 2021-12-17 19:30:28
Am 17.12.21 um 12:26 schrieb Han Xin:
quoted hunk ↗ jump to hunk
From: Han Xin <redacted> Fix a strbuf leak in "write_loose_object()" sugguested by Ævar Arnfjörð Bjarmason. Helped-by: Ævar Arnfjörð Bjarmason [off-list ref] Signed-off-by: Han Xin <redacted> --- object-file.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)diff --git a/object-file.c b/object-file.c index eb1426f98c..32acf1dad6 100644 --- a/object-file.c +++ b/object-file.c@@ -1874,11 +1874,14 @@ static int write_loose_object(const struct object_id *oid, char *hdr,
Relevant context lines: static struct strbuf tmp_file = STRBUF_INIT; static struct strbuf filename = STRBUF_INIT; loose_object_path(the_repository, &filename, oid);
quoted hunk ↗ jump to hunk
fd = create_tmpfile(&tmp_file, filename.buf); if (fd < 0) { if (flags & HASH_SILENT) - return -1; + ret = -1; else if (errno == EACCES) - return error(_("insufficient permission for adding an object to repository database %s"), get_object_directory()); + ret = error(_("insufficient permission for adding an " + "object to repository database %s"), + get_object_directory()); else - return error_errno(_("unable to create temporary file")); + ret = error_errno(_("unable to create temporary file")); + goto cleanup; } /* Set it up */@@ -1930,7 +1933,11 @@ static int write_loose_object(const struct object_id *oid, char *hdr, warning_errno(_("failed utime() on %s"), tmp_file.buf); } - return finalize_object_file(tmp_file.buf, filename.buf); + ret = finalize_object_file(tmp_file.buf, filename.buf); +cleanup: + strbuf_release(&filename); + strbuf_release(&tmp_file);
There was no leak before. Both strbufs are static and both functions they are passed to (loose_object_path() and create_tmpfile()) reset them first. So while the allocated memory was not released before, it was reused. Not sure if making write_loose_object() allocate and release these buffers on every call has much of a performance impact. The only reason I can think of for wanting such a change is to get rid of the static buffers, to allow the function to be used by concurrent threads. So I think either keeping the code as-is or also making the strbufs non-static would be better (but then discussing a possible performance impact in the commit message would be nice).
+ return ret; } static int freshen_loose_object(const struct object_id *oid)