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

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?

Laszlo

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

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);
According to https://docs.python.org/3/c-api/arg.html#building-values,

"O" increments the count of py_array.  So before the Py_BuildValue
call, py_array has a refcount of 1.  I think (although the docs are
unclear) that if Py_BuildValue fails, it rolls back any refcount
changes it makes before returning (since there is no new object
created, nothing exists that needs a second reference) - but you are
indeed left with py_array needing to be cleaned up to avoid a memory
leak.
> > +  if (args == NULL) {
> > +    PyErr_PrintEx (0);
> > +    goto out;
> > +  }
So reducing the reference count here on the error path makes sense.
However, on success, it means py_array now has a refcount of 2, and
args has a refcount of 1 (where cleaning up args will reduce only 1 of
the refs to py_array)...
> > +
> >    Py_INCREF (args);
> > -
> >    py_r = PyObject_CallObject (py_callback, args);
> > -
> >    Py_DECREF (args);
This temporary increment/decrement of args around PyObject_CallObject
is a safety factor to ensure that the call itself doesn't try to
reclaim args while still in use.  I'm not sure if it is strictly
required, but it doesn't hurt.  At any rate, at this point, args is
back to refcount 1.
> > -
> >    if (py_r != NULL)
> >      Py_DECREF (py_r);
> >    else
> >      /* Callback threw an exception: print it. */
> >      PyErr_PrintEx (0);
> >  
> > + out:
> >    PyGILState_Release (py_save);
> >  }
...and we are STILL not releasing our count, of either py_array, or of args.

If I understand Python bindings in C correctly, you want to
unconditionally decrement the refcount of py_array after Py_BuildValue
(either reducing it to 0 because args is NULL and you don't need py_array
anymore, or reducing it to 1 because cleaning up args will take care
of it), and you also need to add a Py_DECREF(args) under out.
> >  
> 
> 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"
Reference transfer is done with "N", not "O".  That would be
an
alternative to decreasing the refcount of py_array on success, but not
eliminate the need to decrease the refcount on Py_BuildValue failure.
> 
> - 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?
Not quite.  "O" is different than "N".

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org