{"thread":{"id":"61273","subject":"[PATCH] usage: report vsnprintf(3) failure","startedAt":"2024-04-05T18:59:57Z","lastAt":"2024-04-05T19:20:25Z","messageCount":2,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"492314","messageId":"3da13298-b6a6-4391-b8e8-5dae9a28b860@web.de","threadId":"61273","inReplyTo":null,"subject":"[PATCH] usage: report vsnprintf(3) failure","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-05T18:59:52Z","receivedAt":"2024-04-05T18:59:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"vreportf(), which is used e.g. by die() and warning() by default, calls\nvsnprintf(3) to format the message to report.  If that call fails, it\nonly prints the prefix, e.g. \"fatal: \" or \"warning: \".  This at least\ninforms users that they were supposed to get a message and reveals its\nseverity, but leaves them wondering what it may have been about.\n\nHere's an example where vreportf() tries to print a message with a 2GB\nstring, which is too much for vsnprintf(3):\n\n  $ perl -le 'print \"create refs/heads/\", \"a\"x2**31' | git update-ref --stdin\n  fatal:\n\nAt least report the formatting error along with the offending message\n(unformatted) to indicate why that message is empty.  Use fprintf(3)\ninstead of error() to get the message out directly and avoid recursing\nback into vreportf().\n\nWith this patch we get:\n\n  $ perl -le 'print \"create refs/heads/\", \"a\"x2**31' | git update-ref --stdin\n  error: unable to format message: invalid ref format: %s\n  fatal:\n\n... which allows users to at least get an idea of what went wrong.\n\nSuggested-by: Jeff King <peff@peff.net>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n usage.c | 5 ++++-\n 1 file changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/usage.c b/usage.c\nindex 09f0ed509b..7a2f7805f5 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -19,8 +19,11 @@ static void vreportf(const char *prefix, const char *err, va_list params)\n \t}\n \tmemcpy(msg, prefix, prefix_len);\n \tp = msg + prefix_len;\n-\tif (vsnprintf(p, pend - p, err, params) < 0)\n+\tif (vsnprintf(p, pend - p, err, params) < 0) {\n+\t\tfprintf(stderr, _(\"error: unable to format message: %s\\n\"),\n+\t\t\terr);\n \t\t*p = '\\0'; /* vsnprintf() failed, clip at prefix */\n+\t}\n\n \tfor (; p != pend - 1 && *p; p++) {\n \t\tif (iscntrl(*p) && *p != '\\t' && *p != '\\n')\n--\n2.44.0\n"},{"id":"492317","messageId":"20240405192024.GB2561807@coredump.intra.peff.net","threadId":"61273","inReplyTo":"3da13298-b6a6-4391-b8e8-5dae9a28b860@web.de","subject":"Re: [PATCH] usage: report vsnprintf(3) failure","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-05T19:20:24Z","receivedAt":"2024-04-05T19:20:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2024 at 08:59:52PM +0200, René Scharfe wrote:\n\n> vreportf(), which is used e.g. by die() and warning() by default, calls\n> vsnprintf(3) to format the message to report.  If that call fails, it\n> only prints the prefix, e.g. \"fatal: \" or \"warning: \".  This at least\n> informs users that they were supposed to get a message and reveals its\n> severity, but leaves them wondering what it may have been about.\n> \n> Here's an example where vreportf() tries to print a message with a 2GB\n> string, which is too much for vsnprintf(3):\n> \n>   $ perl -le 'print \"create refs/heads/\", \"a\"x2**31' | git update-ref --stdin\n>   fatal:\n> \n> At least report the formatting error along with the offending message\n> (unformatted) to indicate why that message is empty.  Use fprintf(3)\n> instead of error() to get the message out directly and avoid recursing\n> back into vreportf().\n> \n> With this patch we get:\n> \n>   $ perl -le 'print \"create refs/heads/\", \"a\"x2**31' | git update-ref --stdin\n>   error: unable to format message: invalid ref format: %s\n>   fatal:\n> \n> ... which allows users to at least get an idea of what went wrong.\n\nThanks, I think this is a good change and you've nicely summarized the\nsituation above. And the patch itself:\n\n> diff --git a/usage.c b/usage.c\n> index 09f0ed509b..7a2f7805f5 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -19,8 +19,11 @@ static void vreportf(const char *prefix, const char *err, va_list params)\n>  \t}\n>  \tmemcpy(msg, prefix, prefix_len);\n>  \tp = msg + prefix_len;\n> -\tif (vsnprintf(p, pend - p, err, params) < 0)\n> +\tif (vsnprintf(p, pend - p, err, params) < 0) {\n> +\t\tfprintf(stderr, _(\"error: unable to format message: %s\\n\"),\n> +\t\t\terr);\n>  \t\t*p = '\\0'; /* vsnprintf() failed, clip at prefix */\n> +\t}\n\nis nice and simply, and shouldn't have any unexpected side effects.\n\n-Peff\n"}]}