[Outreachy] Return value before or after free()?

[Outreachy] Return value before or after free()?

[Miriam R.]

Dear all,
in run-command.c file `exists_in_PATH()` function does this:

static int exists_in_PATH(const char *file)
{
char *r = locate_in_PATH(file);
free(r);
return r != NULL;
}

I wonder if it is correct to do return r != NULL; after free(r);

Could be this version more readable? :

static int exists_in_PATH(const char *file)
{
char *r = locate_in_PATH(file);
int res = (r != NULL);
free(r);
return res;
}

Could you please give me your feedback?

Thank you!

Best,
Miriam.

Re: [Outreachy] Return value before or after free()?

[Jeff King]

On Mon, Jan 06, 2020 at 10:15:53PM +0100, Miriam R. wrote:

> in run-command.c file `exists_in_PATH()` function does this:
> 
> static int exists_in_PATH(const char *file)
> {
> char *r = locate_in_PATH(file);
> free(r);
> return r != NULL;
> }
> 
> I wonder if it is correct to do return r != NULL; after free(r);

It is technically undefined behavior according to the C standard, but I
think it would be hard to find an implementation where it was not
perfectly fine in practice.

Ref: http://c-faq.com/malloc/ptrafterfree.html

I'd probably leave it alone unless it is causing a problem (e.g., a
static analyzer complaining).

-Peff

Re: [Outreachy] Return value before or after free()?

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Mon, Jan 06, 2020 at 10:15:53PM +0100, Miriam R. wrote:

>> in run-command.c file `exists_in_PATH()` function does this:
>>
>> static int exists_in_PATH(const char *file)
>> {
>>	char *r = locate_in_PATH(file);
>>	free(r);
>>	return r != NULL;
>> }
>>
>> I wonder if it is correct to do return r != NULL; after free(r);
>
> It is technically undefined behavior according to the C standard, but I
> think it would be hard to find an implementation where it was not
> perfectly fine in practice.
>
> Ref: http://c-faq.com/malloc/ptrafterfree.html
>
> I'd probably leave it alone unless it is causing a problem (e.g., a
> static analyzer complaining).

Today I learned.

Miriam, do you have more context?  Did you notice this while reading or
did a tool bring it to your attention?

(Because I was curious, here's what I chased down in C99:

7.20.3.2 "The free function" says: "The free function causes the space
pointed to by ptr to be deallocated, that is, made available for
further allocation."

6.2.4 "Storage durations of objects" says: "The value of a pointer
becomes indeterminate when the object it points to reaches the end of
its lifetime."

6.2.5 "Types" says: "A pointer type describes an object whose value
provides a reference to an entity of the referenced type."

6.5.9 "Equality operators": "Two pointers compare equal if and only if
both are null pointers, both are pointers to the same object
(including a pointer to an object and a subobject at its beginning) or
function, both are pointers to one past the last element of the same
array object, or one is a pointer to one past the end of one array
object and the other is a pointer to the start of a different array
object that happens to immediately follow the first array object in
the address space."

J.2 "Undefined behavior": "The behavior is undefined in the following
circumstances: [...] The value of an object with automatic storage
duration is used while it is indeterminate (6.2.4, 6.7.8, 6.8)"

The reference to automatic storage duration there is interesting.  Of
course `r` here does have automatic storage duration, but the
distinction from

	char **r = xmalloc(sizeof(*r));
	*r = locate_in_PATH(file);
	free(*r);
	/* leak r */
	return *r != NULL;

is peculiar.  It looks like exists_in_PATH is indeed producing
undefined behavior, but the intention of the standard was probably to
make the behavior implementation defined.)

Thanks,
Jonathan

Re: [Outreachy] Return value before or after free()?

[Andreas Schwab]

On Jan 06 2020, Jeff King wrote:

> On Mon, Jan 06, 2020 at 10:15:53PM +0100, Miriam R. wrote:
>
>> in run-command.c file `exists_in_PATH()` function does this:
>> 
>> static int exists_in_PATH(const char *file)
>> {
>> char *r = locate_in_PATH(file);
>> free(r);
>> return r != NULL;
>> }
>> 
>> I wonder if it is correct to do return r != NULL; after free(r);
>
> It is technically undefined behavior according to the C standard, but I
> think it would be hard to find an implementation where it was not
> perfectly fine in practice.

Compilers get constantly better at exploiting undefined behaviour, so I
would not count on it.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [Outreachy] Return value before or after free()?

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

On 2020-01-06 at 21:30:51, Jeff King wrote:
> On Mon, Jan 06, 2020 at 10:15:53PM +0100, Miriam R. wrote:
> 
> > in run-command.c file `exists_in_PATH()` function does this:
> > 
> > static int exists_in_PATH(const char *file)
> > {
> > char *r = locate_in_PATH(file);
> > free(r);
> > return r != NULL;
> > }
> > 
> > I wonder if it is correct to do return r != NULL; after free(r);
> 
> It is technically undefined behavior according to the C standard, but I
> think it would be hard to find an implementation where it was not
> perfectly fine in practice.
> 
> Ref: http://c-faq.com/malloc/ptrafterfree.html
> 
> I'd probably leave it alone unless it is causing a problem (e.g., a
> static analyzer complaining).

Unfortunately, compilers have gotten much more aggressive about assuming
that undefined behavior never occurs and rewriting code based on that.
clang is not as bad about doing that, but GCC is very aggressive about
it.  There are multiple instances where NULL pointer checks have been
optimized out because the compiler exploited undefined behavior to
assume a pointer was never NULL.

In this case, the only case in which we can safely assume that this
behavior is acceptable is that r is NULL, in which case C11 tells us
that "no action occurs" due to the free. So the compiler could just
optimize this out to a "return 0".  Just because it doesn't now doesn't
mean we can assume it won't in the future, so we do need to fix this.

I'll send a patch.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [Outreachy] Return value before or after free()?

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1395 bytes --]

On 2020-01-07 at 01:08:09, brian m. carlson wrote:
> Unfortunately, compilers have gotten much more aggressive about assuming
> that undefined behavior never occurs and rewriting code based on that.
> clang is not as bad about doing that, but GCC is very aggressive about
> it.  There are multiple instances where NULL pointer checks have been
> optimized out because the compiler exploited undefined behavior to
> assume a pointer was never NULL.
> 
> In this case, the only case in which we can safely assume that this
> behavior is acceptable is that r is NULL, in which case C11 tells us
> that "no action occurs" due to the free. So the compiler could just
> optimize this out to a "return 0".  Just because it doesn't now doesn't
> mean we can assume it won't in the future, so we do need to fix this.
> 
> I'll send a patch.

Oof, I just realized that you had tagged this with "[Outreachy]", which
means that you were probably planning on sending a patch to fix this,
and then I went and did it instead, so let me apologize for doing that.

I sent it because oftentimes we say "we should fix this thing" and then
never do it because nobody sends a patch, but in this case I should have
paid more attention and waited for you to respond and send one instead.

Again, sorry about that.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [Outreachy] Return value before or after free()?

[Miriam R.]

El mar., 7 ene. 2020 a las 2:59, brian m. carlson
(<sandals@crustytoothpaste.net>) escribió:
>
> On 2020-01-07 at 01:08:09, brian m. carlson wrote:
> > Unfortunately, compilers have gotten much more aggressive about assuming
> > that undefined behavior never occurs and rewriting code based on that.
> > clang is not as bad about doing that, but GCC is very aggressive about
> > it.  There are multiple instances where NULL pointer checks have been
> > optimized out because the compiler exploited undefined behavior to
> > assume a pointer was never NULL.
> >
> > In this case, the only case in which we can safely assume that this
> > behavior is acceptable is that r is NULL, in which case C11 tells us
> > that "no action occurs" due to the free. So the compiler could just
> > optimize this out to a "return 0".  Just because it doesn't now doesn't
> > mean we can assume it won't in the future, so we do need to fix this.
> >
> > I'll send a patch.
>
> Oof, I just realized that you had tagged this with "[Outreachy]", which
> means that you were probably planning on sending a patch to fix this,
> and then I went and did it instead, so let me apologize for doing that.
>
Sorry for my late reply, but I have been traveling all day.

Don't worry Brian, I am working on finishing bisect-helper conversion
from shell to C.
I am planning to send a patch related with this function as part of a
patch series. My patch only changes the static header to be used in
bisect related files.
My mentor (Christian Couder) detected this and suggested me to ask the
community.

> I sent it because oftentimes we say "we should fix this thing" and then
> never do it because nobody sends a patch, but in this case I should have
> paid more attention and waited for you to respond and send one instead.
>
Don't worry again, next time if my question is related to a patch I am
going to send I will actively write it and this way there will be no
confusion. :)

> Again, sorry about that.
> --
> brian m. carlson: Houston, Texas, US
> OpenPGP: https://keybase.io/bk2204