Re: [PATCH 5/5] Add missing incref when creating Inferior Python object

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-01-23 17:40, Simon Marchi wrote:
> The test py-inferior.exp fails when using my debug build of Python 3.6.
> I don't see it failing with my system's default Python, but it might be
> related to the different memory allocation scheme used when doing a
> build with pydebug.
> 
> The issue is that we are missing a Py_INCREF in
> inferior_to_inferior_object.  The PyObject_New function initializes the
> object with a refcount of 1.  If we assume that this refcount
> corresponds to the reference we are keeping in the inferior data, then
> we are missing an incref for the reference we are returning.  We can
> also see it the other way.  If the refcount added by PyObject_New is 
> for
> the reference we are returning, then we are missing one for the 
> inferior
> data.
> 
> The counterpart for this incref is in py_free_inferior.
> 
> Here's how I can get it to crash:
> 
>   $ ./gdb -nx -ex "set debug python 1"
>   (gdb) add-inferior
>   Added inferior 2
>   (gdb) python infs = gdb.inferiors()
>   Creating Python Inferior object inf = 1
>   Creating Python Inferior object inf = 2
>   (gdb) remove-inferiors 2
>   py_free_inferior inf = 2
>   infpy_dealloc inf = <unknown>
>   (gdb) python infs = None
>   Fatal Python error: Objects/tupleobject.c:243 object at
> 0x7f9cf1a568d8 has negative ref count -1
> 
>   Current thread 0x00007f9cf1b68780 (most recent call first):
>     File "<string>", line 1 in <module>
>   [1]    408 abort (core dumped)  ./gdb -nx -ex "set debug python 1"
> 
> After having created the inferiors object, their refcount is 1 (which
> comes from PyObject_New), but it should be two.  The gdb inferior 
> object
> has a reference and the "infs" list has a reference.
> 
> When invoking remove-inferiors, py_free_inferior gets called.  It does
> the decref that corresponds to the reference that the gdb inferior
> object kept.  At this moment, the refcount drops to 0 and the object
> gets deallocated, even though the "infs" list still has a reference.
> When we set "infs" to None, Python tries to decref the already zero
> refcount and the assert triggers.
> 
> With this patch, it looks better:
> 
>   (gdb) add-inferior
>   Added inferior 2
>   (gdb) python infs = gdb.inferiors()
>   Creating Python Inferior object inf = 1
>   Creating Python Inferior object inf = 2
>   (gdb) remove-inferiors 2
>   py_free_inferior inf = 2
>   (gdb) python infs = None
>   infpy_dealloc inf = <unknown>
> 
> gdb/ChangeLog:
> 
> 	* python/py-inferior.c (inferior_to_inferior_object): Manually
> 	increment reference count when creating the object as well.
> ---
>  gdb/python/py-inferior.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
> index 340dddcfbd..24ef4f0ec8 100644
> --- a/gdb/python/py-inferior.c
> +++ b/gdb/python/py-inferior.c
> @@ -227,10 +227,13 @@ inferior_to_inferior_object (struct inferior 
> *inferior)
>        inf_obj->threads = NULL;
>        inf_obj->nthreads = 0;
> 
> +      /* PyObject_New initializes the new object with a refcount of 1. 
>  This
> +	 counts for the reference we are keeping in the inferior data.  */
>        set_inferior_data (inferior, infpy_inf_data_key, inf_obj);
>      }
> -  else
> -    Py_INCREF ((PyObject *)inf_obj);
> +
> +  /* We are returning a new reference.  */
> +  Py_INCREF (inf_obj);
> 
>    return gdbpy_inf_ref (inf_obj);
>  }

Ping for this patch only.  It's actually not dependent on the rest of 
the series and fixes an actual bug, so I think it could go in by itself.