Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()

> On Tue, Apr 02, 2024 at 03:48:45PM +0200, René Scharfe wrote:
>
>> Makes sense.  Could be rolled into a new wrapper, xvsnprintf();
>> imap-send.c::nfvasprintf() could call it as well.
>>
>> There are also callers of vsnprintf(3) that use its return value without
>> checking for error: builtin/receive-pack.c::report_message(),
>> path.c::mksnpath() and arguably imap-send.c::nfsnprintf().
>
> Hmm, yeah. Those are all OK not to use xsnprintf(), since they handle
> truncation themselves. But the first two don't look like they handle a
> negative return well. In report_message(), we'd end up shrinking "sz".
> That's potentially an out-of-bounds problem, except that we'll always
> have put a non-empty prefix into the buffer.

> For mksnpath(), though, I
> suspect that trying to format a very long name could result in the
> output being full of uninitialized bytes.
>
> It only has one caller, which creates "foo~1" when we got EEXIST from
> "foo". So I doubt you can get up to too much mischief with it. But it
> could easily be replaced by mkpathdup() (or even a reusable strbuf
> outside the loop if you really wanted to hyper-optimize)
>
> And then we could get rid of mksnpath() entirely, and its horrible
> bad_path failure mode.

>> We can throw in errno to distinguish between EILSEQ (invalid wide
>> character) and EOVERFLOW.  And we'd better not call die_errno() to avoid
>> triggering a recursion warning.  We can open-code it instead:
>>
>>         if (vsnprintf(p, pend - p, err, params) < 0) {
>>                 fprintf(stderr, _("%sunable to format message '%s': %s\n"),
>>                         _("fatal: "), err, strerror(errno));
>>                 exit(128);
>>         }
>
> We could also just throw it into the buffer and let the rest of the
> function proceed, like:
>
> diff --git a/usage.c b/usage.c
> index 09f0ed509b..5baab98fa3 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -19,8 +19,10 @@ static void vreportf(const char *prefix, const char *err, va_list params)
>  	}
>  	memcpy(msg, prefix, prefix_len);
>  	p = msg + prefix_len;
> -	if (vsnprintf(p, pend - p, err, params) < 0)
> +	if (vsnprintf(p, pend - p, err, params) < 0) {
> +		if (snprintf(p, pend - p, "could not format error: %s", err) < 0)
>  		*p = '\0'; /* vsnprintf() failed, clip at prefix */
> +	}
>
>  	for (; p != pend - 1 && *p; p++) {
>  		if (iscntrl(*p) && *p != '\t' && *p != '\n')
>
> Though most of the rest of the function is not that useful (it is mostly
> removing unreadable chars, and hopefully we do not have any of those in
> our format strings!).

> I had not thought about showing strerror(). The C
> standard does mention a negative value for encoding errors, but says
> nothing about errno. POSIX does seem to mention EILSEQ and EOVERFLOW.
> So this...
>
>> But when I ran your test command (on macOS 14.4.1) ten times with this
>> change I got:
>>
>> fatal: unable to format message 'invalid ref format: %s': Invalid argument
>> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0
>> fatal: unable to format message 'invalid ref format: %s': Invalid argument
>> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0
>> fatal: unable to format message 'invalid ref format: %s': Invalid argument
>> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0
>> fatal: unable to format message 'invalid ref format: %s': Invalid argument
>> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0
>> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0
>> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0
>>
>> Which scares me.  Why is errno sometimes zero?  Why EINVAL instead of
>> EOVERFLOW?  O_o
>
> ...is just confusing. I do think even without worrying about errno,
> simply saying "I tried to format 'some error: %s' and couldn't" is going
> to be way more useful than just "fatal: ". The only reason it would fail
> is that there's something gross in that "%s". We can't be more specific
> without interpreting the printf-format ourselves (which is probably not
> worth it).