| Message ID | 20211217112629.12334-2-chiyutianyi@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | unpack large blobs in stream | expand |
Am 17.12.21 um 12:26 schrieb Han Xin: > 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); > 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)
René Scharfe <l.s.r@web.de> writes: > 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). Makes sense.
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, 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); + return ret; } static int freshen_loose_object(const struct object_id *oid)