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: Wed, 3 Apr 2024 12:01:13 +0200 [thread overview]
Message-ID: <b48fd3ee-2975-481f-aa0e-8ec4d07ea705@web.de> (raw)
In-Reply-To: <20240403011818.GB892394@coredump.intra.peff.net>
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é
next prev parent reply other threads:[~2024-04-03 10:09 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
2024-04-03 1:18 ` Jeff King
2024-04-03 10:01 ` René Scharfe [this message]
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=b48fd3ee-2975-481f-aa0e-8ec4d07ea705@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).