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

From: Jeff King <peff@peff.net>
Date: Apr 5, 2024, 17:45 UTC
Message-ID: <20240405174537.GD2529133@coredump.intra.peff.net>
In-Reply-To: <6f727926-a901-4d9f-8a7f-d966f222f15d@web.de>

On Fri, Apr 05, 2024 at 03:10:33PM +0200, René Scharfe wrote:

Show 17 quoted lines

> >>>  	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!).
> 
> Hmm, this might be worth doing to avoid messing up the terminal if
> 'err' points into the weeds for some reason.

I think we have bigger problems if "err" is messed up, because we'll be interpreting garbage as a format string and almost certainly triggering undefined behavior. And in general if we are not using a constant string as the format it's a potential security vulnerability. So I think we can mostly rely on the compile-time -Wformat checks for this.

Show 21 quoted lines

> OK, right, a format error doesn't have to be fatal and we can keep
> running and possibly provide more details.
> 
> But mixing the format error with the actual payload message is not ideal.
> At least we should give the format error its proper prefix, while still
> reporting the prefix of the original message, e.g. like this:
> 
>    error: unable to format message: unable to open loose object %s
>    fatal:
> 
> ... or this:
> 
>    error: unable to format message: fatal: unable to open loose object %s
> 
> I tend to like the first one slightly better, even though the empty
> message looks silly.  It doesn't mix the two and answers the questions
> that I would have:  Why did the program stop?  Due to a fatal error.
> Why is the fatal message silent?  The preceding error explains it.
> 
> While the second one only reveals the fatality somewhere in the middle
> of the text, weakening the meaning of prefixes.

Yeah, I think your first one is good. It's _ugly_, but it's easy to figure out what happened, and this is not something people generally see (and the status quo is just "fatal:", so you're easily 50% less ugly).

> I still don't know why the error code varies between runs, but it
> clearly does not come from vsnprintf(3) -- if I set errno to some
> arbitrary value before the call, it is kept.  Which is enough to
> convince me to ignore errno here.

Agreed. Thanks for digging into it.

-Peff