[Libguestfs] [PATCH 1/2] python: Avoid crash if callback parameters cannot be built

[Richard W.M. Jones]

On Thu, Feb 16, 2023 at 03:09:02PM +0100, Laszlo Ersek wrote:
> On 2/14/23 19:51, Richard W.M. Jones wrote:
> > In the case that building the parameters to the Python event callback
> > fails, args was returned as NULL.  We immediately tried to call
> > Py_INCREF on this which crashed.  Returning NULL means the Python
> > function threw an exception, so print the exception and return (there
> > is no way to return an error here - the event is lost).
> > 
> > Reported-by: Yonatan Shtarkman
> > See: https://listman.redhat.com/archives/libguestfs/2023-February/030653.html
> > ---
> >  python/handle.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/python/handle.c b/python/handle.c
> > index c8752baf47..f37e939e03 100644
> > --- a/python/handle.c
> > +++ b/python/handle.c
> > @@ -134,18 +134,21 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
> >    args = Py_BuildValue ("(Kis#O)",
> >                          (unsigned PY_LONG_LONG) event, event_handle,
> >                          buf, buf_len, py_array);
> > +  if (args == NULL) {
> > +    PyErr_PrintEx (0);
> > +    goto out;
> > +  }
> > +
> >    Py_INCREF (args);
> > -
> >    py_r = PyObject_CallObject (py_callback, args);
> > -
> >    Py_DECREF (args);
> > -
> >    if (py_r != NULL)
> >      Py_DECREF (py_r);
> >    else
> >      /* Callback threw an exception: print it. */
> >      PyErr_PrintEx (0);
> >  
> > + out:
> >    PyGILState_Release (py_save);
> >  }
> >  
> 
> My understanding of object references in this function is the following:
> 
> - PyList_New creates a new object / new reference "py_array"
> 
> - Py_BuildValue with the "O" format specifier transfers the new list's
> *sole* reference (= ownership) to the just-built higher-level object "args"
> 
> - when "args" is killed (decref'd), it takes care of "py_array".
> 
> Consequently, if Py_BuildValue fails, "py_array" continues owning the
> new list -- and I believe that, if we take the new error branch, we leak
> the object pointed-to by "py_array". Is that the case?

Yes I think it would leak.  Sent the fix as another patch.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v