{"thread":{"id":"55051","subject":"Re: [PATCH] strbuf.c: optimize program logic","startedAt":"2021-01-26T06:26:31Z","lastAt":"2021-01-30T09:08:48Z","messageCount":8,"participants":["Junio C Hamano","胡哲宁","阿德烈 via GitGitGadget","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"415277","messageId":"xmqqy2gg2pdm.fsf@gitster.c.googlers.com","threadId":"55051","inReplyTo":"pull.846.git.1611637582625.gitgitgadget@gmail.com","subject":"Re: [PATCH] strbuf.c: optimize program logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-26T06:17:41Z","receivedAt":"2021-01-26T06:26:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"阿德烈 via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: ZheNing Hu <adlternative@gmail.com>\n>\n> the usage in strbuf.h tell us\"Alloc is somehow a\n> \"private\" member that should not be messed with.\n> use `strbuf_avail()`instead.\"\n\nWhen we use the word \"private\", it generally means it is private to\nthe implementation of the API.  IOW, it is usually fine for the\nimplementation of the API (i.e. for strbuf API, what you see in\nstrbuf.c) to use private members.\n\nIn any case, these changes are _not_ optimizations.  \n\nReplacing (alloc - len - 1) with strbuf_avail() is at best an\nequivalent rewrite (which is a good thing from readability's point\nof view, but not an optimization).  We know sb->alloc during the\nloop is never 0, but the compiler may miss the fact, so the inlined\nimplementation of _avail, i.e.\n\n\tstatic inline size_t strbuf_avail(const struct strbuf *sb)\n\t{\n\t        return sb->alloc ? sb->alloc - sb->len - 1 : 0;\n        }\n\nmay not incur call overhead, but may be pessimizing the executed\ncode.\n\nIf you compare the code in the loop in the second hunk below with\nwhat _setlen() does, I think you'll see the overhead of _setlen()\nrelative to the original code is even higher, so it may also be\npessimizing, not optimizing.\n\nSo, overall, I am not all that enthused to see this patch.\n\n\nOne thing I noticed is that, whether open coded like sb->len += got\nor made into parameter to strbuf_setlen(sb, sb->len + got), we are\nnot careful about sb->len growing too large and overflowing with the\naddition.  That may potentially be an interesting thing to look\ninto, but at the same time, unlike the usual \"compute the number of\nbytes we need to allocate and then call xmalloc()\" pattern, where we\ntry to be careful in the \"compute\" step by using st_add() macros,\nthis code actually keep growing the buffer, so by the time the size_t\noverflows and wraps around, we'd certainly have exhausted the memory\nalready, so it won't be an issue.\n\nBut we may want to audit existing code that is not careful when\npreparing the second parameter to strbuf_setlen().  We just\nanalyzed, if we were to accept this patch, that \"sb->len + got\" that\nappear as the second parameter to new call of strbuf_setlen() looks\nbad but would not matter in practice, but we may not be so lucky in\nother places.\n\nThanks for a food for thought.\n\n> diff --git a/strbuf.c b/strbuf.c\n> index e3397cc4c72..76f560a28d0 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -517,7 +517,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n>  \n>  \tstrbuf_grow(sb, hint ? hint : 8192);\n>  \tfor (;;) {\n> -\t\tssize_t want = sb->alloc - sb->len - 1;\n> +\t\tssize_t want = strbuf_avail(sb);\n>  \t\tssize_t got = read_in_full(fd, sb->buf + sb->len, want);\n>  \n>  \t\tif (got < 0) {\n> @@ -527,7 +527,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n>  \t\t\t\tstrbuf_setlen(sb, oldlen);\n>  \t\t\treturn -1;\n>  \t\t}\n> -\t\tsb->len += got;\n> +\t\tstrbuf_setlen(sb, sb->len + got);\n>  \t\tif (got < want)\n>  \t\t\tbreak;\n>  \t\tstrbuf_grow(sb, 8192);\n> @@ -543,7 +543,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n>  \tssize_t cnt;\n>  \n>  \tstrbuf_grow(sb, hint ? hint : 8192);\n> -\tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n> +\tcnt = xread(fd, sb->buf + sb->len, strbuf_avail(sb));\n>  \tif (cnt > 0)\n>  \t\tstrbuf_setlen(sb, sb->len + cnt);\n>  \telse if (oldalloc == 0)\n>\n> base-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707\n"},{"id":"415279","messageId":"CAOLTT8T1N2FSK3GiLaQUZt-OO5qzjQz7iq2cuKxasmuwnEZoXw@mail.gmail.com","threadId":"55051","inReplyTo":"xmqqy2gg2pdm.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] strbuf.c: optimize program logic","fromName":"胡哲宁","fromEmail":"adlternative@gmail.com","sentAt":"2021-01-26T10:44:57Z","receivedAt":"2021-01-26T12:32:40Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Junio C Hamano <gitster@pobox.com> 于2021年1月26日周二 下午2:17写道：\n>\n> \"阿德烈 via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: ZheNing Hu <adlternative@gmail.com>\n> >\n> > the usage in strbuf.h tell us\"Alloc is somehow a\n> > \"private\" member that should not be messed with.\n> > use `strbuf_avail()`instead.\"\n>\n> When we use the word \"private\", it generally means it is private to\n> the implementation of the API.  IOW, it is usually fine for the\n> implementation of the API (i.e. for strbuf API, what you see in\n> strbuf.c) to use private members.\n>\nWell, I just think most other functions in strbuf.c follow the use\nof `strbuf_avail()` instead of \"sb->alloc-sb->len-1\", and the\n\"sb->alloc-sb->len-1\" that appears in `strbuf_read()` is not so uniform.\n> In any case, these changes are _not_ optimizations.\n>\n> Replacing (alloc - len - 1) with strbuf_avail() is at best an\n> equivalent rewrite (which is a good thing from readability's point\n> of view, but not an optimization).  We know sb->alloc during the\n> loop is never 0, but the compiler may miss the fact, so the inlined\n> implementation of _avail, i.e.\n>\n>         static inline size_t strbuf_avail(const struct strbuf *sb)\n>         {\n>                 return sb->alloc ? sb->alloc - sb->len - 1 : 0;\n>         }\n>\n> may not incur call overhead, but may be pessimizing the executed\n> code.\nI agree,It may be a good practice not to use redundant inline functions,\nbecause it will not make the git binary file too bloated.\n>\n> If you compare the code in the loop in the second hunk below with\n> what _setlen() does, I think you'll see the overhead of _setlen()\n> relative to the original code is even higher, so it may also be\n> pessimizing, not optimizing.\n>\n> So, overall, I am not all that enthused to see this patch.\n>\n>\n> One thing I noticed is that, whether open coded like sb->len += got\n> or made into parameter to strbuf_setlen(sb, sb->len + got), we are\n> not careful about sb->len growing too large and overflowing with the\n> addition.  That may potentially be an interesting thing to look\n> into, but at the same time, unlike the usual \"compute the number of\n> bytes we need to allocate and then call xmalloc()\" pattern, where we\n> try to be careful in the \"compute\" step by using st_add() macros,\n> this code actually keep growing the buffer, so by the time the size_t\n> overflows and wraps around, we'd certainly have exhausted the memory\n> already, so it won't be an issue.\n>\nThis is true, but is there any good way to avoid this form of overflow?\n> But we may want to audit existing code that is not careful when\n> preparing the second parameter to strbuf_setlen().  We just\n> analyzed, if we were to accept this patch, that \"sb->len + got\" that\n> appear as the second parameter to new call of strbuf_setlen() looks\n> bad but would not matter in practice, but we may not be so lucky in\n> other places.\n>\nI thought before `strbuf_read_once()`have almost analogous\n\"strbuf_setlen(sb, sb->len + cnt)\",so I change it.May be you are right,\n\"sb->len + got\"is not safe.\n> Thanks for a food for thought.\n>\n> > diff --git a/strbuf.c b/strbuf.c\n> > index e3397cc4c72..76f560a28d0 100644\n> > --- a/strbuf.c\n> > +++ b/strbuf.c\n> > @@ -517,7 +517,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n> >\n> >       strbuf_grow(sb, hint ? hint : 8192);\n> >       for (;;) {\n> > -             ssize_t want = sb->alloc - sb->len - 1;\n> > +             ssize_t want = strbuf_avail(sb);\n> >               ssize_t got = read_in_full(fd, sb->buf + sb->len, want);\n> >\n> >               if (got < 0) {\n> > @@ -527,7 +527,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n> >                               strbuf_setlen(sb, oldlen);\n> >                       return -1;\n> >               }\n> > -             sb->len += got;\n> > +             strbuf_setlen(sb, sb->len + got);\n> >               if (got < want)\n> >                       break;\n> >               strbuf_grow(sb, 8192);\n> > @@ -543,7 +543,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n> >       ssize_t cnt;\n> >\n> >       strbuf_grow(sb, hint ? hint : 8192);\n> > -     cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n> > +     cnt = xread(fd, sb->buf + sb->len, strbuf_avail(sb));\n> >       if (cnt > 0)\n> >               strbuf_setlen(sb, sb->len + cnt);\n> >       else if (oldalloc == 0)\n> >\n> > base-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707\n"},{"id":"415328","messageId":"pull.846.git.1611637582625.gitgitgadget@gmail.com","threadId":"55051","inReplyTo":null,"subject":"[PATCH] strbuf.c: optimize program logic","fromName":"阿德烈 via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-01-26T05:06:22Z","receivedAt":"2021-01-26T21:59:36Z","isPatch":true,"sender":{"key":"name:阿德烈","avatar":null},"body":"From: ZheNing Hu <adlternative@gmail.com>\n\nthe usage in strbuf.h tell us\"Alloc is somehow a\n\"private\" member that should not be messed with.\nuse `strbuf_avail()`instead.\"\n\nI notice that `strbuf_read` and `strbuf_read_once` may\nuse \"sb->alloc - sb->len - 1\" instead of using\n`strbuf_avail()`,so I change it,in the same time,\nI change `sb->len+= got` to `strbuf_setlen()`,it may\nbetter to use existing api.\n\nSigned-off-by: ZheNing Hu <adlternative@gmail.com>\n---\n    strbuf.c: optimize program logic\n    \n    use strbuf_avail() is better than use Exposed .len and .alloc use\n    strbuf_setlen() is better than use Exposed .len,so i change them in\n    \"strbuf.c\".\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-846%2Fadlternative%2Fstrbuf_read_optimization-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-846/adlternative/strbuf_read_optimization-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/846\n\n strbuf.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex e3397cc4c72..76f560a28d0 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -517,7 +517,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \n \tstrbuf_grow(sb, hint ? hint : 8192);\n \tfor (;;) {\n-\t\tssize_t want = sb->alloc - sb->len - 1;\n+\t\tssize_t want = strbuf_avail(sb);\n \t\tssize_t got = read_in_full(fd, sb->buf + sb->len, want);\n \n \t\tif (got < 0) {\n@@ -527,7 +527,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)\n \t\t\t\tstrbuf_setlen(sb, oldlen);\n \t\t\treturn -1;\n \t\t}\n-\t\tsb->len += got;\n+\t\tstrbuf_setlen(sb, sb->len + got);\n \t\tif (got < want)\n \t\t\tbreak;\n \t\tstrbuf_grow(sb, 8192);\n@@ -543,7 +543,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)\n \tssize_t cnt;\n \n \tstrbuf_grow(sb, hint ? hint : 8192);\n-\tcnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);\n+\tcnt = xread(fd, sb->buf + sb->len, strbuf_avail(sb));\n \tif (cnt > 0)\n \t\tstrbuf_setlen(sb, sb->len + cnt);\n \telse if (oldalloc == 0)\n\nbase-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707\n-- \ngitgitgadget\n"},{"id":"415336","messageId":"YBBeLIhd+VHS25CE@coredump.intra.peff.net","threadId":"55051","inReplyTo":"xmqqy2gg2pdm.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] strbuf.c: optimize program logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-26T18:23:40Z","receivedAt":"2021-01-26T22:19:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 25, 2021 at 10:17:41PM -0800, Junio C Hamano wrote:\n\n> \"阿德烈 via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n> > From: ZheNing Hu <adlternative@gmail.com>\n> >\n> > the usage in strbuf.h tell us\"Alloc is somehow a\n> > \"private\" member that should not be messed with.\n> > use `strbuf_avail()`instead.\"\n> \n> When we use the word \"private\", it generally means it is private to\n> the implementation of the API.  IOW, it is usually fine for the\n> implementation of the API (i.e. for strbuf API, what you see in\n> strbuf.c) to use private members.\n> \n> In any case, these changes are _not_ optimizations.  \n\nYeah, I had both of those thoughts, too. :)\n\nThough...\n\n> Replacing (alloc - len - 1) with strbuf_avail() is at best an\n> equivalent rewrite (which is a good thing from readability's point\n> of view, but not an optimization).  We know sb->alloc during the\n> loop is never 0, but the compiler may miss the fact, so the inlined\n> implementation of _avail, i.e.\n> \n> \tstatic inline size_t strbuf_avail(const struct strbuf *sb)\n> \t{\n> \t        return sb->alloc ? sb->alloc - sb->len - 1 : 0;\n>         }\n> \n> may not incur call overhead, but may be pessimizing the executed\n> code.\n> \n> If you compare the code in the loop in the second hunk below with\n> what _setlen() does, I think you'll see the overhead of _setlen()\n> relative to the original code is even higher, so it may also be\n> pessimizing, not optimizing.\n> \n> So, overall, I am not all that enthused to see this patch.\n\nI would generally value readability/consistency here over trying to\nmicro-optimize an if-zero check.\n\nHowever, if strbuf_avail() ever did return 0, I'm not sure the loop\nwould make forward progress:\n\n          strbuf_grow(sb, hint ? hint : 8192);\n          for (;;) {\n                  ssize_t want = strbuf_avail(sb);\n                  ssize_t got = read_in_full(fd, sb->buf + sb->len, want);\n  \n                  if (got < 0) {\n                          if (oldalloc == 0)\n                                  strbuf_release(sb);\n                          else\n                                  strbuf_setlen(sb, oldlen);\n                          return -1;\n                  }\n                  strbuf_setlen(sb, sb->len + got);\n                  if (got < want)\n                          break;\n                  strbuf_grow(sb, 8192);\n          }\n\nwe'd just ask to read 0 bytes over and over. That almost makes me want\nto add:\n\n  if (!want)\n\tBUG(\"strbuf did not actually grow!?\");\n\nor possibly to teach the \"if (got < want)\" condition to check for a zero\nreturn (though I guess that would probably just end up confusing us into\nthinking we hit EOF).\n\n> One thing I noticed is that, whether open coded like sb->len += got\n> or made into parameter to strbuf_setlen(sb, sb->len + got), we are\n> not careful about sb->len growing too large and overflowing with the\n> addition.  That may potentially be an interesting thing to look\n> into, but at the same time, unlike the usual \"compute the number of\n> bytes we need to allocate and then call xmalloc()\" pattern, where we\n> try to be careful in the \"compute\" step by using st_add() macros,\n> this code actually keep growing the buffer, so by the time the size_t\n> overflows and wraps around, we'd certainly have exhausted the memory\n> already, so it won't be an issue.\n\nI think \"len\" is OK here. An invariant of strbuf is that \"len\" is\nsmaller than \"alloc\" for obvious reasons. So as long as the actual\nstrbuf_grow() is safe, then extending \"len\".\n\nI'm not sure that strbuf_grow() is safe, though. It relies on\nALLOC_GROW, which does not use st_add(), etc.\n\n-Peff\n\nPS The original patch does not seem to have made it to the list for some\n   reason (I didn't get a copy, and neither did lore.kernel.org).\n"},{"id":"415340","messageId":"xmqq8s8f3596.fsf@gitster.c.googlers.com","threadId":"55051","inReplyTo":"CAOLTT8T1N2FSK3GiLaQUZt-OO5qzjQz7iq2cuKxasmuwnEZoXw@mail.gmail.com","subject":"Re: [PATCH] strbuf.c: optimize program logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-26T18:47:01Z","receivedAt":"2021-01-26T22:19:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"胡哲宁 <adlternative@gmail.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> 于2021年1月26日周二 下午2:17写道：\n>>\n>> \"阿德烈 via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>\n>> > From: ZheNing Hu <adlternative@gmail.com>\n>> >\n>> > the usage in strbuf.h tell us\"Alloc is somehow a\n>> > \"private\" member that should not be messed with.\n>> > use `strbuf_avail()`instead.\"\n>>\n>> When we use the word \"private\", it generally means it is private to\n>> the implementation of the API.  IOW, it is usually fine for the\n>> implementation of the API (i.e. for strbuf API, what you see in\n>> strbuf.c) to use private members.\n>>\n> Well, I just think most other functions in strbuf.c follow the use\n> of `strbuf_avail()` instead of \"sb->alloc-sb->len-1\", and the\n> \"sb->alloc-sb->len-1\" that appears in `strbuf_read()` is not so uniform.\n\nI actually wouldn't have minded if this were sold as \"code clean-up\nto use _avail() when we open-code in the implementation of strbuf\nAPI in codepaths that are not performance critical.\"\n\nI am not sure about the _setlen() side of the thing.  It is quite\nobvious what is going on in the original, and it falls into \"when it\nis written one way that is good enough, replacing it with another\nthat is not significantly better often ends up being mere code\nchurn.\", I would think.\n\nThanks.\n"},{"id":"415356","messageId":"xmqqft2n1mkr.fsf@gitster.c.googlers.com","threadId":"55051","inReplyTo":"YBBeLIhd+VHS25CE@coredump.intra.peff.net","subject":"Re: [PATCH] strbuf.c: optimize program logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-26T20:15:48Z","receivedAt":"2021-01-26T22:19:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> PS The original patch does not seem to have made it to the list for some\n>    reason (I didn't get a copy, and neither did lore.kernel.org).\n\nYes, it seems today's one of these days where vger is hiccuping.  I\nsee out of order deliveries of responses to messages I've yet to\nsee.\n"},{"id":"415562","messageId":"CAOLTT8RroMdaty-SwAfK3OSeuFA7PE6ZhPCL4vgrxFxNihdGbQ@mail.gmail.com","threadId":"55051","inReplyTo":"YBBeLIhd+VHS25CE@coredump.intra.peff.net","subject":"Re: [PATCH] strbuf.c: optimize program logic","fromName":"胡哲宁","fromEmail":"adlternative@gmail.com","sentAt":"2021-01-29T06:09:12Z","receivedAt":"2021-01-29T06:08:23Z","isPatch":true,"sender":{"key":"adlternative@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58138461?v=4"},"body":"Jeff King <peff@peff.net> 于2021年1月27日周三 上午2:23写道：\n>\n> On Mon, Jan 25, 2021 at 10:17:41PM -0800, Junio C Hamano wrote:\n>\n> > \"阿德烈 via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> >\n> > > From: ZheNing Hu <adlternative@gmail.com>\n> > >\n> > > the usage in strbuf.h tell us\"Alloc is somehow a\n> > > \"private\" member that should not be messed with.\n> > > use `strbuf_avail()`instead.\"\n> >\n> > When we use the word \"private\", it generally means it is private to\n> > the implementation of the API.  IOW, it is usually fine for the\n> > implementation of the API (i.e. for strbuf API, what you see in\n> > strbuf.c) to use private members.\n> >\n> > In any case, these changes are _not_ optimizations.\n>\n> Yeah, I had both of those thoughts, too. :)\n>\n> Though...\n>\n> > Replacing (alloc - len - 1) with strbuf_avail() is at best an\n> > equivalent rewrite (which is a good thing from readability's point\n> > of view, but not an optimization).  We know sb->alloc during the\n> > loop is never 0, but the compiler may miss the fact, so the inlined\n> > implementation of _avail, i.e.\n> >\n> >       static inline size_t strbuf_avail(const struct strbuf *sb)\n> >       {\n> >               return sb->alloc ? sb->alloc - sb->len - 1 : 0;\n> >         }\n> >\n> > may not incur call overhead, but may be pessimizing the executed\n> > code.\n> >\n> > If you compare the code in the loop in the second hunk below with\n> > what _setlen() does, I think you'll see the overhead of _setlen()\n> > relative to the original code is even higher, so it may also be\n> > pessimizing, not optimizing.\n> >\n> > So, overall, I am not all that enthused to see this patch.\n>\n> I would generally value readability/consistency here over trying to\n> micro-optimize an if-zero check.\n>\n> However, if strbuf_avail() ever did return 0, I'm not sure the loop\n> would make forward progress:\n>\n>           strbuf_grow(sb, hint ? hint : 8192);\n>           for (;;) {\n>                   ssize_t want = strbuf_avail(sb);\n>                   ssize_t got = read_in_full(fd, sb->buf + sb->len, want);\n>\n>                   if (got < 0) {\n>                           if (oldalloc == 0)\n>                                   strbuf_release(sb);\n>                           else\n>                                   strbuf_setlen(sb, oldlen);\n>                           return -1;\n>                   }\n>                   strbuf_setlen(sb, sb->len + got);\n>                   if (got < want)\n>                           break;\n>                   strbuf_grow(sb, 8192);\n>           }\n>\n> we'd just ask to read 0 bytes over and over. That almost makes me want\n> to add:\n>\n>   if (!want)\n>         BUG(\"strbuf did not actually grow!?\");\n>\n> or possibly to teach the \"if (got < want)\" condition to check for a zero\n> return (though I guess that would probably just end up confusing us into\n> thinking we hit EOF).\n>\nI agree.But I always feel that there will be no such strange bug.\n> > One thing I noticed is that, whether open coded like sb->len += got\n> > or made into parameter to strbuf_setlen(sb, sb->len + got), we are\n> > not careful about sb->len growing too large and overflowing with the\n> > addition.  That may potentially be an interesting thing to look\n> > into, but at the same time, unlike the usual \"compute the number of\n> > bytes we need to allocate and then call xmalloc()\" pattern, where we\n> > try to be careful in the \"compute\" step by using st_add() macros,\n> > this code actually keep growing the buffer, so by the time the size_t\n> > overflows and wraps around, we'd certainly have exhausted the memory\n> > already, so it won't be an issue.\n>\n> I think \"len\" is OK here. An invariant of strbuf is that \"len\" is\n> smaller than \"alloc\" for obvious reasons. So as long as the actual\n> strbuf_grow() is safe, then extending \"len\".\n>\n> I'm not sure that strbuf_grow() is safe, though. It relies on\n> ALLOC_GROW, which does not use st_add(), etc.\n>\nI want to say is that `strbuf_grow()` have checked overflow before\n `ALLOC_GROW`,so that `strbuf_grow()`is maybe also safe too? :)\nvoid strbuf_grow(struct strbuf *sb, size_t extra)\n{\n       int new_buf = !sb->alloc;\n       if (unsigned_add_overflows(extra, 1) ||\n           unsigned_add_overflows(sb->len, extra + 1))\n               die(\"you want to use way too much memory\");\n       ...\n}\n> -Peff\n>\n> PS The original patch does not seem to have made it to the list for some\n>    reason (I didn't get a copy, and neither did lore.kernel.org).\n"},{"id":"415643","messageId":"YBUd504cLUJTLEQY@coredump.intra.peff.net","threadId":"55051","inReplyTo":"CAOLTT8RroMdaty-SwAfK3OSeuFA7PE6ZhPCL4vgrxFxNihdGbQ@mail.gmail.com","subject":"Re: [PATCH] strbuf.c: optimize program logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-30T08:50:47Z","receivedAt":"2021-01-30T09:08:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 29, 2021 at 02:09:12PM +0800, 胡哲宁 wrote:\n\n> > I'm not sure that strbuf_grow() is safe, though. It relies on\n> > ALLOC_GROW, which does not use st_add(), etc.\n> >\n> I want to say is that `strbuf_grow()` have checked overflow before\n>  `ALLOC_GROW`,so that `strbuf_grow()`is maybe also safe too? :)\n> void strbuf_grow(struct strbuf *sb, size_t extra)\n> {\n>        int new_buf = !sb->alloc;\n>        if (unsigned_add_overflows(extra, 1) ||\n>            unsigned_add_overflows(sb->len, extra + 1))\n>                die(\"you want to use way too much memory\");\n>        ...\n\nOh, you're right. I misread it as checking only \"extra\", but of course\nthe second line there is making sure our total requested size does not\noverflow.\n\nI do think ALLOC_GROW() could still overflow internally as it sizes\nlarger than sb->len + extra. But this is quite unlikely on a 64-bit\nsystem, as it would imply we're using on the order of 2^63 bytes of RAM\nbefore we enter the function. It potentially could be a problem on a\n32-bit system, though I'm not sure how much in practice (the numbers are\nsmall enough to be reasonable, but I'm not sure it's realistic that a\nsingle strbuf could already be using half of the available address\nspace).\n\n-Peff\n"}]}