Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()
Am 03.04.24 um 03:18 schrieb Jeff King:
Show 14 quoted lines
> 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.
Show 11 quoted lines
> 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().
Show 33 quoted lines
>> 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!
Show 28 quoted lines
> 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é
René Scharfe