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

Siddharth Agarwal sid at less-broken.com
Thu Jun 12 17:04:42 CDT 2014


On 06/11/2014 09: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/

As Greg said, goto is absolutely the right choice for error handling in 
C. I generally don't consider forward gotos to be a problem. It is 
*backward* gotos that get really hairy.


More information about the Mercurial-devel mailing list