Re: [PATCH v4 1/4] Avoid Coverity warning about unfree()d git_exec_path()
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Jan 3, 2017, 21:33 UTC
- Message-ID
- <alpine.DEB.2.20.1701032231400.3469@virtualbox>
- In-Reply-To
- <CAGZ79kZ--jp08pK+xwn1N2VQQr8bA5+DveE2HsoY90R1gR6c_A@mail.gmail.com>
Hi Stefan,
On Tue, 3 Jan 2017, Stefan Beller wrote:
Show 21 quoted lines
> 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. > > 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 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?
Ciao, Dscho