Re: [PATCH 2/6] commit: copy saved getenv() result

From: Jeff King <peff@peff.net>
Date: Jan 15, 2019, 19:17 UTC
Message-ID: <20190115191714.GD4886@sigill.intra.peff.net>
In-Reply-To: <nycvar.QRO.7.76.6.1901151502130.41@tvgsbejvaqbjf.bet>

On Tue, Jan 15, 2019 at 03:05:50PM +0100, Johannes Schindelin wrote:

Show 7 quoted lines

> > But certainly it would not be the first such instance in our code base.
> 
> Just because a lot of our code has grown historically does not mean that
> we need to add code that shares the same shortcomings. FREE_AND_NULL() was
> not available for a long time, after all, so it is understandable that we
> did not use it back then. But it is available now, so we no longer have an
> excuse to add less defensive code.

Fair enough. I am happy to start using FREE_AND_NULL() consistently if nobody things it's too opaque (or that it creates confusion that we somehow expect to look at the variable again and need it to be NULL). I think the compiler should generally be able to optimize out the NULL assignment in most cases anyway.

> > In theory a static analyzer should easily be able to figure out such a
> > problem, too, so maybe it is not worth being defensive about.
> 
> How often do you run a static analyzer?

Stefan was routinely running coverity, though I haven't seen results in a while. I think we should make sure that continues, as it did turn up some useful results (and a lot of cruft, too, but on the whole I have found it useful).

I also count gcc as a static analyzer, since it can and does point out many simple problems. I don't know that it can point out an obvious user-after-free like this, though.

That said....

> My point being: if we can prevent future mistakes easily, and it does not
> add too much code churn, why not just do it. No need to rely on fancy
> stuff that might not even be available on your preferred platform.

Yeah, I do agree (which is why I included it in the patch ;) ).

-Peff