Re: [PATCH 11/26] cat-file: fix memory leak

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Apr 28, 2017, 09:59 UTC
Message-ID: <alpine.DEB.2.20.1704281155520.3480@virtualbox>
In-Reply-To: <xmqqo9vimldf.fsf@gitster.mtv.corp.google.com>

Hi Junio,

On Wed, 26 Apr 2017, Junio C Hamano wrote:

Show 25 quoted lines

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > Discovered by Coverity.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/cat-file.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> > index 1890d7a6390..9af863e7915 100644
> > --- a/builtin/cat-file.c
> > +++ b/builtin/cat-file.c
> > @@ -165,6 +165,7 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name,
> >  		die("git cat-file %s: bad file", obj_name);
> >  
> >  	write_or_die(1, buf, size);
> > +	free(buf);
> >  	return 0;
> >  }
> 
> This is a border-line "Meh".  Just like we do not free resources
> immediately before calling die(), we can leave this as-is as the
> only thing that happens after this is a return from cmd_cat_file()
> back to main() that exits.

If you are mostly concerned about the status quo, that is true.

I am a lot more concerned with future changes, where we may easily decide that it is time to move a file-local function out of its hiding place and make it more usable.

From that perspective, it is one thing to have a blatant memory leak in a cmd_*() function, and it is an entirely different matter to have such a leak in a function that happens to be called only from cmd_*() functions: somebody familiar enough with Git's coding conventions (such as myself) will *expect* cmd_*() to have leaks left and right and pay attention when libifying that code, but be a lot less concerned about such leaks in other functions.

And of course this concerns me more than you, as I am still trying to drive forward the effort to convert more scripts into builtins.

So on my own behalf: thank you for accepting this patch.

Ciao, Dscho