Hello Theo,

Theo Buehler wrote on Wed, May 21, 2025 at 02:13:16AM +0200:
> On Tue, May 20, 2025 at 06:54:40PM +0200, Ingo Schwarze wrote:
>> 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.

That's also an option.

To clarify, i'm not objecting to florian@'s diff, i'm merely
suggesting an alternative that seems clearer to me.

>> 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

I think that would be inverted.

> 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.

In complicated cases where it's not clear whether an error check
is needed, you may be right that erring towards treating stuff
as error checks, and using assert(3) only when the invariant is
certain, is the more cautious option.

But in this case, the whole point of the code is to first
calculate the required length, allocate the buffer large enough
to fit, and then use it.  Should the snprintf(3) return value
check ever trigger, that would be *incorrect behaviour*
because the watch(1) utility is not supposed to ever fail
due to the length of the command line.

I think florian@ should take tedu@'s malloc(3) suggestion,
resolve the conflict, and (given that his patch works for me),
if he feels confident, proceed, optionally using those of our
suggestions he likes, or, if he wants to make extra sure,
send his preferred final patch (that does not conflict)
for a definitive OK.

Yours,
  Ingo