Libguestfs - Feb 2023 - [PATCH v3 2/2] python: Avoid leaking py_array and py_args in event callbacks

See also:

https://listman.redhat.com/archives/libguestfs/2023-February/030730.html
https://listman.redhat.com/archives/libguestfs/2023-February/030745.html
https://listman.redhat.com/archives/libguestfs/2023-February/030746.html

Fixes: commit 6ef5837e2d8c5d4d83eff51c0201eb2e08f719de
Thanks: Laszlo Ersek, Eric Blake
---
 python/handle.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/python/handle.c b/python/handle.c
index 8eeabe60a7..85089e6bce 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -134,6 +134,7 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
   args = Py_BuildValue ("(Kiy#O)",
                         (unsigned PY_LONG_LONG) event, event_handle,
                         buf, buf_len, py_array);
+  Py_DECREF (py_array);
   if (args == NULL) {
     PyErr_PrintEx (0);
     goto out;
-- 
2.39.0

On 2/17/23 18:45, Richard W.M. Jones wrote:
> See also:
> 
> https://listman.redhat.com/archives/libguestfs/2023-February/030730.html
> https://listman.redhat.com/archives/libguestfs/2023-February/030745.html
> https://listman.redhat.com/archives/libguestfs/2023-February/030746.html
> 
> Fixes: commit 6ef5837e2d8c5d4d83eff51c0201eb2e08f719de
> Thanks: Laszlo Ersek, Eric Blake
> ---
>  python/handle.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/python/handle.c b/python/handle.c
> index 8eeabe60a7..85089e6bce 100644
> --- a/python/handle.c
> +++ b/python/handle.c
> @@ -134,6 +134,7 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
>    args = Py_BuildValue ("(Kiy#O)",
>                          (unsigned PY_LONG_LONG) event, event_handle,
>                          buf, buf_len, py_array);
> +  Py_DECREF (py_array);
>    if (args == NULL) {
>      PyErr_PrintEx (0);
>      goto out;
I *think* doing it like this is safe in this instance.

I got concerned for a minute because of:

https://docs.python.org/3/c-api/refcounting.html?highlight=py_decref#c.Py_DECREF

    Warning

    The deallocation function can cause arbitrary Python code to be
    invoked (e.g. when a class instance with a __del__() method is
    deallocated). While exceptions in such code are not propagated, the
    executed code has free access to all Python global variables. This
    means that any object that is reachable from a global variable
    should be in a consistent state before Py_DECREF() is invoked. For
    example, code to delete an object from a list should copy a
    reference to the deleted object in a temporary variable, update the
    list data structure, and then call Py_DECREF() for the temporary
    variable.

and:

https://docs.python.org/3/c-api/exceptions.html?highlight=pyerr_printex#c.PyErr_PrintEx

    Call this function only when the error indicator is set. Otherwise
    it will cause a fatal error!

So I had two concerns:

- can Py_DECREF() clear the error indicator?

- can Py_DECREF() lead to the exception (pending form Py_BuildValue())
being replaced (masked?) with a different exception?

I think the code is OK:

- My reading of the docs implies that the error indicator only gets
cleared with explicit PyErr_Clear() or PyErr_PrintEx() calls.

- We're only releasing a simple list of longs, so no particular
exception should be expected while destructing that.

And the testing described in the cover letter confirms this.

Reviewed-by: Laszlo Ersek <lersek at redhat.com>