gdb/python: fix memory leak in python inferior code

When a user creates a gdb.Inferior object for the first time a new
Python object is created.  This object is then cached within GDB's
inferior object using the registry mechanism (see
inferior_to_inferior_object in py-inferior.c, specifically the calls
to inferior_data and set_inferior_data).

The Python Reference to the gdb.Inferior object held within the real
inferior object ensures that the reference count on the Python
gdb.Inferior object never reaches zero while the GDB inferior object
continues to exist.

At the same time, the gdb.Inferior object maintains a C++ pointer back
to GDB's real inferior object.  We therefore end up with a system that
looks like this:

                   Python Reference
                         |
                         |
    .----------.         |          .--------------.
    |          |------------------->|              |
    | inferior |                    | gdb.Inferior |
    |          |<-------------------|              |
    '----------'         |          '--------------'
                         |
                         |
                    C++ Pointer

When GDB's inferior object is deleted (say the inferior exits) then
py_free_inferior is called (thanks to the registry system), this
function looks up the Python gdb.Inferior object and sets the C++
pointer to nullptr and finally reduces the reference count on the
Python gdb.Inferior object.

If at this point the user still holds a reference to the Python
gdb.Inferior object then nothing happens.  However, the gdb.Inferior
object is now in the non-valid state (see infpy_is_valid in
py-inferior.c), but otherwise, everything is fine.

However, if there are no further references to the Python gdb.Inferior
object, or, once the user has given up all their references to the
gdb.Inferior object, then infpy_dealloc is called.

This function currently checks to see if the inferior pointer within
the gdb.Inferior object is nullptr or not.  If the pointer is nullptr
then infpy_dealloc immediately returns.

Only when the inferior point in the gdb.Inferior is not nullptr do
we (a) set the gdb.Inferior reference inside GDB's inferior to
nullptr, and (b) call the underlying Python tp_free function.

There are a number things wrong here:

  1.  The Python gdb.Inferior reference within GDB's inferior object
  holds a reference count, thus, setting this reference to nullptr
  without first decrementing the reference count would leak a
  reference, however...

  2. As GDB's inferior holds a reference then infpy_dealloc will never
  be called until GDB's inferior object is deleted.  Deleting a GDB
  inferior ohject calls py_free_inferior, and so gives up the
  reference.  At this point there is no longer a need to call
  set_inferior_data to set the field back to NULL, that field must
  have been cleared in order to get the reference count to zero, which
  means...

  3. If we know that py_free_inferior must be called before
  infpy_dealloc, then we know that the inferior pointer in
  gdb.Inferior will always be nullptr when infpy_dealloc is called,
  this means that the call to the underlying tp_free function will
  always be skipped.  Skipping this call will cause Python to leak the
  memory associated with the gdb.Inferior object, which is what we
  currently always do.

Given all of the above, I assert that the C++ pointer within
gdb.Inferior will always be nullptr when infpy_dealloc is called.
That's what this patch does.

I wrote a test for this issue making use of Pythons tracemalloc
module, which allows us to spot this memory leak.

1 files changed, 12 insertions, 4 deletions

diff --git a/gdb/python/py-inferior.c b/gdb/python/py-inferior.c
index 0659c28..c8de41d 100644
--- a/gdb/python/py-inferior.c
+++ b/gdb/python/py-inferior.c
@@ -864,12 +864,20 @@ static void
 infpy_dealloc (PyObject *obj)
 {
   inferior_object *inf_obj = (inferior_object *) obj;
-  struct inferior *inf = inf_obj->inferior;
 
-  if (! inf)
-    return;
+  /* The inferior itself holds a reference to this Python object, which
+     will keep the reference count of this object above zero until GDB
+     deletes the inferior and py_free_inferior is called.
+
+     Once py_free_inferior has been called then the link between this
+     Python object and the inferior is set to nullptr, and then the
+     reference count on this Python object is decremented.
+
+     The result of all this is that the link between this Python object and
+     the inferior should always have been set to nullptr before this
+     function is called.  */
+  gdb_assert (inf_obj->inferior == nullptr);
 
-  set_inferior_data (inf, infpy_inf_data_key, NULL);
   Py_TYPE (obj)->tp_free (obj);
 }