{"thread":{"id":"61348","subject":"[PATCH] don't report vsnprintf(3) error as bug","startedAt":"2024-04-21T12:40:42Z","lastAt":"2024-04-23T22:26:54Z","messageCount":3,"participants":["René Scharfe","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"493264","messageId":"ea752a2b-9b74-4a59-a037-4782abf7161e@web.de","threadId":"61348","inReplyTo":null,"subject":"[PATCH] don't report vsnprintf(3) error as bug","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-21T12:40:28Z","receivedAt":"2024-04-21T12:40:42Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"strbuf_addf() has been reporting a negative return value of vsnprintf(3)\nas a bug since f141bd804d (Handle broken vsnprintf implementations in\nstrbuf, 2007-11-13). Other functions copied that behavior:\n\n7b03c89ebd (add xsnprintf helper function, 2015-09-24)\n5ef264dbdb (strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`, 2019-02-25)\n8d25663d70 (mem-pool: add mem_pool_strfmt(), 2024-02-25)\n\nHowever, vsnprintf(3) can legitimately return a negative value if the\nformatted output would be longer than INT_MAX. Stop accusing it of\nbeing broken and just report the fact that formatting failed.\n\nSuggested-by: Jeff King \nSigned-off-by: René Scharfe \n---\n mem-pool.c | 3 ++-\n strbuf.c | 4 ++--\n wrapper.c | 2 +-\n 3 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/mem-pool.c b/mem-pool.c\nindex 3065b12b23..a3ba38831d 100644\n--- a/mem-pool.c\n+++ b/mem-pool.c\n@@ -4,6 +4,7 @@\n\n #include \"git-compat-util.h\"\n #include \"mem-pool.h\"\n+#include \"gettext.h\"\n\n #define BLOCK_GROWTH_SIZE (1024 * 1024 - sizeof(struct mp_block))\n\n@@ -122,7 +123,7 @@ static char *mem_pool_strvfmt(struct mem_pool *pool, const char *fmt,\n \tlen = vsnprintf(next_free, available, fmt, cp);\n \tva_end(cp);\n \tif (len < 0)\n-\t\tBUG(\"your vsnprintf is broken (returned %d)\", len);\n+\t\tdie(_(\"unable to format message: %s\"), fmt);\n\n \tsize = st_add(len, 1); /* 1 for NUL */\n \tret = mem_pool_alloc(pool, size);\ndiff --git a/strbuf.c b/strbuf.c\nindex 1492a08225..0d929e4e19 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -277,7 +277,7 @@ void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt, va_list ap)\n \tlen = vsnprintf(sb->buf + sb->len, 0, fmt, cp);\n \tva_end(cp);\n \tif (len < 0)\n-\t\tBUG(\"your vsnprintf is broken (returned %d)\", len);\n+\t\tdie(_(\"unable to format message: %s\"), fmt);\n \tif (!len)\n \t\treturn; /* nothing to do */\n \tif (unsigned_add_overflows(sb->len, len))\n@@ -404,7 +404,7 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)\n \tlen = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, cp);\n \tva_end(cp);\n \tif (len < 0)\n-\t\tBUG(\"your vsnprintf is broken (returned %d)\", len);\n+\t\tdie(_(\"unable to format message: %s\"), fmt);\n \tif (len > strbuf_avail(sb)) {\n \t\tstrbuf_grow(sb, len);\n \t\tlen = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);\ndiff --git a/wrapper.c b/wrapper.c\nindex eeac3741cf..f87d90bf57 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -670,7 +670,7 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)\n \tva_end(ap);\n\n \tif (len < 0)\n-\t\tBUG(\"your snprintf is broken\");\n+\t\tdie(_(\"unable to format message: %s\"), fmt);\n \tif (len >= max)\n \t\tBUG(\"attempt to snprintf into too-small buffer\");\n \treturn len;\n--\n2.44.0\n"},{"id":"493290","messageId":"xmqqa5lm1pr5.fsf@gitster.g","threadId":"61348","inReplyTo":"ea752a2b-9b74-4a59-a037-4782abf7161e@web.de","subject":"Re: [PATCH] don't report vsnprintf(3) error as bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-21T19:26:22Z","receivedAt":"2024-04-21T19:26:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> strbuf_addf() has been reporting a negative return value of vsnprintf(3)\n> as a bug since f141bd804d (Handle broken vsnprintf implementations in\n> strbuf, 2007-11-13). Other functions copied that behavior:\n>\n> 7b03c89ebd (add xsnprintf helper function, 2015-09-24)\n> 5ef264dbdb (strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`, 2019-02-25)\n> 8d25663d70 (mem-pool: add mem_pool_strfmt(), 2024-02-25)\n>\n> However, vsnprintf(3) can legitimately return a negative value if the\n> formatted output would be longer than INT_MAX. Stop accusing it of\n> being broken and just report the fact that formatting failed.\n\n\"\"\" ... function returns the number of characters that would have\nbeen written had n been sufficiently large, not counting the\nterminating null character, or a negative value if an encoding error\noccurred. Thus, the null-terminated output has been completely\nwritten if and only if the returned value is nonnegative and less\nthan n.\"\"\" is what I read in some versions of ISO/IEC 9899. It is\ncurious that it does not say anything about the consequence of a\nparameter error arising from int (the type snprintf family of\nfunctions returns) being narrower than size_t (the type of the\nparameter n), but your point still stands that vsnprintf() can\nlegitimately fail, and it is not a programming error.\n\nThanks, will queue.\n"},{"id":"493377","messageId":"20240423222652.GD1172807@coredump.intra.peff.net","threadId":"61348","inReplyTo":"xmqqa5lm1pr5.fsf@gitster.g","subject":"Re: [PATCH] don't report vsnprintf(3) error as bug","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-23T22:26:52Z","receivedAt":"2024-04-23T22:26:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 21, 2024 at 12:26:22PM -0700, Junio C Hamano wrote:\n\n> René Scharfe writes:\n> \n> > strbuf_addf() has been reporting a negative return value of vsnprintf(3)\n> > as a bug since f141bd804d (Handle broken vsnprintf implementations in\n> > strbuf, 2007-11-13). Other functions copied that behavior:\n> >\n> > 7b03c89ebd (add xsnprintf helper function, 2015-09-24)\n> > 5ef264dbdb (strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`, 2019-02-25)\n> > 8d25663d70 (mem-pool: add mem_pool_strfmt(), 2024-02-25)\n> >\n> > However, vsnprintf(3) can legitimately return a negative value if the\n> > formatted output would be longer than INT_MAX. Stop accusing it of\n> > being broken and just report the fact that formatting failed.\n> \n> \"\"\" ... function returns the number of characters that would have\n> been written had n been sufficiently large, not counting the\n> terminating null character, or a negative value if an encoding error\n> occurred. Thus, the null-terminated output has been completely\n> written if and only if the returned value is nonnegative and less\n> than n.\"\"\" is what I read in some versions of ISO/IEC 9899. It is\n> curious that it does not say anything about the consequence of a\n> parameter error arising from int (the type snprintf family of\n> functions returns) being narrower than size_t (the type of the\n> parameter n), but your point still stands that vsnprintf() can\n> legitimately fail, and it is not a programming error.\n\nPOSIX does say:\n\n The snprintf() function shall fail if:\n\n EOVERFLOW\n The value of n is greater than {INT_MAX}.\n\nBut mostly the INT_MAX thing is simply the one thing we've seen in\npractice. I wouldn't be surprised if there are other conditions that can\ntrigger an error return from vsnprintf. E.g., POSIX says:\n\n If a conversion specification does not match one of the above forms,\n the behavior is undefined.\n\nOf course \"undefined\" is much worse than returning -1, but it seems like\na reasonable thing for an implementation to choose to do (either that or\njust output the character literally).\n\nWe should be immune-ish to such an issue by virtue of -Wformat (we're\nonly allowed to pass literal strings, and they must all be understood by\nthe compiler). Of course that's platform-specific and we only check\n-Werror on some platforms. So gcc allows \"%b\", but it may be meaningless\non AIX, and who knows what their libc will do. ;)\n\nThat case kind of _is_ a BUG() situation. But I don't think it's worth\ntrying to differentiate that. Switching all of these to die() makes the\nmost sense (i.e., I like René's patch).\n\n-Peff\n"}]}