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

From: Stefan Beller <sbeller@google.com>
Date: Jan 4, 2017, 18:09 UTC
Message-ID: <CAGZ79kbNYZpYh9fMDKEhMiO5L8SbbtSC8f79RAa-wsOUxE8LDA@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.20.1701032231400.3469@virtualbox>

On Tue, Jan 3, 2017 at 1:33 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 7 quoted lines

>
> This patch was originally only to appease Coverity, but it actually *does*
> plug a very real memory leak: previously, *every* call to git_exec_path()
> *possibly* returned a newly-malloc()ed buffer. Now, the first call will
> store that pointer in a static variable and reuse it later.
>
> Could you maybe help me with improving the commit message?

As someone not familiar with that area of code, this explained it enough for me to understand, so maybe:

    exec_cmd: do not leak via git_exec_path

    Every call to git_exec_path() possibly returned a newly-malloc()ed
    buffer. Now, the first call will allocate the buffer and subsequent
    calls return a pointer to it, which then prevents leaking memory
    on each call.

The return value of a "const char *" hints to the caller, that the memory is not owned by the caller, do we need to be explicit there (i.e. a comment declaring the memory ownership? Probably not.)

Thanks, Stefan