Re: [RFC] cocci: .buf in a strbuf object can never be NULL
- From
René Scharfe <l.s.r@web.de>
- Date
- Mar 21, 2026, 23:41 UTC
- Message-ID
- <ca9fa6c7-f693-4b85-a17f-8deeb05b45f7@web.de>
- In-Reply-To
- <20260321211828.GB736981@coredump.intra.peff.net>
On 3/21/26 10:18 PM, Jeff King wrote:
Show 25 quoted lines
> 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.Caller strbuf_appendwholeline() handles a single line and invokes strbuf_release() on error, so it swings in the opposite direction.
> 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.
I also don't think it's a problem.
> I feel like there's a lot of discussion in this thread but we're not > achieving anything practical.
Funny how attention works.
> If we do anything, I think it would be: > > - drop the feof and reset at the top of the function, which are > redundant
Easy win. We can also drop the feof(3) call from the non-getdelim(3) version, but need to keep the reset there.
> - make a noop read on an unallocated strbuf retain the unallocated > state (your example above)
That makes the function conform to the convention of rolling back on error. This transactional behavior is a bit easier to understand. The non-getdelim(3) version doesn't do that, though. It returns whatever it got and leaves error checking and rollback to its callers.
getdelim(3) doesn't allow that -- it has no way to indicate the length of partial reads. If we are OK with throwing away partial lines then we better do that consistently in both versions? Sounds a bit messed up to bin perfectly good data just because some other platform has a fancy function that goes quiet when it stumbles. The alternative of having inconsistent behavior seems worse, though.
René