Libguestfs - Feb 2023 - python: Avoid leaking py_array and py_args in event callbacks

Version 1 was here:

  https://listman.redhat.com/archives/libguestfs/2023-February/030732.html

Following Eric's suggestion here:

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

let's decrement the reference of py_array right after adding it to the
args.  (This works even if args fails to be built).

However the other part of Eric's suggestion is wrong as it ends up
calling Py_DECREF (args) when args == NULL along the error path.  This
lead me to look more closely at this patch:

  https://listman.redhat.com/archives/libguestfs/2019-January/020346.html

which I believe is wrong (at least, the part that fiddles with the
reference to args).  I cannot reproduce the original problem, nor can
I find any justification by looking at the documentation of
PyObject_CallObject.

So we start by reverting that commit.

Rich.

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

Sorry, ignore this, I found yet another problem.  Will post v3.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On 2/17/23 18:39, Richard W.M. Jones wrote:
> Version 1 was here:
> 
>   https://listman.redhat.com/archives/libguestfs/2023-February/030732.html
> 
> Following Eric's suggestion here:
> 
>   https://listman.redhat.com/archives/libguestfs/2023-February/030746.html
> 
> let's decrement the reference of py_array right after adding it to the
> args.  (This works even if args fails to be built).
I agree; (not having seen your new patch) I'd come to the same
conclusion after reading Eric's comment. We need to drop the initial
reference on py_array unconditionally -- if we succeed constructing the
tuple, it takes ownership, so we should drop our original (temporary)
reference; and if the tuple construction fails, there's nobody to foist
py_array upon, so we need to drop it just the same.
> 
> However the other part of Eric's suggestion is wrong as it ends up
> calling Py_DECREF (args) when args == NULL along the error path.  This
> lead me to look more closely at this patch:
> 
>   https://listman.redhat.com/archives/libguestfs/2019-January/020346.html
> 
> which I believe is wrong (at least, the part that fiddles with the
> reference to args).  I cannot reproduce the original problem, nor can
> I find any justification by looking at the documentation of
> PyObject_CallObject.
> 
> So we start by reverting that commit.
Yup, when I first looked at your v1 patch, git-blame had led me to
commit 85235aec8377 ("python: fix call of Python handlers of events",
2019-01-22), and I found the Py_INCREF() call dubious.

Laszlo