Re: [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()

[Jeff King <peff@peff.net>]

On Tue, Jan 03, 2017 at 12:11:25PM -0800, Stefan Beller wrote:

> On Mon, Jan 2, 2017 at 8:22 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > Technically, it is correct that git_exec_path() returns a possibly
> > malloc()ed string. Practically, it is *sometimes* not malloc()ed. So
> > let's just use a static variable to make it a singleton. That'll shut
> > Coverity up, hopefully.
> 
> I picked up this patch and applied it to the coverity branch
> that I maintain at github/stefanbeller/git.
> 
> I'd love to see this patch upstream as it reduces my maintenance
> burden of the coverity branch by a patch.

There is another lurking issue in that function, which is that the
return value of getenv() is not guaranteed to last beyond more calls to
getenv() or setenv(). It should probably xstrdup() that result, too.

At that point 2 out of 3 of the return cases would be malloc'd strings,
so we _could_ switch the third and say "caller must free the result".
But I think I prefer something like Dscho's solution (more on that
below).

> Early on when Git was new to coverity, some arguments were made
> that patches like these only clutter the main code base which is read
> by a lot of people, hence we want these quirks for coverity not upstream.
> And I think that still holds.
> 
> If this patch is only to appease coverity (as the commit message eludes
> to) I think this may be a bad idea for upstream.  If this patch fixes an
> actual problem, then the commit message needs to spell that out.

This is a real leak, though in all cases the program typically exits
soon afterwards. But we leak from list_commands(), for example, and it
is not immediately obvious that this is only called right before
exiting.

But I think more important is that caching the result in a static
variable communicates something (both to Coverity and to a reader of the
code). This is a value we expect to live through the life of the
program, and it is OK for it to "leak" when it goes out of scope by the
program exiting.

So even though the behavior does not really change, the annotation has
value.

-Peff