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

login
register
mail settings
Submitter Kevin Bullock
Date June 12, 2014, 3:53 a.m.
Message ID <79CD695C-F509-41FA-9A64-36A8B8872E56@ringworld.org>
Download mbox | patch
Permalink /patch/4981/
State Superseded
Commit 8da100383dc32c1f12dcc3ce8dd4ea38c6281158
Headers show

Comments

Sean Farley - June 12, 2014, 4:44 a.m.
Kevin Bullock writes:

> On Jun 11, 2014, at 5:43 PM, danek.duvall@oracle.com wrote:
>
>> # HG changeset patch
>> # User Danek Duvall <danek.duvall@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/
Gregory Szorc - June 12, 2014, 5:18 a.m.
On 6/11/2014 9:44 PM, Sean Farley wrote:
> 
> Kevin Bullock writes:
> 
>> On Jun 11, 2014, at 5:43 PM, danek.duvall@oracle.com wrote:
>>
>>> # HG changeset patch
>>> # User Danek Duvall <danek.duvall@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/
Siddharth Agarwal - June 12, 2014, 10:04 p.m.
On 06/11/2014 09:44 PM, Sean Farley wrote:
> Kevin Bullock writes:
>
>> On Jun 11, 2014, at 5:43 PM, danek.duvall@oracle.com wrote:
>>
>>> # HG changeset patch
>>> # User Danek Duvall <danek.duvall@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.
Sean Farley - June 12, 2014, 10:14 p.m.
Siddharth Agarwal writes:

> On 06/11/2014 09:44 PM, Sean Farley wrote:
>> Kevin Bullock writes:
>>
>>> On Jun 11, 2014, at 5:43 PM, danek.duvall@oracle.com wrote:
>>>
>>>> # HG changeset patch
>>>> # User Danek Duvall <danek.duvall@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.

Sorry. I actually agree with both you and Greg. I just couldn't help
make the reference to xkcd :-)

Patch

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;
+        }
 
 	dict = PyDict_New();
 	if (dict == NULL)
@@ -1427,20 +1429,17 @@  static PyObject *find_deepest(indexObjec
 	}
 
 	keys = PyDict_Keys(dict);
+        goto cleanup;
 
+bail:
+        keys = NULL;
+cleanup:
 	free(depth);
 	free(seen);
 	free(interesting);
 	Py_DECREF(dict);
 
 	return keys;
-bail:
-	free(depth);
-	free(seen);
-	free(interesting);
-	Py_XDECREF(dict);
-
-	return NULL;
 }
 
 /*