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

[Jeff King <peff@peff.net>]

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

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