Re: [PATCH v6 1/6] object-file.c: release strbuf in write_loose_object()
- From
René Scharfe <l.s.r@web.de>
- Date
- Dec 17, 2021, 19:28 UTC
- Message-ID
- <c860c56f-ce25-4391-7f65-50c9d5d80c2c@web.de>
- In-Reply-To
- <20211217112629.12334-2-chiyutianyi@gmail.com>
Am 17.12.21 um 12:26 schrieb Han Xin:
Show 16 quoted lines
> From: Han Xin <hanxin.hx@alibaba-inc.com> > > Fix a strbuf leak in "write_loose_object()" sugguested by > Ævar Arnfjörð Bjarmason. > > Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > Signed-off-by: Han Xin <hanxin.hx@alibaba-inc.com> > --- > 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);
Show 26 quoted lines
> 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)