Re: [PATCH] don't report vsnprintf(3) error as bug

From: Jeff King <peff@peff.net>
Date: Apr 23, 2024, 22:26 UTC
Message-ID: <20240423222652.GD1172807@coredump.intra.peff.net>
In-Reply-To: <xmqqa5lm1pr5.fsf@gitster.g>

On Sun, Apr 21, 2024 at 12:26:22PM -0700, Junio C Hamano wrote:

Show 25 quoted lines

> René Scharfe <l.s.r@web.de> writes:
> 
> > strbuf_addf() has been reporting a negative return value of vsnprintf(3)
> > as a bug since f141bd804d (Handle broken vsnprintf implementations in
> > strbuf, 2007-11-13).  Other functions copied that behavior:
> >
> > 7b03c89ebd (add xsnprintf helper function, 2015-09-24)
> > 5ef264dbdb (strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`, 2019-02-25)
> > 8d25663d70 (mem-pool: add mem_pool_strfmt(), 2024-02-25)
> >
> > However, vsnprintf(3) can legitimately return a negative value if the
> > formatted output would be longer than INT_MAX.  Stop accusing it of
> > being broken and just report the fact that formatting failed.
> 
> """ ... function returns the number of characters that would have
> been written had n been sufficiently large, not counting the
> terminating null character, or a negative value if an encoding error
> occurred. Thus, the null-terminated output has been completely
> written if and only if the returned value is nonnegative and less
> than n.""" is what I read in some versions of ISO/IEC 9899.  It is
> curious that it does not say anything about the consequence of a
> parameter error arising from int (the type snprintf family of
> functions returns) being narrower than size_t (the type of the
> parameter n), but your point still stands that vsnprintf() can
> legitimately fail, and it is not a programming error.

POSIX does say:

       The snprintf() function shall fail if:

       EOVERFLOW
              The value of n is greater than {INT_MAX}.

But mostly the INT_MAX thing is simply the one thing we've seen in practice. I wouldn't be surprised if there are other conditions that can trigger an error return from vsnprintf. E.g., POSIX says:

  If a conversion specification does not match one of the above forms,
  the behavior is undefined.

Of course "undefined" is much worse than returning -1, but it seems like a reasonable thing for an implementation to choose to do (either that or just output the character literally).

We should be immune-ish to such an issue by virtue of -Wformat (we're only allowed to pass literal strings, and they must all be understood by the compiler). Of course that's platform-specific and we only check -Werror on some platforms. So gcc allows "%b", but it may be meaningless on AIX, and who knows what their libc will do. ;)

That case kind of _is_ a BUG() situation. But I don't think it's worth trying to differentiate that. Switching all of these to die() makes the most sense (i.e., I like René's patch).

-Peff