On Sat, May 25, 2024 at 12:46:35AM -0400, Jeff King wrote: > On Fri, May 24, 2024 at 12:03:29PM +0200, Patrick Steinhardt wrote: > > > In `strbuf_appendwholeline()` we call `strbuf_getwholeline()` with a > > temporary buffer. In case the call returns an error we indicate this by > > returning EOF, but never release the temporary buffer. This can lead to > > a memory leak when the line has been partially filled. Fix this. > > Hmm, doesn't this indicate a bug in getwholeline()? Most strbuf > functions on error try to leave the allocation as-is. > > At the end of the getdelim() version (which is probably what you're > running), when we see an error we do: > > if (!sb->buf) > strbuf_init(sb, 0); > else > strbuf_reset(sb); > return EOF; > > So if getdelim() returned error and left us with a buffer (but still > returned -1 for error!), I think this code is assuming that the buffer > it left us with was the same one that existed beforehand. > > But your commit message implies that it might allocate, hit an error, > and then return that error along with an allocated buffer? Looks like > that matches the getdelim() manpage, which says: > > If *lineptr was set to NULL before the call, then the buffer should be > freed by the user program even on failure. > > So should we do something like: > > diff --git a/strbuf.c b/strbuf.c > index e1076c9891..e37165812b 100644 > --- a/strbuf.c > +++ b/strbuf.c > @@ -659,7 +659,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) > if (!sb->buf) > strbuf_init(sb, 0); > else > - strbuf_reset(sb); > + strbuf_release(sb); > return EOF; > } > #else > > That assumes sb->alloc is valid after a failed call, since > strbuf_release() checks it. But that seems reasonable. If not, we'd need > to free() and re-initialize it ourselves, and the code is more like: > > diff --git a/strbuf.c b/strbuf.c > index e1076c9891..aed699c6bf 100644 > --- a/strbuf.c > +++ b/strbuf.c > @@ -656,10 +656,8 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term) > * we can just re-init, but otherwise we should make sure that our > * length is empty, and that the result is NUL-terminated. > */ > - if (!sb->buf) > - strbuf_init(sb, 0); > - else > - strbuf_reset(sb); > + FREE_AND_NULL(sb->buf); > + strbuf_init(sb, 0); > return EOF; > } > #else > > But I think either of those would solve your leak, _and_ would help with > similar leaks of strbuf_getwholeline() and friends. I'm not quite convinced that `strbuf_getwholeline()` should deallocate the buffer for the caller, I think that makes for quite a confusing calling convention. The caller may want to reuse the buffer for other operations, and it feels hostile to release the buffer under their feet. The only edge case where I think it would make sense to free allocated data is when being passed a not-yet-allocated strbuf. But I wonder whether the added complexity would be worth it. I've been going through all callsites and couldn't spot any that doesn't free the buffer on EOF. So I'd propose to leave this as-is and revisit if we eventually see that this is causing more memory leaks. Patrick
Attachment:
signature.asc
Description: PGP signature
