Re: [PATCH] run-command: avoid undefined behavior in exists_in_PATH
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 7, 2020, 16:58 UTC
- Message-ID
- <xmqqzhez5hlq.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20200107110145.GA1073219@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 28 quoted lines
> On Tue, Jan 07, 2020 at 01:36:40AM +0000, brian m. carlson wrote: > >> In this function, we free the pointer we get from locate_in_PATH and >> then check whether it's NULL. However, this is undefined behavior if >> the pointer is non-NULL, since the C standard no longer permits us to >> use a valid pointer after freeing it. >> >> The only case in which the C standard would permit this to be defined >> behavior is if r were NULL, since it states that in such a case "no >> action occurs" as a result of calling free. >> >> It's easy to suggest that this is not likely to be a problem, but we >> know that GCC does aggressively exploit the fact that undefined >> behavior can never occur to optimize and rewrite code, even when that's >> contrary to the expectations of the programmer. It is, in fact, very >> common for it to omit NULL pointer checks, just as we have here. > > OK, I agree it makes sense to be on the safe side here (and the patch is > obviously the right fix). > >> Noticed-by: Miriam R. <mirucam@gmail.com> >> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > > I think Miriam actually posted the same patch in her initial email: > > https://lore.kernel.org/git/CAN7CjDDBA0ZoCG9aaQf5rg3gxqny=EjR6v6jE1mnxvUJQSF_0Q@mail.gmail.com/ > > I don't know how we want to handle authorship.
I think the explanation in the log message has as much value as, if not more than, the actual patch text, in this case. Noticed-by: may be striking the right balance.
Thanks, all.