Re: [RFC] cocci: .buf in a strbuf object can never be NULL
- From
Jeff King <peff@peff.net>
- Date
- Mar 21, 2026, 21:18 UTC
- 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:
Show 13 quoted lines
> 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.
Show 11 quoted lines
> 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