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