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.
-Peff
