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

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);
 }
 
-- 
2.39.0

The event callback gets a buffer parameter which is usually something
like a log message.  However as it comes from C it is not necessarily
well-formed (eg) UTF-8 but could contain any old byte sequence.

In the test case provided by the reported, we failed to encode the
buffer as str with this error:

UnicodeDecodeError: 'utf-8' codec can't decode byte 0xc3 in position
137: unexpected end of data

Use bytes instead.

Reported-by: Yonatan Shtarkman
See: https://listman.redhat.com/archives/libguestfs/2023-February/030653.html
---
 python/handle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/python/handle.c b/python/handle.c
index f37e939e03..bf639b5789 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -131,7 +131,7 @@ guestfs_int_py_event_callback_wrapper (guestfs_h *g,
   }
 
   /* XXX As with Perl we don't pass the guestfs_h handle here. */
-  args = Py_BuildValue ("(Kis#O)",
+  args = Py_BuildValue ("(Kiy#O)",
                         (unsigned PY_LONG_LONG) event, event_handle,
                         buf, buf_len, py_array);
   if (args == NULL) {
-- 
2.39.0

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