From: "René Scharfe" <l.s.r@web.de>
To: Jeff King <peff@peff.net>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()
Date: Tue, 2 Apr 2024 15:48:45 +0200 [thread overview]
Message-ID: <9f26b9f0-f8d7-4988-b6d4-e0446dab30b1@web.de> (raw)
In-Reply-To: <20240401033642.GB2639525@coredump.intra.peff.net>
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é
next prev parent reply other threads:[~2024-04-02 14:03 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-31 18:53 [PATCH] mem-pool: use st_add() in mem_pool_strvfmt() René Scharfe
2024-04-01 3:36 ` Jeff King
2024-04-02 13:48 ` René Scharfe [this message]
2024-04-03 1:18 ` Jeff King
2024-04-03 10:01 ` René Scharfe
2024-04-03 20:48 ` Jeff King
2024-04-03 21:20 ` Jeff King
2024-04-05 13:10 ` René Scharfe
2024-04-05 17:45 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9f26b9f0-f8d7-4988-b6d4-e0446dab30b1@web.de \
--to=l.s.r@web.de \
--cc=git@vger.kernel.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).