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

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

On Mon, Jan 2, 2017 at 8:22 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

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