From: Jeff King <peff@peff.net>
Date: Sat, 21 Mar 2026 21:18:28 GMT
Subject: Re: [RFC] cocci: .buf in a strbuf object can never be NULL
Message-ID: <20260321211828.GB736981@coredump.intra.peff.net>
In-Reply-To: <3e387439-c066-4e45-b28b-43f77c8824d6@web.de>

On Sat, Mar 21, 2026 at 09:47:18PM +0100, René Scharfe wrote:

> And yet this function can turn an empty strbuf into an allocated one
> without rolling it back on error, leaving code similar to this silly
> example here leaking:
> 
> 	int copy_one_line(FILE *in, FILE *out, int term)
> 	{
> 		struct strbuf sb = STRBUF_INIT;
> 		if (strbuf_getwholeline(&sb, in, term))
> 			return -1;
> 		fwrite(sb.buf, 1, sb.len, out);
> 		strbuf_release(&sb);
> 		return 0;
> 	}

Yes, I almost pointed that out, but I think it's mostly a non-issue
in practice because you'd generally call it multiple times (usually in a
loop, but sometimes just multiple individual calls). And then you have
to release if any call ever succeeded, which means either doing so after
the loop ends or in a cleanup block.

Grepping for 'if (strbuf_get.*line', the closest I found was
get_mail_commit_oid(), which reads a single line. It doesn't have an
early return, though, since it has to clean up the FILE pointer anyway.

So I dunno. I don't think it's been a problem in practice, but I'm not
opposed to future-proofing if it's easy to do.

> Some strbuf functions restore the original state in such a case by
> calling strbuf_release(), strbuf_getwholeline() doesn't.  If we are OK
> with that then it could be simplified by growing the buffer upfront:
> 
> 	int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
> 	{
> 		ssize_t r;
> 
> 		strbuf_grow(sb, 0);
> 		errno = 0;
> 		r = getdelim(&sb->buf, &sb->alloc, term, fp);

This causes two allocations, but presumably only the first call of many,
so not a big deal in practice.

I feel like there's a lot of discussion in this thread but we're not
achieving anything practical. If we do anything, I think it would be:

  - drop the feof and reset at the top of the function, which are
    redundant

  - make a noop read on an unallocated strbuf retain the unallocated
    state (your example above)

Could the function be rewritten differently, or maybe even made a little
simpler? Perhaps, but who cares? The function has been largely untouched
for a decade and the behavior is fine. And there are a bunch of pitfalls
that a rewrite risks falling into.

-Peff