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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 6, 2020, 22:47 UTC
Message-ID: <20200106224747.GA92456@google.com>
In-Reply-To: <20200106213051.GD980197@coredump.intra.peff.net>

Hi,

Jeff King wrote:

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

Show 19 quoted lines

>> 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