* [PATCH] don't report vsnprintf(3) error as bug
@ 2024-04-21 12:40 René Scharfe
2024-04-21 19:26 ` Junio C Hamano
0 siblings, 1 reply; 3+ messages in thread
From: René Scharfe @ 2024-04-21 12:40 UTC (permalink / raw)
To: Git List; +Cc: Jeff King
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 <peff@peff.net>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
mem-pool.c | 3 ++-
strbuf.c | 4 ++--
wrapper.c | 2 +-
3 files changed, 5 insertions(+), 4 deletions(-)
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;
--
2.44.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] don't report vsnprintf(3) error as bug
2024-04-21 12:40 [PATCH] don't report vsnprintf(3) error as bug René Scharfe
@ 2024-04-21 19:26 ` Junio C Hamano
2024-04-23 22:26 ` Jeff King
0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2024-04-21 19:26 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King
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.
Thanks, will queue.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] don't report vsnprintf(3) error as bug
2024-04-21 19:26 ` Junio C Hamano
@ 2024-04-23 22:26 ` Jeff King
0 siblings, 0 replies; 3+ messages in thread
From: Jeff King @ 2024-04-23 22:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: René Scharfe, Git List
On Sun, Apr 21, 2024 at 12:26:22PM -0700, Junio C Hamano wrote:
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-23 22:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-21 12:40 [PATCH] don't report vsnprintf(3) error as bug René Scharfe
2024-04-21 19:26 ` Junio C Hamano
2024-04-23 22:26 ` Jeff King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).