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:
signature.asc
Description: PGP signature
