{"thread":{"id":"61238","subject":"[PATCH] mem-pool: use st_add() in mem_pool_strvfmt()","startedAt":"2024-03-31T18:53:09Z","lastAt":"2024-04-05T17:45:39Z","messageCount":9,"participants":["René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"491925","messageId":"bbe00b9e-64d8-4ec8-a2b9-2c6917c72dbd@web.de","threadId":"61238","inReplyTo":null,"subject":"[PATCH] mem-pool: use st_add() in mem_pool_strvfmt()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-03-31T18:53:07Z","receivedAt":"2024-03-31T18:53:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If len is INT_MAX in mem_pool_strvfmt(), then len + 1 overflows.\nCasting it to size_t would prevent that.  Use st_add() to go a step\nfurther and make the addition *obviously* safe.  The compiler can\noptimize the check away on platforms where SIZE_MAX > INT_MAX, i.e.\nbasically everywhere.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n mem-pool.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/mem-pool.c b/mem-pool.c\nindex 2078c22b09..3065b12b23 100644\n--- a/mem-pool.c\n+++ b/mem-pool.c\n@@ -115,6 +115,7 @@ static char *mem_pool_strvfmt(struct mem_pool *pool, const char *fmt,\n \tsize_t available = block ? block->end - block->next_free : 0;\n \tva_list cp;\n \tint len, len2;\n+\tsize_t size;\n \tchar *ret;\n\n \tva_copy(cp, ap);\n@@ -123,13 +124,14 @@ static char *mem_pool_strvfmt(struct mem_pool *pool, const char *fmt,\n \tif (len < 0)\n \t\tBUG(\"your vsnprintf is broken (returned %d)\", len);\n\n-\tret = mem_pool_alloc(pool, len + 1);  /* 1 for NUL */\n+\tsize = st_add(len, 1); /* 1 for NUL */\n+\tret = mem_pool_alloc(pool, size);\n\n \t/* Shortcut; relies on mem_pool_alloc() not touching buffer contents. */\n \tif (ret == next_free)\n \t\treturn ret;\n\n-\tlen2 = vsnprintf(ret, len + 1, fmt, ap);\n+\tlen2 = vsnprintf(ret, size, fmt, ap);\n \tif (len2 != len)\n \t\tBUG(\"your vsnprintf is broken (returns inconsistent lengths)\");\n \treturn ret;\n--\n2.44.0\n"},{"id":"491941","messageId":"20240401033642.GB2639525@coredump.intra.peff.net","threadId":"61238","inReplyTo":"bbe00b9e-64d8-4ec8-a2b9-2c6917c72dbd@web.de","subject":"Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-01T03:36:42Z","receivedAt":"2024-04-01T03:36:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 31, 2024 at 08:53:07PM +0200, René Scharfe wrote:\n\n> If len is INT_MAX in mem_pool_strvfmt(), then len + 1 overflows.\n> Casting it to size_t would prevent that.  Use st_add() to go a step\n> further and make the addition *obviously* safe.  The compiler can\n> optimize the check away on platforms where SIZE_MAX > INT_MAX, i.e.\n> basically everywhere.\n\nYeah, I think this is a good thing to do. I was confused at first why we\nhad an \"int\" at all, but it's the usual crappy snprintf interface.\n\nWhich, by the way...\n\n> @@ -123,13 +124,14 @@ static char *mem_pool_strvfmt(struct mem_pool *pool, const char *fmt,\n>  \tif (len < 0)\n>  \t\tBUG(\"your vsnprintf is broken (returned %d)\", len);\n\nNot new in your patch, and I know this is copied from the strbuf code,\nbut I think a BUG() is probably the wrong thing. We added it long ago to\nlet us know about broken vsnprintf() implementations, but we'd have\nflushed those out by now, as nothing in Git would work on such a\nplatform.\n\nAnd meanwhile there are legitimate reasons for a non-broken vsnprintf()\nto return -1: namely that it is the only useful thing they can do when\nthe requested string is larger than INT_MAX (e.g., \"%s\" on a string that\nis over 2GB). This is sort of academic, of course. There's no useful\nerror to return here, and anybody who manages to shove 2GB into a place\nwhere we expect a short string fully deserves to have their program\nabort.\n\nI don't have a good example of where you can trigger this (it used to be\neasy with long attribute names, but these days we refuse to parse them).\nBut in general probably calling die() is more appropriate.\n\nThere's a similar call in vreportf() that tries to keep going, but it\nends up with lousy results. E.g., try:\n\n  perl -le 'print \"create refs/heads/\", \"a\"x2147483648, \"HEAD\"' |\n  git update-ref --stdin\n\nwhich results in just \"fatal: \", since formatting the error string\nfails. Perhaps we should just print the unexpanded format string\n(\"invalid ref format: %s\" in this case). It's not great, but it's better\nthan nothing.\n\nI guess I diverged quite far from reviewing your patch. ;) It obviously\nlooks fine, but the snprintf() int return value got me off on a tangent.\n\n-Peff\n"},{"id":"492056","messageId":"9f26b9f0-f8d7-4988-b6d4-e0446dab30b1@web.de","threadId":"61238","inReplyTo":"20240401033642.GB2639525@coredump.intra.peff.net","subject":"Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-02T13:48:45Z","receivedAt":"2024-04-02T13:54:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.04.24 um 05:36 schrieb Jeff King:\n> On Sun, Mar 31, 2024 at 08:53:07PM +0200, René Scharfe wrote:\n>\n> Which, by the way...\n>\n>> @@ -123,13 +124,14 @@ static char *mem_pool_strvfmt(struct mem_pool *pool, const char *fmt,\n>>  \tif (len < 0)\n>>  \t\tBUG(\"your vsnprintf is broken (returned %d)\", len);\n>\n> Not new in your patch, and I know this is copied from the strbuf code,\n> but I think a BUG() is probably the wrong thing. We added it long ago to\n> let us know about broken vsnprintf() implementations, but we'd have\n> flushed those out by now, as nothing in Git would work on such a\n> platform.\n>\n> And meanwhile there are legitimate reasons for a non-broken vsnprintf()\n> to return -1: namely that it is the only useful thing they can do when\n> the requested string is larger than INT_MAX (e.g., \"%s\" on a string that\n> is over 2GB). This is sort of academic, of course. There's no useful\n> error to return here, and anybody who manages to shove 2GB into a place\n> where we expect a short string fully deserves to have their program\n> abort.\n>\n> I don't have a good example of where you can trigger this (it used to be\n> easy with long attribute names, but these days we refuse to parse them).\n> But in general probably calling die() is more appropriate.\n\nMakes sense.  Could be rolled into a new wrapper, xvsnprintf();\nimap-send.c::nfvasprintf() could call it as well.\n\nThere are also callers of vsnprintf(3) that use its return value without\nchecking for error: builtin/receive-pack.c::report_message(),\npath.c::mksnpath() and arguably imap-send.c::nfsnprintf().\n\n> There's a similar call in vreportf() that tries to keep going, but it\n> ends up with lousy results. E.g., try:\n>\n>   perl -le 'print \"create refs/heads/\", \"a\"x2147483648, \"HEAD\"' |\n>   git update-ref --stdin\n>\n> which results in just \"fatal: \", since formatting the error string\n> fails. Perhaps we should just print the unexpanded format string\n> (\"invalid ref format: %s\" in this case). It's not great, but it's better\n> than nothing.\n\nWe can throw in errno to distinguish between EILSEQ (invalid wide\ncharacter) and EOVERFLOW.  And we'd better not call die_errno() to avoid\ntriggering a recursion warning.  We can open-code it instead:\n\n        if (vsnprintf(p, pend - p, err, params) < 0) {\n                fprintf(stderr, _(\"%sunable to format message '%s': %s\\n\"),\n                        _(\"fatal: \"), err, strerror(errno));\n                exit(128);\n        }\n\nBut when I ran your test command (on macOS 14.4.1) ten times with this\nchange I got:\n\nfatal: unable to format message 'invalid ref format: %s': Invalid argument\nfatal: unable to format message 'invalid ref format: %s': Undefined error: 0\nfatal: unable to format message 'invalid ref format: %s': Invalid argument\nfatal: unable to format message 'invalid ref format: %s': Undefined error: 0\nfatal: unable to format message 'invalid ref format: %s': Invalid argument\nfatal: unable to format message 'invalid ref format: %s': Undefined error: 0\nfatal: unable to format message 'invalid ref format: %s': Invalid argument\nfatal: unable to format message 'invalid ref format: %s': Undefined error: 0\nfatal: unable to format message 'invalid ref format: %s': Undefined error: 0\nfatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n\nWhich scares me.  Why is errno sometimes zero?  Why EINVAL instead of\nEOVERFLOW?  O_o\n\nRené\n"},{"id":"492127","messageId":"20240403011818.GB892394@coredump.intra.peff.net","threadId":"61238","inReplyTo":"9f26b9f0-f8d7-4988-b6d4-e0446dab30b1@web.de","subject":"Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-03T01:18:18Z","receivedAt":"2024-04-03T01:18:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 02, 2024 at 03:48:45PM +0200, René Scharfe wrote:\n\n> Makes sense.  Could be rolled into a new wrapper, xvsnprintf();\n> imap-send.c::nfvasprintf() could call it as well.\n> \n> There are also callers of vsnprintf(3) that use its return value without\n> checking for error: builtin/receive-pack.c::report_message(),\n> path.c::mksnpath() and arguably imap-send.c::nfsnprintf().\n\nHmm, yeah. Those are all OK not to use xsnprintf(), since they handle\ntruncation themselves. But the first two don't look like they handle a\nnegative return well. In report_message(), we'd end up shrinking \"sz\".\nThat's potentially an out-of-bounds problem, except that we'll always\nhave put a non-empty prefix into the buffer. For mksnpath(), though, I\nsuspect that trying to format a very long name could result in the\noutput being full of uninitialized bytes.\n\nIt only has one caller, which creates \"foo~1\" when we got EEXIST from\n\"foo\". So I doubt you can get up to too much mischief with it. But it\ncould easily be replaced by mkpathdup() (or even a reusable strbuf\noutside the loop if you really wanted to hyper-optimize)\n\nAnd then we could get rid of mksnpath() entirely, and its horrible\nbad_path failure mode.\n\n> We can throw in errno to distinguish between EILSEQ (invalid wide\n> character) and EOVERFLOW.  And we'd better not call die_errno() to avoid\n> triggering a recursion warning.  We can open-code it instead:\n> \n>         if (vsnprintf(p, pend - p, err, params) < 0) {\n>                 fprintf(stderr, _(\"%sunable to format message '%s': %s\\n\"),\n>                         _(\"fatal: \"), err, strerror(errno));\n>                 exit(128);\n>         }\n\nWe could also just throw it into the buffer and let the rest of the\nfunction proceed, like:\n\ndiff --git a/usage.c b/usage.c\nindex 09f0ed509b..5baab98fa3 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -19,8 +19,10 @@ 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\tif (snprintf(p, pend - p, \"could not format error: %s\", err) < 0)\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\nThough most of the rest of the function is not that useful (it is mostly\nremoving unreadable chars, and hopefully we do not have any of those in\nour format strings!). I had not thought about showing strerror(). The C\nstandard does mention a negative value for encoding errors, but says\nnothing about errno. POSIX does seem to mention EILSEQ and EOVERFLOW.\nSo this...\n\n> But when I ran your test command (on macOS 14.4.1) ten times with this\n> change I got:\n> \n> fatal: unable to format message 'invalid ref format: %s': Invalid argument\n> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n> fatal: unable to format message 'invalid ref format: %s': Invalid argument\n> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n> fatal: unable to format message 'invalid ref format: %s': Invalid argument\n> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n> fatal: unable to format message 'invalid ref format: %s': Invalid argument\n> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n> \n> Which scares me.  Why is errno sometimes zero?  Why EINVAL instead of\n> EOVERFLOW?  O_o\n\n...is just confusing. I do think even without worrying about errno,\nsimply saying \"I tried to format 'some error: %s' and couldn't\" is going\nto be way more useful than just \"fatal: \". The only reason it would fail\nis that there's something gross in that \"%s\". We can't be more specific\nwithout interpreting the printf-format ourselves (which is probably not\nworth it).\n\n-Peff\n"},{"id":"492147","messageId":"b48fd3ee-2975-481f-aa0e-8ec4d07ea705@web.de","threadId":"61238","inReplyTo":"20240403011818.GB892394@coredump.intra.peff.net","subject":"Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-03T10:01:13Z","receivedAt":"2024-04-03T10:06:35Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.04.24 um 03:18 schrieb Jeff King:\n> On Tue, Apr 02, 2024 at 03:48:45PM +0200, René Scharfe wrote:\n>\n>> Makes sense.  Could be rolled into a new wrapper, xvsnprintf();\n>> imap-send.c::nfvasprintf() could call it as well.\n>>\n>> There are also callers of vsnprintf(3) that use its return value without\n>> checking for error: builtin/receive-pack.c::report_message(),\n>> path.c::mksnpath() and arguably imap-send.c::nfsnprintf().\n>\n> Hmm, yeah. Those are all OK not to use xsnprintf(), since they handle\n> truncation themselves. But the first two don't look like they handle a\n> negative return well. In report_message(), we'd end up shrinking \"sz\".\n> That's potentially an out-of-bounds problem, except that we'll always\n> have put a non-empty prefix into the buffer.\n\nGetting only a truncated prefix is hopefully detected as invalid, but\nexplicit handling would probably be better.\n\n> For mksnpath(), though, I\n> suspect that trying to format a very long name could result in the\n> output being full of uninitialized bytes.\n>\n> It only has one caller, which creates \"foo~1\" when we got EEXIST from\n> \"foo\". So I doubt you can get up to too much mischief with it. But it\n> could easily be replaced by mkpathdup() (or even a reusable strbuf\n> outside the loop if you really wanted to hyper-optimize)\n>\n> And then we could get rid of mksnpath() entirely, and its horrible\n> bad_path failure mode.\n\nmkpath() would be perfect but unusable in multiple threads.  Cleaning\nup after mkpathdup() is a bit iffy in that caller.  strbuf would be a\nbit much in that error path, I think, and you might have to export or\nreimplement strbuf_cleanup_path().\n\n>> We can throw in errno to distinguish between EILSEQ (invalid wide\n>> character) and EOVERFLOW.  And we'd better not call die_errno() to avoid\n>> triggering a recursion warning.  We can open-code it instead:\n>>\n>>         if (vsnprintf(p, pend - p, err, params) < 0) {\n>>                 fprintf(stderr, _(\"%sunable to format message '%s': %s\\n\"),\n>>                         _(\"fatal: \"), err, strerror(errno));\n>>                 exit(128);\n>>         }\n>\n> We could also just throw it into the buffer and let the rest of the\n> function proceed, like:\n>\n> diff --git a/usage.c b/usage.c\n> index 09f0ed509b..5baab98fa3 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -19,8 +19,10 @@ 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\tif (snprintf(p, pend - p, \"could not format error: %s\", err) < 0)\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>\n> Though most of the rest of the function is not that useful (it is mostly\n> removing unreadable chars, and hopefully we do not have any of those in\n> our format strings!).\n\nFor warnings and usage messages this would keep the prefix and not\ndie.  This would look a bit strange, no?\n\n\tusage: could not format error: TERRIBLE MESSAGE!\n\n> I had not thought about showing strerror(). The C\n> standard does mention a negative value for encoding errors, but says\n> nothing about errno. POSIX does seem to mention EILSEQ and EOVERFLOW.\n> So this...\n>\n>> But when I ran your test command (on macOS 14.4.1) ten times with this\n>> change I got:\n>>\n>> fatal: unable to format message 'invalid ref format: %s': Invalid argument\n>> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n>> fatal: unable to format message 'invalid ref format: %s': Invalid argument\n>> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n>> fatal: unable to format message 'invalid ref format: %s': Invalid argument\n>> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n>> fatal: unable to format message 'invalid ref format: %s': Invalid argument\n>> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n>> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n>> fatal: unable to format message 'invalid ref format: %s': Undefined error: 0\n>>\n>> Which scares me.  Why is errno sometimes zero?  Why EINVAL instead of\n>> EOVERFLOW?  O_o\n>\n> ...is just confusing. I do think even without worrying about errno,\n> simply saying \"I tried to format 'some error: %s' and couldn't\" is going\n> to be way more useful than just \"fatal: \". The only reason it would fail\n> is that there's something gross in that \"%s\". We can't be more specific\n> without interpreting the printf-format ourselves (which is probably not\n> worth it).\n\nYes, showing errno doesn't add that much value.  Except in this case it\nshows that there's something going on that I don't understand.  Dare I\ndig deeper?  Probably not..\n\nRené\n"},{"id":"492188","messageId":"20240403204836.GC1949464@coredump.intra.peff.net","threadId":"61238","inReplyTo":"b48fd3ee-2975-481f-aa0e-8ec4d07ea705@web.de","subject":"Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-03T20:48:36Z","receivedAt":"2024-04-03T20:48:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 03, 2024 at 12:01:13PM +0200, René Scharfe wrote:\n\n> > For mksnpath(), though, I\n> > suspect that trying to format a very long name could result in the\n> > output being full of uninitialized bytes.\n> >\n> > It only has one caller, which creates \"foo~1\" when we got EEXIST from\n> > \"foo\". So I doubt you can get up to too much mischief with it. But it\n> > could easily be replaced by mkpathdup() (or even a reusable strbuf\n> > outside the loop if you really wanted to hyper-optimize)\n> >\n> > And then we could get rid of mksnpath() entirely, and its horrible\n> > bad_path failure mode.\n> \n> mkpath() would be perfect but unusable in multiple threads.  Cleaning\n> up after mkpathdup() is a bit iffy in that caller.  strbuf would be a\n> bit much in that error path, I think, and you might have to export or\n> reimplement strbuf_cleanup_path().\n\nYeah, I'd prefer not to go to mkpath(), even though that's the simplest\nthing, just because we should be reducing the subtle non-reentrant parts\nof the code over time. I don't think the cleanup handling for\nmkpathdup() is too bad:\n\ndiff --git a/apply.c b/apply.c\nindex 432837a674..15dfe607ff 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -4502,20 +4502,25 @@ static int create_one_file(struct apply_state *state,\n \t\tunsigned int nr = getpid();\n \n \t\tfor (;;) {\n-\t\t\tchar newpath[PATH_MAX];\n-\t\t\tmksnpath(newpath, sizeof(newpath), \"%s~%u\", path, nr);\n+\t\t\tchar *newpath = mkpathdup(\"%s~%u\", path, nr);\n \t\t\tres = try_create_file(state, newpath, mode, buf, size);\n-\t\t\tif (res < 0)\n+\t\t\tif (res < 0) {\n+\t\t\t\tfree(newpath);\n \t\t\t\treturn -1;\n+\t\t\t}\n \t\t\tif (!res) {\n-\t\t\t\tif (!rename(newpath, path))\n+\t\t\t\tif (!rename(newpath, path)) {\n+\t\t\t\t\tfree(newpath);\n \t\t\t\t\treturn 0;\n+\t\t\t\t}\n \t\t\t\tunlink_or_warn(newpath);\n+\t\t\t\tfree(newpath);\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tif (errno != EEXIST)\n \t\t\t\tbreak;\n \t\t\t++nr;\n+\t\t\tfree(newpath);\n \t\t}\n \t}\n \treturn error_errno(_(\"unable to write file '%s' mode %o\"),\n\nIt even gets a little easier if you hoist it to a strbuf outside the\nloop, as you don't need to free on each loop iteration. That seems worth\nit to me to get rid of a function that IMHO is mis-designed (it is\nreally only useful if you assume paths are bounded by PATH_MAX, which we\nknow isn't always true).\n\n> > diff --git a/usage.c b/usage.c\n> > index 09f0ed509b..5baab98fa3 100644\n> > --- a/usage.c\n> > +++ b/usage.c\n> > @@ -19,8 +19,10 @@ 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\tif (snprintf(p, pend - p, \"could not format error: %s\", err) < 0)\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> >\n> > Though most of the rest of the function is not that useful (it is mostly\n> > removing unreadable chars, and hopefully we do not have any of those in\n> > our format strings!).\n> \n> For warnings and usage messages this would keep the prefix and not\n> die.  This would look a bit strange, no?\n> \n> \tusage: could not format error: TERRIBLE MESSAGE!\n\nSure, but I think any solution here is going to look strange. Keep in\nmind that we're trying to improve the case where we print _nothing_\nuseful at all. If you do see this on a non-fatal message, the subsequent\nmessages may be informative. E.g.:\n\n  error: could not format error: unable to open loose object %s\n  fatal: bad object HEAD~12\n\nis probably better than just exiting after the first.\n\n> Yes, showing errno doesn't add that much value.  Except in this case it\n> shows that there's something going on that I don't understand.  Dare I\n> dig deeper?  Probably not..\n\nWell, let us know if you do. ;)\n\n-Peff\n"},{"id":"492190","messageId":"20240403212013.GA1957295@coredump.intra.peff.net","threadId":"61238","inReplyTo":"20240403204836.GC1949464@coredump.intra.peff.net","subject":"Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-03T21:20:13Z","receivedAt":"2024-04-03T21:20:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 03, 2024 at 04:48:36PM -0400, Jeff King wrote:\n\n> Yeah, I'd prefer not to go to mkpath(), even though that's the simplest\n> thing, just because we should be reducing the subtle non-reentrant parts\n> of the code over time. I don't think the cleanup handling for\n> mkpathdup() is too bad:\n> \n> diff --git a/apply.c b/apply.c\n> index 432837a674..15dfe607ff 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -4502,20 +4502,25 @@ static int create_one_file(struct apply_state *state,\n>  \t\tunsigned int nr = getpid();\n>  \n>  \t\tfor (;;) {\n> -\t\t\tchar newpath[PATH_MAX];\n> -\t\t\tmksnpath(newpath, sizeof(newpath), \"%s~%u\", path, nr);\n> +\t\t\tchar *newpath = mkpathdup(\"%s~%u\", path, nr);\n>  \t\t\tres = try_create_file(state, newpath, mode, buf, size);\n> -\t\t\tif (res < 0)\n> +\t\t\tif (res < 0) {\n> +\t\t\t\tfree(newpath);\n>  \t\t\t\treturn -1;\n> +\t\t\t}\n>  \t\t\tif (!res) {\n> -\t\t\t\tif (!rename(newpath, path))\n> +\t\t\t\tif (!rename(newpath, path)) {\n> +\t\t\t\t\tfree(newpath);\n>  \t\t\t\t\treturn 0;\n> +\t\t\t\t}\n>  \t\t\t\tunlink_or_warn(newpath);\n> +\t\t\t\tfree(newpath);\n>  \t\t\t\tbreak;\n>  \t\t\t}\n>  \t\t\tif (errno != EEXIST)\n>  \t\t\t\tbreak;\n>  \t\t\t++nr;\n> +\t\t\tfree(newpath);\n>  \t\t}\n>  \t}\n>  \treturn error_errno(_(\"unable to write file '%s' mode %o\"),\n\nOK, this misses one of the breaks, and potentially clobbers errno in\nthat path by calling free(). So it is harder than I said. I still think\nit's worth doing, though.\n\n-Peff\n"},{"id":"492293","messageId":"6f727926-a901-4d9f-8a7f-d966f222f15d@web.de","threadId":"61238","inReplyTo":"20240403204836.GC1949464@coredump.intra.peff.net","subject":"Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-04-05T13:10:33Z","receivedAt":"2024-04-05T13:10:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.04.24 um 22:48 schrieb Jeff King:\n> On Wed, Apr 03, 2024 at 12:01:13PM +0200, René Scharfe wrote:\n>\n>>> diff --git a/usage.c b/usage.c\n>>> index 09f0ed509b..5baab98fa3 100644\n>>> --- a/usage.c\n>>> +++ b/usage.c\n>>> @@ -19,8 +19,10 @@ 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\tif (snprintf(p, pend - p, \"could not format error: %s\", err) < 0)\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>>>\n>>> Though most of the rest of the function is not that useful (it is mostly\n>>> removing unreadable chars, and hopefully we do not have any of those in\n>>> our format strings!).\n\nHmm, this might be worth doing to avoid messing up the terminal if\n'err' points into the weeds for some reason.\n\n>> For warnings and usage messages this would keep the prefix and not\n>> die.  This would look a bit strange, no?\n>>\n>> \tusage: could not format error: TERRIBLE MESSAGE!\n>\n> Sure, but I think any solution here is going to look strange. Keep in\n> mind that we're trying to improve the case where we print _nothing_\n> useful at all. If you do see this on a non-fatal message, the subsequent\n> messages may be informative. E.g.:\n>\n>   error: could not format error: unable to open loose object %s\n>   fatal: bad object HEAD~12\n>\n> is probably better than just exiting after the first.\n\nOK, right, a format error doesn't have to be fatal and we can keep\nrunning and possibly provide more details.\n\nBut mixing the format error with the actual payload message is not ideal.\nAt least we should give the format error its proper prefix, while still\nreporting the prefix of the original message, e.g. like this:\n\n   error: unable to format message: unable to open loose object %s\n   fatal:\n\n... or this:\n\n   error: unable to format message: fatal: unable to open loose object %s\n\nI tend to like the first one slightly better, even though the empty\nmessage looks silly.  It doesn't mix the two and answers the questions\nthat I would have:  Why did the program stop?  Due to a fatal error.\nWhy is the fatal message silent?  The preceding error explains it.\n\nWhile the second one only reveals the fatality somewhere in the middle\nof the text, weakening the meaning of prefixes.\n\n>> Yes, showing errno doesn't add that much value.  Except in this case it\n>> shows that there's something going on that I don't understand.  Dare I\n>> dig deeper?  Probably not..\n>\n> Well, let us know if you do. ;)\n\nI still don't know why the error code varies between runs, but it\nclearly does not come from vsnprintf(3) -- if I set errno to some\narbitrary value before the call, it is kept.  Which is enough to\nconvince me to ignore errno here.\n\nRené\n"},{"id":"492311","messageId":"20240405174537.GD2529133@coredump.intra.peff.net","threadId":"61238","inReplyTo":"6f727926-a901-4d9f-8a7f-d966f222f15d@web.de","subject":"Re: [PATCH] mem-pool: use st_add() in mem_pool_strvfmt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-04-05T17:45:37Z","receivedAt":"2024-04-05T17:45:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 05, 2024 at 03:10:33PM +0200, René Scharfe wrote:\n\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\tif (snprintf(p, pend - p, \"could not format error: %s\", err) < 0)\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> >>>\n> >>> Though most of the rest of the function is not that useful (it is mostly\n> >>> removing unreadable chars, and hopefully we do not have any of those in\n> >>> our format strings!).\n> \n> Hmm, this might be worth doing to avoid messing up the terminal if\n> 'err' points into the weeds for some reason.\n\nI think we have bigger problems if \"err\" is messed up, because we'll be\ninterpreting garbage as a format string and almost certainly triggering\nundefined behavior. And in general if we are not using a constant string\nas the format it's a potential security vulnerability. So I think we can\nmostly rely on the compile-time -Wformat checks for this.\n\n> OK, right, a format error doesn't have to be fatal and we can keep\n> running and possibly provide more details.\n> \n> But mixing the format error with the actual payload message is not ideal.\n> At least we should give the format error its proper prefix, while still\n> reporting the prefix of the original message, e.g. like this:\n> \n>    error: unable to format message: unable to open loose object %s\n>    fatal:\n> \n> ... or this:\n> \n>    error: unable to format message: fatal: unable to open loose object %s\n> \n> I tend to like the first one slightly better, even though the empty\n> message looks silly.  It doesn't mix the two and answers the questions\n> that I would have:  Why did the program stop?  Due to a fatal error.\n> Why is the fatal message silent?  The preceding error explains it.\n> \n> While the second one only reveals the fatality somewhere in the middle\n> of the text, weakening the meaning of prefixes.\n\nYeah, I think your first one is good. It's _ugly_, but it's easy to\nfigure out what happened, and this is not something people generally see\n(and the status quo is just \"fatal:\", so you're easily 50% less ugly).\n\n> I still don't know why the error code varies between runs, but it\n> clearly does not come from vsnprintf(3) -- if I set errno to some\n> arbitrary value before the call, it is kept.  Which is enough to\n> convince me to ignore errno here.\n\nAgreed. Thanks for digging into it.\n\n-Peff\n"}]}