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

From: René Scharfe <l.s.r@web.de>
Date: Apr 2, 2024, 13:48 UTC
Message-ID: <9f26b9f0-f8d7-4988-b6d4-e0446dab30b1@web.de>
In-Reply-To: <20240401033642.GB2639525@coredump.intra.peff.net>

Am 01.04.24 um 05:36 schrieb Jeff King:

Show 25 quoted lines

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

Show 10 quoted lines

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