From: René Scharfe <l.s.r@web.de>
Date: Sat, 21 Mar 2026 23:41:04 GMT
Subject: Re: [RFC] cocci: .buf in a strbuf object can never be NULL
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:
> 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é