[Python] Patch to fix memory leak

[Python] Patch to fix memory leak

[Paul Koning]

PyList_Append increments the reference count of the item being appended to the list, and two of the places in the GDB Python code that use it don't take that into account.  The result is a memory leak.  The attached patch fixes that.

Tested by using a debug build of Python with reference counting enabled, and turning on dumping of final leftover objects.

I don't have commit privs.

	paul

ChangeLog:

2011-09-06  Paul Koning  <pkoning@equallogic.com>

	* python/py-cmd.c (gdbpy_string_to_argv): Decrement reference
	count of item appended to list.
	* python/py-type.c (typy_fields): Likewise.

Index: python/py-cmd.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-cmd.c,v
retrieving revision 1.15
diff -u -r1.15 py-cmd.c
--- python/py-cmd.c	5 Aug 2011 14:24:10 -0000	1.15
+++ python/py-cmd.c	6 Sep 2011 17:50:48 -0000
@@ -685,14 +685,12 @@
 	  if (argp == NULL
 	      || PyList_Append (py_argv, argp) < 0)
 	    {
-	      if (argp != NULL)
-		{
-		  Py_DECREF (argp);
-		}
+	      Py_XDECREF (argp);
 	      Py_DECREF (py_argv);
 	      freeargv (c_argv);
 	      return NULL;
 	    }
+	  Py_DECREF (argp);
 	}
 
       freeargv (c_argv);
Index: python/py-type.c
===================================================================
RCS file: /cvs/src/src/gdb/python/py-type.c,v
retrieving revision 1.20
diff -u -r1.20 py-type.c
--- python/py-type.c	18 Aug 2011 16:17:39 -0000	1.20
+++ python/py-type.c	6 Sep 2011 17:50:48 -0000
@@ -246,6 +246,7 @@
 	  Py_DECREF (result);
 	  return NULL;
 	}
+      Py_DECREF (dict);
     }
 
   return result;

Re: [Python] Patch to fix memory leak

[Phil Muldoon]

Paul Koning <paulkoning@comcast.net> writes:

> PyList_Append increments the reference count of the item being
> appended to the list, and two of the places in the GDB Python code
> that use it don't take that into account.  The result is a memory
> leak.  The attached patch fixes that.

For some reason the Python API manual is silent about PyList_Append
incrementing the reference count.  But I did check, and you are correct,
the reference count is incremented when added to a list.  

> Tested by using a debug build of Python with reference counting
> enabled, and turning on dumping of final leftover objects.

This is fine, thanks for doing this.  I cannot give you check-in
permission as I am not a maintainer.  Please wait until one of the
maintainers signs-off before you go ahead.

Cheers,

Phil

Re: [Python] Patch to fix memory leak

[Jan Kratochvil]

On Wed, 07 Sep 2011 11:27:24 +0200, Phil Muldoon wrote:
> This is fine, thanks for doing this.  I cannot give you check-in
> permission as I am not a maintainer.  Please wait until one of the
> maintainers signs-off before you go ahead.

I agree with both mails, check it in, please.


Thanks,
Jan

Re: [Python] Patch to fix memory leak

[Paul Koning]

On Sep 7, 2011, at 5:27 AM, Phil Muldoon wrote:

> Paul Koning <paulkoning@comcast.net> writes:
> 
>> PyList_Append increments the reference count of the item being
>> appended to the list, and two of the places in the GDB Python code
>> that use it don't take that into account.  The result is a memory
>> leak.  The attached patch fixes that.
> 
> For some reason the Python API manual is silent about PyList_Append
> incrementing the reference count.  But I did check, and you are correct,
> the reference count is incremented when added to a list.  

Apart from looking at the source, I find the most helpful statement is in the C API document, Introduction, Reference Count details, which says that when you pass an object as an argument the called function may "steal" the reference, or not -- and few functions steal the reference.  (For that matter, few functions return borrowed references.)  So the general rule is that passed objects are not stolen, and returned objects are new references.  Stolen and borrowed references are mentioned explicitly in the documentation of the function in question, and silence means not borrowed, not stolen.

>> Tested by using a debug build of Python with reference counting
>> enabled, and turning on dumping of final leftover objects.
> 
> This is fine, thanks for doing this.  I cannot give you check-in
> permission as I am not a maintainer.  Please wait until one of the
> maintainers signs-off before you go ahead.

I don't have write privs so I'm relying on someone else to do the commit once approved.

	paul

Re: [Python] Patch to fix memory leak

[Sergio Durigan Junior]

Paul Koning <paulkoning@comcast.net> writes:

> I don't have write privs so I'm relying on someone else to do the commit once approved.

Did you have more patches approved?  If so, I believe you can apply for
a copyright assignment (thus gaining commit rights).  If that is the
case, please send me an email offlist and I'll forward the form to you.

Regards.

PING: [Python] Patch to fix memory leak

[Paul Koning]

Ping...

I can commit this now but I need approval first.

	paul


Begin forwarded message:

> From: Paul Koning <paulkoning@comcast.net>
> Date: September 6, 2011 2:01:02 PM EDT
> To: gdb-patches@sourceware.org
> Subject: [Python] Patch to fix memory leak
> 
> PyList_Append increments the reference count of the item being appended to the list, and two of the places in the GDB Python code that use it don't take that into account.  The result is a memory leak.  The attached patch fixes that.
> 
> Tested by using a debug build of Python with reference counting enabled, and turning on dumping of final leftover objects.
> 
> I don't have commit privs.
> 
> 	paul
> 
> ChangeLog:
> 
> 2011-09-06  Paul Koning  <pkoning@equallogic.com>
> 
> 	* python/py-cmd.c (gdbpy_string_to_argv): Decrement reference
> 	count of item appended to list.
> 	* python/py-type.c (typy_fields): Likewise.
> 
> Index: python/py-cmd.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/python/py-cmd.c,v
> retrieving revision 1.15
> diff -u -r1.15 py-cmd.c
> --- python/py-cmd.c	5 Aug 2011 14:24:10 -0000	1.15
> +++ python/py-cmd.c	6 Sep 2011 17:50:48 -0000
> @@ -685,14 +685,12 @@
> 	  if (argp == NULL
> 	      || PyList_Append (py_argv, argp) < 0)
> 	    {
> -	      if (argp != NULL)
> -		{
> -		  Py_DECREF (argp);
> -		}
> +	      Py_XDECREF (argp);
> 	      Py_DECREF (py_argv);
> 	      freeargv (c_argv);
> 	      return NULL;
> 	    }
> +	  Py_DECREF (argp);
> 	}
> 
>       freeargv (c_argv);
> Index: python/py-type.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/python/py-type.c,v
> retrieving revision 1.20
> diff -u -r1.20 py-type.c
> --- python/py-type.c	18 Aug 2011 16:17:39 -0000	1.20
> +++ python/py-type.c	6 Sep 2011 17:50:48 -0000
> @@ -246,6 +246,7 @@
> 	  Py_DECREF (result);
> 	  return NULL;
> 	}
> +      Py_DECREF (dict);
>     }
> 
>   return result;
> 

Re: PING: [Python] Patch to fix memory leak

[Jan Kratochvil]

On Thu, 15 Sep 2011 20:03:31 +0200, Paul Koning wrote:
> Ping...
> 
> I can commit this now but I need approval first.

It was approved:
	http://sourceware.org/ml/gdb-patches/2011-09/msg00111.html


Thanks,
Jan

Re: PING: [Python] Patch to fix memory leak

[Paul Koning]

On Sep 15, 2011, at 2:08 PM, Jan Kratochvil wrote:

> On Thu, 15 Sep 2011 20:03:31 +0200, Paul Koning wrote:
>> Ping...
>> 
>> I can commit this now but I need approval first.
> 
> It was approved:
> 	http://sourceware.org/ml/gdb-patches/2011-09/msg00111.html
> 

I missed that.  Thanks.  Committed.

	paul