{"thread":{"id":"54170","subject":"Re: [PATCH] xrealloc: do not reuse pointer freed by zero-length realloc()","startedAt":"2020-09-01T13:10:23Z","lastAt":"2020-09-03T03:50:16Z","messageCount":10,"participants":["Derrick Stolee","Jeff King","Andreas Schwab","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"404860","messageId":"c81b7225-a663-1598-62b3-bd80457d5648@gmail.com","threadId":"54170","inReplyTo":"20200901111800.GA3115584@coredump.intra.peff.net","subject":"Re: [PATCH] xrealloc: do not reuse pointer freed by zero-length realloc()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-09-01T13:04:36Z","receivedAt":"2020-09-01T13:10:23Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/1/2020 7:18 AM, Jeff King wrote:\n> This patch fixes a bug where xrealloc(ptr, 0) can double-free and\n> corrupt the heap on some platforms (including at least glibc).\n\n!!! Good find !!!\n\n> The simplest fix here is to just pass \"ret\" (which we know to be NULL)\n> to the follow-up realloc(). That does mean that a system which _doesn't_\n> free the original pointer would leak it. But that interpretation of the\n> standard seems unlikely (if a system didn't deallocate in this case, I'd\n> expect it to simply return the original pointer). If it turns out to be\n> an issue, we can handle the \"!size\" case up front instead, before we\n> call realloc() at all.\n\nAdding an `if (!size) {free(ptr); return NULL;}` block was what I\nexpected. Was that chosen just so we can rely more on the system\nrealloc(), or is there a performance implication that I'm not\nseeing?\n\n> @@ -120,7 +120,7 @@ void *xrealloc(void *ptr, size_t size)\n>  \tmemory_limit_check(size, 0);\n>  \tret = realloc(ptr, size);\n>  \tif (!ret && !size)\n> -\t\tret = realloc(ptr, 1);\n> +\t\tret = realloc(ret, 1);\n\nI appreciate all the additional context for such a small change.\n\nLGTM.\n-Stolee\n"},{"id":"404863","messageId":"20200901135105.GA3284077@coredump.intra.peff.net","threadId":"54170","inReplyTo":"c81b7225-a663-1598-62b3-bd80457d5648@gmail.com","subject":"Re: [PATCH] xrealloc: do not reuse pointer freed by zero-length realloc()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-01T13:51:05Z","receivedAt":"2020-09-01T13:51:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 01, 2020 at 09:04:36AM -0400, Derrick Stolee wrote:\n\n> > The simplest fix here is to just pass \"ret\" (which we know to be NULL)\n> > to the follow-up realloc(). That does mean that a system which _doesn't_\n> > free the original pointer would leak it. But that interpretation of the\n> > standard seems unlikely (if a system didn't deallocate in this case, I'd\n> > expect it to simply return the original pointer). If it turns out to be\n> > an issue, we can handle the \"!size\" case up front instead, before we\n> > call realloc() at all.\n> \n> Adding an `if (!size) {free(ptr); return NULL;}` block was what I\n> expected. Was that chosen just so we can rely more on the system\n> realloc(), or is there a performance implication that I'm not\n> seeing?\n\nI went back and forth on whether to do that or not. This case should\nbasically never happen, so I like both the performance and readability\nof only triggering it when realloc() returns NULL. But it would get rid\nof the hand-waving above, and I doubt the performance is measurable.\n\nIf we do handle it up-front, then I think we'd actually want:\n\n  if (!size) {\n          free(ptr);\n\t  return xmalloc(0);\n  }\n\n(i.e., to never return NULL for consistency with xmalloc() and\nxcalloc()).\n\n> > @@ -120,7 +120,7 @@ void *xrealloc(void *ptr, size_t size)\n> >  \tmemory_limit_check(size, 0);\n> >  \tret = realloc(ptr, size);\n> >  \tif (!ret && !size)\n> > -\t\tret = realloc(ptr, 1);\n> > +\t\tret = realloc(ret, 1);\n> \n> I appreciate all the additional context for such a small change.\n\nSomebody's got to complete with you for ratio of commit message to diff\nlines. :)\n\n-Peff\n"},{"id":"404864","messageId":"a176961b-645f-4dcb-3439-596f814891f4@gmail.com","threadId":"54170","inReplyTo":"20200901135105.GA3284077@coredump.intra.peff.net","subject":"Re: [PATCH] xrealloc: do not reuse pointer freed by zero-length realloc()","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-09-01T14:24:18Z","receivedAt":"2020-09-01T14:31:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/1/2020 9:51 AM, Jeff King wrote:\n> On Tue, Sep 01, 2020 at 09:04:36AM -0400, Derrick Stolee wrote:\n>> Adding an `if (!size) {free(ptr); return NULL;}` block was what I\n>> expected. Was that chosen just so we can rely more on the system\n>> realloc(), or is there a performance implication that I'm not\n>> seeing?\n> \n> I went back and forth on whether to do that or not. This case should\n> basically never happen, so I like both the performance and readability\n> of only triggering it when realloc() returns NULL. But it would get rid\n> of the hand-waving above, and I doubt the performance is measurable.\n> \n> If we do handle it up-front, then I think we'd actually want:\n> \n>   if (!size) {\n>           free(ptr);\n> \t  return xmalloc(0);\n>   }\n> \n> (i.e., to never return NULL for consistency with xmalloc() and\n> xcalloc()).\n\nGood point. In that sense, your change makes a lot more\nsense for staying consistent without strangeness like xmalloc(0).\n\n>>> @@ -120,7 +120,7 @@ void *xrealloc(void *ptr, size_t size)\n>>>  \tmemory_limit_check(size, 0);\n>>>  \tret = realloc(ptr, size);\n>>>  \tif (!ret && !size)\n>>> -\t\tret = realloc(ptr, 1);\n>>> +\t\tret = realloc(ret, 1);\n>>\n>> I appreciate all the additional context for such a small change.\n> \n> Somebody's got to complete with you for ratio of commit message to diff\n> lines. :)\n\nPretty sure I have a long way to match, but it is important to\nhave goals.\n\n-Stolee\n"},{"id":"404868","messageId":"20200901111800.GA3115584@coredump.intra.peff.net","threadId":"54170","inReplyTo":null,"subject":"[PATCH] xrealloc: do not reuse pointer freed by zero-length realloc()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-01T11:18:00Z","receivedAt":"2020-09-01T14:54:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This patch fixes a bug where xrealloc(ptr, 0) can double-free and\ncorrupt the heap on some platforms (including at least glibc).\n\nThe C99 standard says of malloc (section 7.20.3):\n\n  If the size of the space requested is zero, the behavior is\n  implementation-defined: either a null pointer is returned, or the\n  behavior is as if the size were some nonzero value, except that the\n  returned pointer shall not be used to access an object.\n\nSo we might get NULL back, or we might get an actual pointer (but we're\nnot allowed to look at its contents). To simplify our code, our\nxmalloc() handles a NULL return by converting it into a single-byte\nallocation. That way callers get consistent behavior. This was done way\nback in 4e7a2eccc2 (?alloc: do not return NULL when asked for zero\nbytes, 2005-12-29).\n\nWe also gave xcalloc() and xrealloc() the same treatment. And according\nto C99, that is fine; the text above is in a paragraph that applies to\nall three. But what happens to the memory we passed to realloc() in such\na case? I.e., if we do:\n\n  ret = realloc(ptr, 0);\n\nand \"ptr\" is non-NULL, but we get NULL back, is \"ptr\" still valid? C99\ndoesn't cover this case specifically, but says (section 7.20.3.4):\n\n  The realloc function deallocates the old object pointed to by ptr and\n  returns a pointer to a new object that has the size specified by size.\n\nSo \"ptr\" is now deallocated, and we must only look at \"ret\". And since\n\"ret\" is NULL, that means we have no allocated object at all. But that's\nnot quite the whole story. It also says:\n\n  If memory for the new object cannot be allocated, the old object is\n  not deallocated and its value is unchanged.\n  [...]\n  The realloc function returns a pointer to the new object (which may\n  have the same value as a pointer to the old object), or a null pointer\n  if the new object could not be allocated.\n\nSo if we see a NULL return with a non-zero size, we can expect that the\noriginal object _is_ still valid. But with a zero size, it's ambiguous.\nThe NULL return might mean a failure (in which case the object is\nvalid), or it might mean that we successfully allocated nothing, and\nused NULL to represent that.\n\nThe glibc manpage for realloc() explicitly says:\n\n  [...]if size is equal to zero, and ptr is not NULL, then the call is\n  equivalent to free(ptr).\n\nLikewise, this StackOverflow answer:\n\n  https://stackoverflow.com/a/2135302\n\nclaims that C89 gave similar guidance (but I don't have a copy to verify\nit). A comment on this answer:\n\n  https://stackoverflow.com/a/2022410\n\nclaims that Microsoft's CRT behaves the same.\n\nBut our current \"retry with 1 byte\" code passes the original pointer\nagain. So on glibc, we effectively free() the pointer and then try to\nrealloc() it again, which is undefined behavior.\n\nThe simplest fix here is to just pass \"ret\" (which we know to be NULL)\nto the follow-up realloc(). That does mean that a system which _doesn't_\nfree the original pointer would leak it. But that interpretation of the\nstandard seems unlikely (if a system didn't deallocate in this case, I'd\nexpect it to simply return the original pointer). If it turns out to be\nan issue, we can handle the \"!size\" case up front instead, before we\ncall realloc() at all.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n wrapper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 4ff4a9c3db..b0d375beee 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -120,7 +120,7 @@ void *xrealloc(void *ptr, size_t size)\n \tmemory_limit_check(size, 0);\n \tret = realloc(ptr, size);\n \tif (!ret && !size)\n-\t\tret = realloc(ptr, 1);\n+\t\tret = realloc(ret, 1);\n \tif (!ret)\n \t\tdie(\"Out of memory, realloc failed\");\n \treturn ret;\n-- \n2.28.0.844.g468840f815\n"},{"id":"404870","messageId":"87o8mp8qx4.fsf@igel.home","threadId":"54170","inReplyTo":"20200901111800.GA3115584@coredump.intra.peff.net","subject":"Re: [PATCH] xrealloc: do not reuse pointer freed by zero-length realloc()","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2020-09-01T15:20:23Z","receivedAt":"2020-09-01T15:20:35Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Sep 01 2020, Jeff King wrote:\n\n> diff --git a/wrapper.c b/wrapper.c\n> index 4ff4a9c3db..b0d375beee 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -120,7 +120,7 @@ void *xrealloc(void *ptr, size_t size)\n>  \tmemory_limit_check(size, 0);\n>  \tret = realloc(ptr, size);\n>  \tif (!ret && !size)\n> -\t\tret = realloc(ptr, 1);\n> +\t\tret = realloc(ret, 1);\n\nSince you already know ret == NULL, you could just say malloc(1), As\nwritten, it looks like a typo (why use a variable to pass a constant?).\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"404872","messageId":"xmqq7dtdebik.fsf@gitster.c.googlers.com","threadId":"54170","inReplyTo":"20200901111800.GA3115584@coredump.intra.peff.net","subject":"Re: [PATCH] xrealloc: do not reuse pointer freed by zero-length realloc()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-01T15:56:35Z","receivedAt":"2020-09-01T15:56:47Z","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> The simplest fix here is to just pass \"ret\" (which we know to be NULL)\n> to the follow-up realloc(). That does mean that a system which _doesn't_\n> free the original pointer would leak it. But that interpretation of the\n> standard seems unlikely (if a system didn't deallocate in this case, I'd\n> expect it to simply return the original pointer). If it turns out to be\n> an issue, we can handle the \"!size\" case up front instead, before we\n> call realloc() at all.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  wrapper.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/wrapper.c b/wrapper.c\n> index 4ff4a9c3db..b0d375beee 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -120,7 +120,7 @@ void *xrealloc(void *ptr, size_t size)\n>  \tmemory_limit_check(size, 0);\n>  \tret = realloc(ptr, size);\n>  \tif (!ret && !size)\n> -\t\tret = realloc(ptr, 1);\n> +\t\tret = realloc(ret, 1);\n\nThe original does look bogus.\n\nIt however may be easier to reason about if we used malloc(1) in the\nfallback path for \"we got NULL after asking for 0-byte\" instead.  I\nwould have a hard time guessing the reason why we are reallocating\nNULL without going back to this commit, reading the log and seeing\nthe original to see that the reason why we didn't use malloc() but\nrealloc() is we aimed for a minimum change, if I encounter this code\nafter I forgot this discussion.\n\nThanks.\n"},{"id":"404873","messageId":"xmqq3641ebep.fsf@gitster.c.googlers.com","threadId":"54170","inReplyTo":"20200901135105.GA3284077@coredump.intra.peff.net","subject":"Re: [PATCH] xrealloc: do not reuse pointer freed by zero-length realloc()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-01T15:58:54Z","receivedAt":"2020-09-01T15:59:09Z","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> If we do handle it up-front, then I think we'd actually want:\n>\n>   if (!size) {\n>           free(ptr);\n> \t  return xmalloc(0);\n>   }\n>\n> (i.e., to never return NULL for consistency with xmalloc() and\n> xcalloc()).\n\nMakes sense.  I suspect that this is optimizing for a wrong case,\nbut in practice that should not matter.  Not having to worry about\na request to resize to 0-byte in the remainder of the function is\nactually a plus for readability, I would say.\n\n>> > @@ -120,7 +120,7 @@ void *xrealloc(void *ptr, size_t size)\n>> >  \tmemory_limit_check(size, 0);\n>> >  \tret = realloc(ptr, size);\n>> >  \tif (!ret && !size)\n>> > -\t\tret = realloc(ptr, 1);\n>> > +\t\tret = realloc(ret, 1);\n>> \n>> I appreciate all the additional context for such a small change.\n>\n> Somebody's got to complete with you for ratio of commit message to diff\n> lines. :)\n>\n> -Peff\n"},{"id":"404910","messageId":"20200902075439.GA855335@coredump.intra.peff.net","threadId":"54170","inReplyTo":"xmqq3641ebep.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] xrealloc: do not reuse pointer freed by zero-length realloc()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-02T07:54:39Z","receivedAt":"2020-09-02T07:54:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 01, 2020 at 08:58:54AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > If we do handle it up-front, then I think we'd actually want:\n> >\n> >   if (!size) {\n> >           free(ptr);\n> > \t  return xmalloc(0);\n> >   }\n> >\n> > (i.e., to never return NULL for consistency with xmalloc() and\n> > xcalloc()).\n> \n> Makes sense.  I suspect that this is optimizing for a wrong case,\n> but in practice that should not matter.  Not having to worry about\n> a request to resize to 0-byte in the remainder of the function is\n> actually a plus for readability, I would say.\n\nOK. Then here it is like that (and a slightly updated final paragraph in\nthe commit message).\n\nThere are other variants, too:\n\n  - we could use malloc(1) versus xmalloc(0). Maybe more\n    readable/obvious? But also potentially allocates an extra byte when\n    the platform malloc(0) would not need to.\n\n  - we could return a non-NULL \"ptr\" without shrinking it at all (nor\n    allocating anything new). This is perfectly legal, and the\n    underlying realloc() would still know the original size if anybody\n    ever asked to grow it back again.\n\nI have to admit I don't overly care between them. I suspect one of the\nreasons we never ran into this 15-year-old bug is that it's quite hard\nto convince Git to call realloc(0) in the first place. I only saw it\nwhen investigating a bug in another series, and there the problem turned\nout to be reading garbage bytes off the end of a buffer (which we\ninterpreted as a serialized ewah bitmap which happened to have a zero in\nits length byte).\n\n-- >8 --\nSubject: [PATCH] xrealloc: do not reuse pointer freed by zero-length realloc()\n\nThis patch fixes a bug where xrealloc(ptr, 0) can double-free and\ncorrupt the heap on some platforms (including at least glibc).\n\nThe C99 standard says of malloc (section 7.20.3):\n\n  If the size of the space requested is zero, the behavior is\n  implementation-defined: either a null pointer is returned, or the\n  behavior is as if the size were some nonzero value, except that the\n  returned pointer shall not be used to access an object.\n\nSo we might get NULL back, or we might get an actual pointer (but we're\nnot allowed to look at its contents). To simplify our code, our\nxmalloc() handles a NULL return by converting it into a single-byte\nallocation. That way callers get consistent behavior. This was done way\nback in 4e7a2eccc2 (?alloc: do not return NULL when asked for zero\nbytes, 2005-12-29).\n\nWe also gave xcalloc() and xrealloc() the same treatment. And according\nto C99, that is fine; the text above is in a paragraph that applies to\nall three. But what happens to the memory we passed to realloc() in such\na case? I.e., if we do:\n\n  ret = realloc(ptr, 0);\n\nand \"ptr\" is non-NULL, but we get NULL back, is \"ptr\" still valid? C99\ndoesn't cover this case specifically, but says (section 7.20.3.4):\n\n  The realloc function deallocates the old object pointed to by ptr and\n  returns a pointer to a new object that has the size specified by size.\n\nSo \"ptr\" is now deallocated, and we must only look at \"ret\". And since\n\"ret\" is NULL, that means we have no allocated object at all. But that's\nnot quite the whole story. It also says:\n\n  If memory for the new object cannot be allocated, the old object is\n  not deallocated and its value is unchanged.\n  [...]\n  The realloc function returns a pointer to the new object (which may\n  have the same value as a pointer to the old object), or a null pointer\n  if the new object could not be allocated.\n\nSo if we see a NULL return with a non-zero size, we can expect that the\noriginal object _is_ still valid. But with a non-zero size, it's\nambiguous. The NULL return might mean a failure (in which case the\nobject is valid), or it might mean that we successfully allocated\nnothing, and used NULL to represent that.\n\nThe glibc manpage for realloc() explicitly says:\n\n  [...]if size is equal to zero, and ptr is not NULL, then the call is\n  equivalent to free(ptr).\n\nLikewise, this StackOverflow answer:\n\n  https://stackoverflow.com/a/2135302\n\nclaims that C89 gave similar guidance (but I don't have a copy to verify\nit). A comment on this answer:\n\n  https://stackoverflow.com/a/2022410\n\nclaims that Microsoft's CRT behaves the same.\n\nBut our current \"retry with 1 byte\" code passes the original pointer\nagain. So on glibc, we effectively free() the pointer and then try to\nrealloc() it again, which is undefined behavior.\n\nThe simplest fix here is to just pass \"ret\" (which we know to be NULL)\nto the follow-up realloc(). But that means that a system which _doesn't_\nfree the original pointer would leak it. It's not clear if any such\nsystems exist, and that interpretation of the standard seems unlikely\n(I'd expect a system that doesn't deallocate to simply return the\noriginal pointer in this case). But it's easy enough to err on the safe\nside, and just never pass a zero size to realloc() at all.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n wrapper.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 4ff4a9c3db..bcda41e374 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -117,10 +117,13 @@ void *xrealloc(void *ptr, size_t size)\n {\n \tvoid *ret;\n \n+\tif (!size) {\n+\t\tfree(ptr);\n+\t\treturn xmalloc(0);\n+\t}\n+\n \tmemory_limit_check(size, 0);\n \tret = realloc(ptr, size);\n-\tif (!ret && !size)\n-\t\tret = realloc(ptr, 1);\n \tif (!ret)\n \t\tdie(\"Out of memory, realloc failed\");\n \treturn ret;\n-- \n2.28.0.844.g468840f815\n\n"},{"id":"404929","messageId":"xmqqo8moasvp.fsf@gitster.c.googlers.com","threadId":"54170","inReplyTo":"20200902075439.GA855335@coredump.intra.peff.net","subject":"Re: [PATCH] xrealloc: do not reuse pointer freed by zero-length realloc()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-02T19:19:38Z","receivedAt":"2020-09-02T19:19:44Z","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> There are other variants, too:\n>\n>   - we could use malloc(1) versus xmalloc(0). Maybe more\n>     readable/obvious? But also potentially allocates an extra byte when\n>     the platform malloc(0) would not need to.\n>\n>   - we could return a non-NULL \"ptr\" without shrinking it at all (nor\n>     allocating anything new). This is perfectly legal, and the\n>     underlying realloc() would still know the original size if anybody\n>     ever asked to grow it back again.\n>\n> I have to admit I don't overly care between them.\n\nI don't either.  I admit that the latter I didn't think of---it\nfeels tricky and harder to reason about than any other variants.\n\n> I suspect one of the\n> reasons we never ran into this 15-year-old bug is that it's quite hard\n> to convince Git to call realloc(0) in the first place. I only saw it\n> when investigating a bug in another series, and there the problem turned\n> out to be reading garbage bytes off the end of a buffer (which we\n> interpreted as a serialized ewah bitmap which happened to have a zero in\n> its length byte).\n\nThanks.\n\n"},{"id":"404953","messageId":"20200903035008.GF4035286@google.com","threadId":"54170","inReplyTo":"xmqq3641ebep.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] xrealloc: do not reuse pointer freed by zero-length realloc()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-09-03T03:50:08Z","receivedAt":"2020-09-03T03:50:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n\n>> If we do handle it up-front, then I think we'd actually want:\n>>\n>>   if (!size) {\n>>           free(ptr);\n>>           return xmalloc(0);\n>>   }\n>>\n>> (i.e., to never return NULL for consistency with xmalloc() and\n>> xcalloc()).\n>\n> Makes sense.  I suspect that this is optimizing for a wrong case,\n> but in practice that should not matter.  Not having to worry about\n> a request to resize to 0-byte in the remainder of the function is\n> actually a plus for readability, I would say.\n\nI agree with both points: if we were repeatedly shrinking and growing\na buffer and cared about its performance, then we'd want the first\nversion, and since we aren't, we should prefer this version that is\nmore readable.\n\nThanks,\nJonathan\n"}]}