don't report vsnprintf(3) error as bug

Commit Message

René Scharfe April 21, 2024, 12:40 p.m. UTC

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.

Suggested-by: Jeff King <[e-mail]>
Signed-off-by: René Scharfe <[e-mail]>
---
 mem-pool.c | 3 ++-
 strbuf.c   | 4 ++--
 wrapper.c  | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

--
2.44.0

Comments

Junio C Hamano April 21, 2024, 7:26 p.m. UTC | #1

René Scharfe <[e-mail]> 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.

Thanks, will queue.

Jeff King April 23, 2024, 10:26 p.m. UTC | #2

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

> René Scharfe <[e-mail]> 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

Patch

diff --git a/mem-pool.c b/mem-pool.c
index 3065b12b23..a3ba38831d 100644
--- a/mem-pool.c
+++ b/mem-pool.c
@@ -4,6 +4,7 @@ 

 #include "git-compat-util.h"
 #include "mem-pool.h"
+#include "gettext.h"

 #define BLOCK_GROWTH_SIZE (1024 * 1024 - sizeof(struct mp_block))

@@ -122,7 +123,7 @@  static char *mem_pool_strvfmt(struct mem_pool *pool, const char *fmt,
 	len = vsnprintf(next_free, available, fmt, cp);
 	va_end(cp);
 	if (len < 0)
-		BUG("your vsnprintf is broken (returned %d)", len);
+		die(_("unable to format message: %s"), fmt);

 	size = st_add(len, 1); /* 1 for NUL */
 	ret = mem_pool_alloc(pool, size);
diff --git a/strbuf.c b/strbuf.c
index 1492a08225..0d929e4e19 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -277,7 +277,7 @@  void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt, va_list ap)
 	len = vsnprintf(sb->buf + sb->len, 0, fmt, cp);
 	va_end(cp);
 	if (len < 0)
-		BUG("your vsnprintf is broken (returned %d)", len);
+		die(_("unable to format message: %s"), fmt);
 	if (!len)
 		return; /* nothing to do */
 	if (unsigned_add_overflows(sb->len, len))
@@ -404,7 +404,7 @@  void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
 	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, cp);
 	va_end(cp);
 	if (len < 0)
-		BUG("your vsnprintf is broken (returned %d)", len);
+		die(_("unable to format message: %s"), fmt);
 	if (len > strbuf_avail(sb)) {
 		strbuf_grow(sb, len);
 		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
diff --git a/wrapper.c b/wrapper.c
index eeac3741cf..f87d90bf57 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -670,7 +670,7 @@  int xsnprintf(char *dst, size_t max, const char *fmt, ...)
 	va_end(ap);

 	if (len < 0)
-		BUG("your snprintf is broken");
+		die(_("unable to format message: %s"), fmt);
 	if (len >= max)
 		BUG("attempt to snprintf into too-small buffer");
 	return len;