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

["René Scharfe" <l.s.r@web.de>]

Am 03.04.24 um 03:18 schrieb Jeff King:
> 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.

Getting only a truncated prefix is hopefully detected as invalid, but
explicit handling would probably be better.

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

mkpath() would be perfect but unusable in multiple threads.  Cleaning
up after mkpathdup() is a bit iffy in that caller.  strbuf would be a
bit much in that error path, I think, and you might have to export or
reimplement strbuf_cleanup_path().

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

For warnings and usage messages this would keep the prefix and not
die.  This would look a bit strange, no?

	usage: could not format error: TERRIBLE MESSAGE!

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

Yes, showing errno doesn't add that much value.  Except in this case it
shows that there's something going on that I don't understand.  Dare I
dig deeper?  Probably not..

René