Re: [PATCH] mailmap: fix check for freeing memory

From: Jeff King <peff@peff.net>
Date: Aug 20, 2013, 13:52 UTC
Message-ID: <20130820135230.GA32370@sigill.intra.peff.net>
In-Reply-To: <878uzw7a6l.fsf@linux-k42r.v.cablecom.net>

On Tue, Aug 20, 2013 at 03:40:02PM +0200, Thomas Rast wrote:

Show 13 quoted lines

> Stefan Beller <stefanbeller@googlemail.com> writes:
> 
> > The condition as it is written in that line was most likely intended to
> > check for the pointer passed to free(), rather than checking for the
> > 'repo_abbrev', which is already checked against being non null at the
> > beginning of the function.
> [...]
> > -			if (repo_abbrev)
> > +			if (*repo_abbrev)
> >  				free(*repo_abbrev);
> 
> But now the test is useless, because free(NULL) is defined to be a
> no-op.

Yeah, I think we should just drop the conditional completely.

I am not sure of the complete back-story. The earlier check for repo_abbrev to be non-NULL was added by 8503ee4, after this check on free() already existed. So that was when this conditional became redundant.

But the line right after this one unconditionally assigns to "*repo_abbrev", so we would always segfault in such a case anyway (which is what 8503ee4 was fixing).

So I think it was either a misguided "don't pass NULL to free" check that was simply wrong, or it was an incomplete "make sure repo_abbrev is not NULL" check. And the first is useless, and the second is now redundant due to 8503ee4. So it should simply be free().

-Peff