[PATCH stable] parsers.c: fix a couple of memory leaks

Gregory Szorc gregory.szorc at gmail.com
Thu Jun 12 00:18:35 CDT 2014


On 6/11/2014 9:44 PM, Sean Farley wrote:
> 
> Kevin Bullock writes:
> 
>> On Jun 11, 2014, at 5:43 PM, danek.duvall at oracle.com wrote:
>>
>>> # HG changeset patch
>>> # User Danek Duvall <danek.duvall at oracle.com>
>>> # Date 1402525864 25200
>>> #      Wed Jun 11 15:31:04 2014 -0700
>>> # Branch stable
>>> # Node ID b091b262aa6d25e7e4663672daf2b6d8f127ab48
>>> # Parent  b35f8c487e396487e89f98e92da57ac5eb9833af
>>> parsers.c: fix a couple of memory leaks
>>>
>>> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
>>> --- a/mercurial/parsers.c
>>> +++ b/mercurial/parsers.c
>>> @@ -1403,8 +1403,12 @@ static PyObject *find_deepest(indexObjec
>>>  		final |= i;
>>>  		j -= 1;
>>>  	}
>>> -	if (final == 0)
>>> +	if (final == 0) {
>>> +		free(depth);
>>> +		free(seen);
>>> +		free(interesting);
>>>  		return PyList_New(0);
>>> +	}
>>
>> Good catch, but this is now the 3rd place we've got those same `free()`s in this function. Would something like this work instead:
>>
>> diff --git a/mercurial/parsers.c b/mercurial/parsers.c
>> --- a/mercurial/parsers.c
>> +++ b/mercurial/parsers.c
>> @@ -1403,8 +1403,10 @@ static PyObject *find_deepest(indexObjec
>>  		final |= i;
>>  		j -= 1;
>>  	}
>> -	if (final == 0)
>> -		return PyList_New(0);
>> +	if (final == 0) {
>> +                keys = PyList_New(0);
>> +                goto cleanup;
> 
> http://xkcd.com/292/

I support the use of goto in C for intra-function jumping to cleanup
code. The alternatives are duplicated code (leading to sync issues and
bugs like what this patch is fixing), macros (which IMO are harder to
read than goto), or a separate function for cleanup (annoying to
maintain, function call overhead).

http://onlinehut.org/2011/10/goto-is-not-evil-okay/


More information about the Mercurial-devel mailing list