{"thread":{"id":"52575","subject":"[Outreachy] Return value before or after free()?","startedAt":"2020-01-06T21:16:06Z","lastAt":"2020-01-07T20:40:20Z","messageCount":7,"participants":["Miriam R.","Jeff King","Jonathan Nieder","Andreas Schwab","brian m. carlson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"389288","messageId":"CAN7CjDDBA0ZoCG9aaQf5rg3gxqny=EjR6v6jE1mnxvUJQSF_0Q@mail.gmail.com","threadId":"52575","inReplyTo":null,"subject":"[Outreachy] Return value before or after free()?","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-06T21:15:53Z","receivedAt":"2020-01-06T21:16:06Z","isPatch":false,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"Dear all,\nin run-command.c file `exists_in_PATH()` function does this:\n\nstatic int exists_in_PATH(const char *file)\n{\nchar *r = locate_in_PATH(file);\nfree(r);\nreturn r != NULL;\n}\n\nI wonder if it is correct to do return r != NULL; after free(r);\n\nCould be this version more readable? :\n\nstatic int exists_in_PATH(const char *file)\n{\nchar *r = locate_in_PATH(file);\nint res = (r != NULL);\nfree(r);\nreturn res;\n}\n\nCould you please give me your feedback?\n\nThank you!\n\nBest,\nMiriam.\n"},{"id":"389291","messageId":"20200106213051.GD980197@coredump.intra.peff.net","threadId":"52575","inReplyTo":"CAN7CjDDBA0ZoCG9aaQf5rg3gxqny=EjR6v6jE1mnxvUJQSF_0Q@mail.gmail.com","subject":"Re: [Outreachy] Return value before or after free()?","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-06T21:30:51Z","receivedAt":"2020-01-06T21:30:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 06, 2020 at 10:15:53PM +0100, Miriam R. wrote:\n\n> in run-command.c file `exists_in_PATH()` function does this:\n> \n> static int exists_in_PATH(const char *file)\n> {\n> char *r = locate_in_PATH(file);\n> free(r);\n> return r != NULL;\n> }\n> \n> I wonder if it is correct to do return r != NULL; after free(r);\n\nIt is technically undefined behavior according to the C standard, but I\nthink it would be hard to find an implementation where it was not\nperfectly fine in practice.\n\nRef: http://c-faq.com/malloc/ptrafterfree.html\n\nI'd probably leave it alone unless it is causing a problem (e.g., a\nstatic analyzer complaining).\n\n-Peff\n"},{"id":"389292","messageId":"20200106224747.GA92456@google.com","threadId":"52575","inReplyTo":"20200106213051.GD980197@coredump.intra.peff.net","subject":"Re: [Outreachy] Return value before or after free()?","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-01-06T22:47:47Z","receivedAt":"2020-01-06T22:47:52Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n> On Mon, Jan 06, 2020 at 10:15:53PM +0100, Miriam R. wrote:\n\n>> in run-command.c file `exists_in_PATH()` function does this:\n>>\n>> static int exists_in_PATH(const char *file)\n>> {\n>>\tchar *r = locate_in_PATH(file);\n>>\tfree(r);\n>>\treturn r != NULL;\n>> }\n>>\n>> I wonder if it is correct to do return r != NULL; after free(r);\n>\n> It is technically undefined behavior according to the C standard, but I\n> think it would be hard to find an implementation where it was not\n> perfectly fine in practice.\n>\n> Ref: http://c-faq.com/malloc/ptrafterfree.html\n>\n> I'd probably leave it alone unless it is causing a problem (e.g., a\n> static analyzer complaining).\n\nToday I learned.\n\nMiriam, do you have more context?  Did you notice this while reading or\ndid a tool bring it to your attention?\n\n(Because I was curious, here's what I chased down in C99:\n\n7.20.3.2 \"The free function\" says: \"The free function causes the space\npointed to by ptr to be deallocated, that is, made available for\nfurther allocation.\"\n\n6.2.4 \"Storage durations of objects\" says: \"The value of a pointer\nbecomes indeterminate when the object it points to reaches the end of\nits lifetime.\"\n\n6.2.5 \"Types\" says: \"A pointer type describes an object whose value\nprovides a reference to an entity of the referenced type.\"\n\n6.5.9 \"Equality operators\": \"Two pointers compare equal if and only if\nboth are null pointers, both are pointers to the same object\n(including a pointer to an object and a subobject at its beginning) or\nfunction, both are pointers to one past the last element of the same\narray object, or one is a pointer to one past the end of one array\nobject and the other is a pointer to the start of a different array\nobject that happens to immediately follow the first array object in\nthe address space.\"\n\nJ.2 \"Undefined behavior\": \"The behavior is undefined in the following\ncircumstances: [...] The value of an object with automatic storage\nduration is used while it is indeterminate (6.2.4, 6.7.8, 6.8)\"\n\nThe reference to automatic storage duration there is interesting.  Of\ncourse `r` here does have automatic storage duration, but the\ndistinction from\n\n\tchar **r = xmalloc(sizeof(*r));\n\t*r = locate_in_PATH(file);\n\tfree(*r);\n\t/* leak r */\n\treturn *r != NULL;\n\nis peculiar.  It looks like exists_in_PATH is indeed producing\nundefined behavior, but the intention of the standard was probably to\nmake the behavior implementation defined.)\n\nThanks,\nJonathan\n"},{"id":"389303","messageId":"87imlo6twj.fsf@igel.home","threadId":"52575","inReplyTo":"20200106213051.GD980197@coredump.intra.peff.net","subject":"Re: [Outreachy] Return value before or after free()?","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2020-01-06T23:34:52Z","receivedAt":"2020-01-06T23:34:57Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Jan 06 2020, Jeff King wrote:\n\n> On Mon, Jan 06, 2020 at 10:15:53PM +0100, Miriam R. wrote:\n>\n>> in run-command.c file `exists_in_PATH()` function does this:\n>> \n>> static int exists_in_PATH(const char *file)\n>> {\n>> char *r = locate_in_PATH(file);\n>> free(r);\n>> return r != NULL;\n>> }\n>> \n>> I wonder if it is correct to do return r != NULL; after free(r);\n>\n> It is technically undefined behavior according to the C standard, but I\n> think it would be hard to find an implementation where it was not\n> perfectly fine in practice.\n\nCompilers get constantly better at exploiting undefined behaviour, so I\nwould not count on it.\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":"389307","messageId":"20200107010809.GH6570@camp.crustytoothpaste.net","threadId":"52575","inReplyTo":"20200106213051.GD980197@coredump.intra.peff.net","subject":"Re: [Outreachy] Return value before or after free()?","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-01-07T01:08:09Z","receivedAt":"2020-01-07T01:08:19Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-01-06 at 21:30:51, Jeff King wrote:\n> On Mon, Jan 06, 2020 at 10:15:53PM +0100, Miriam R. wrote:\n> \n> > in run-command.c file `exists_in_PATH()` function does this:\n> > \n> > static int exists_in_PATH(const char *file)\n> > {\n> > char *r = locate_in_PATH(file);\n> > free(r);\n> > return r != NULL;\n> > }\n> > \n> > I wonder if it is correct to do return r != NULL; after free(r);\n> \n> It is technically undefined behavior according to the C standard, but I\n> think it would be hard to find an implementation where it was not\n> perfectly fine in practice.\n> \n> Ref: http://c-faq.com/malloc/ptrafterfree.html\n> \n> I'd probably leave it alone unless it is causing a problem (e.g., a\n> static analyzer complaining).\n\nUnfortunately, compilers have gotten much more aggressive about assuming\nthat undefined behavior never occurs and rewriting code based on that.\nclang is not as bad about doing that, but GCC is very aggressive about\nit.  There are multiple instances where NULL pointer checks have been\noptimized out because the compiler exploited undefined behavior to\nassume a pointer was never NULL.\n\nIn this case, the only case in which we can safely assume that this\nbehavior is acceptable is that r is NULL, in which case C11 tells us\nthat \"no action occurs\" due to the free. So the compiler could just\noptimize this out to a \"return 0\".  Just because it doesn't now doesn't\nmean we can assume it won't in the future, so we do need to fix this.\n\nI'll send a patch.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"389312","messageId":"20200107015859.GJ6570@camp.crustytoothpaste.net","threadId":"52575","inReplyTo":"20200107010809.GH6570@camp.crustytoothpaste.net","subject":"Re: [Outreachy] Return value before or after free()?","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2020-01-07T01:58:59Z","receivedAt":"2020-01-07T01:59:08Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2020-01-07 at 01:08:09, brian m. carlson wrote:\n> Unfortunately, compilers have gotten much more aggressive about assuming\n> that undefined behavior never occurs and rewriting code based on that.\n> clang is not as bad about doing that, but GCC is very aggressive about\n> it.  There are multiple instances where NULL pointer checks have been\n> optimized out because the compiler exploited undefined behavior to\n> assume a pointer was never NULL.\n> \n> In this case, the only case in which we can safely assume that this\n> behavior is acceptable is that r is NULL, in which case C11 tells us\n> that \"no action occurs\" due to the free. So the compiler could just\n> optimize this out to a \"return 0\".  Just because it doesn't now doesn't\n> mean we can assume it won't in the future, so we do need to fix this.\n> \n> I'll send a patch.\n\nOof, I just realized that you had tagged this with \"[Outreachy]\", which\nmeans that you were probably planning on sending a patch to fix this,\nand then I went and did it instead, so let me apologize for doing that.\n\nI sent it because oftentimes we say \"we should fix this thing\" and then\nnever do it because nobody sends a patch, but in this case I should have\npaid more attention and waited for you to respond and send one instead.\n\nAgain, sorry about that.\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"389424","messageId":"CAN7CjDAZCxEmkvkqOw++VvBwF54cn_S1J+Uem61aNvjA7F3AiQ@mail.gmail.com","threadId":"52575","inReplyTo":"20200107015859.GJ6570@camp.crustytoothpaste.net","subject":"Re: [Outreachy] Return value before or after free()?","fromName":"Miriam R.","fromEmail":"mirucam@gmail.com","sentAt":"2020-01-07T20:40:08Z","receivedAt":"2020-01-07T20:40:20Z","isPatch":false,"sender":{"key":"mirucam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56339109?v=4"},"body":"El mar., 7 ene. 2020 a las 2:59, brian m. carlson\n(<sandals@crustytoothpaste.net>) escribió:\n>\n> On 2020-01-07 at 01:08:09, brian m. carlson wrote:\n> > Unfortunately, compilers have gotten much more aggressive about assuming\n> > that undefined behavior never occurs and rewriting code based on that.\n> > clang is not as bad about doing that, but GCC is very aggressive about\n> > it.  There are multiple instances where NULL pointer checks have been\n> > optimized out because the compiler exploited undefined behavior to\n> > assume a pointer was never NULL.\n> >\n> > In this case, the only case in which we can safely assume that this\n> > behavior is acceptable is that r is NULL, in which case C11 tells us\n> > that \"no action occurs\" due to the free. So the compiler could just\n> > optimize this out to a \"return 0\".  Just because it doesn't now doesn't\n> > mean we can assume it won't in the future, so we do need to fix this.\n> >\n> > I'll send a patch.\n>\n> Oof, I just realized that you had tagged this with \"[Outreachy]\", which\n> means that you were probably planning on sending a patch to fix this,\n> and then I went and did it instead, so let me apologize for doing that.\n>\nSorry for my late reply, but I have been traveling all day.\n\nDon't worry Brian, I am working on finishing bisect-helper conversion\nfrom shell to C.\nI am planning to send a patch related with this function as part of a\npatch series. My patch only changes the static header to be used in\nbisect related files.\nMy mentor (Christian Couder) detected this and suggested me to ask the\ncommunity.\n\n> I sent it because oftentimes we say \"we should fix this thing\" and then\n> never do it because nobody sends a patch, but in this case I should have\n> paid more attention and waited for you to respond and send one instead.\n>\nDon't worry again, next time if my question is related to a patch I am\ngoing to send I will actively write it and this way there will be no\nconfusion. :)\n\n> Again, sorry about that.\n> --\n> brian m. carlson: Houston, Texas, US\n> OpenPGP: https://keybase.io/bk2204\n"}]}