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

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.

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

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

Hi,

Jeff King wrote:

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

On Jan 06 2020, Jeff King wrote:

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

Andreas.

On 2020-01-06 at 21:30:51, Jeff King 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.

On 2020-01-07 at 01:08:09, brian m. carlson wrote:

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.

El mar., 7 ene. 2020 a las 2:59, brian m. carlson (<sandals@crustytoothpaste.net>) escribió:

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.

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. :)