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

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

Am 01.04.24 um 05:36 schrieb Jeff King:
> On Sun, Mar 31, 2024 at 08:53:07PM +0200, René Scharfe wrote:
>
> Which, by the way...
>
>> @@ -123,13 +124,14 @@ static char *mem_pool_strvfmt(struct mem_pool *pool, const char *fmt,
>>  	if (len < 0)
>>  		BUG("your vsnprintf is broken (returned %d)", len);
>
> Not new in your patch, and I know this is copied from the strbuf code,
> but I think a BUG() is probably the wrong thing. We added it long ago to
> let us know about broken vsnprintf() implementations, but we'd have
> flushed those out by now, as nothing in Git would work on such a
> platform.
>
> And meanwhile there are legitimate reasons for a non-broken vsnprintf()
> to return -1: namely that it is the only useful thing they can do when
> the requested string is larger than INT_MAX (e.g., "%s" on a string that
> is over 2GB). This is sort of academic, of course. There's no useful
> error to return here, and anybody who manages to shove 2GB into a place
> where we expect a short string fully deserves to have their program
> abort.
>
> I don't have a good example of where you can trigger this (it used to be
> easy with long attribute names, but these days we refuse to parse them).
> But in general probably calling die() is more appropriate.

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

> There's a similar call in vreportf() that tries to keep going, but it
> ends up with lousy results. E.g., try:
>
>   perl -le 'print "create refs/heads/", "a"x2147483648, "HEAD"' |
>   git update-ref --stdin
>
> which results in just "fatal: ", since formatting the error string
> fails. Perhaps we should just print the unexpanded format string
> ("invalid ref format: %s" in this case). It's not great, but it's better
> than nothing.

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);
        }

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

René