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

[Laszlo Ersek]

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