On Tue, May 20, 2025 at 06:54:40PM +0200, Ingo Schwarze wrote:
> Hello Florian,
> 
> Florian Obser wrote on Tue, May 20, 2025 at 02:38:57PM +0200:
> 
> > +		if ((size_t)len >= rest)
> > +			errx(1, "overflow");
> 
> Conditionals that are designed to never be true are not nice.
> They make code auditing harder because the intent is confusing
> and the auditor is likely to wonder whether the author was aware
> that the code is unreachable.

I think it would look less strange if it used the canonical error check
that the snprintf() manual tells us to use in the CAVEATS.

I'd probably also be inclined to dedup the two snprintf() calls with
"%s%s" using an auxiliary const char *sep or a ternary operator.

> Since we are not in a library, i'd prefer
> 
> 	assert((size_t)len < rest);

I'd much prefer an assertion for len < 0 || (size_t)len >= rest but I'm
not fully convinced by your unreachability argument. Mechanically error
checking everything with idiomatic checks is perfectly fine in my book.
assert() should not be used for error checking either and it's not
always that clear what's an error check and what's an assertion of an
invariant.