Libguestfs - Sep 2020 - [libnbd PATCH] python: Fix more memory leaks

h.nbd_connect_command was leaking a python object per parameter, and
also leaks memory on failure.  In fact, it was possible to segfault on
something as trivial as:
nbdsh -c 'h.connect_command(["true",1])'

h.nbd_pread was leaking a read buffer on every single synchronous read.

My previous patch to address closure callbacks had a bug in
h.nbd_pread_structured and similar: if the allocation for Closure
fails, we hit 'goto err' prior to initializing the OClosure pointers,
which could lead to freeing garbage.  Similarly, when messing with
non-callable for either the Closure or the OClosure, there was enough
inaccuracy in python reference counting to cause a leak or
double-free.

We were not consistently checking for python function failures (such
as when a wrong type is passed in).

While touching this, drop a pointless cast to char* when calling
PyArg_ParseTuple, and rearrange some of the code for fewer loops over
args/optargs in the generator.

Fixes: 4fab2104ab
Fixes: bf0eee111f
Fixes: 82dac49af0
---

I'm pushing this as followup to my previous patch.

I'm pretty embarrassed that we've let nbdsh leak as many bytes as were
read via h.pread(), all the way since v0.1; that sort of leakage ought
to be easier to detect.  But using valgrind on python is a bit
difficult, to ferret out the real leaks vs. the python garbage
collection reference chain.

 generator/Python.ml | 103 +++++++++++++++++++++-----------------------
 python/handle.c     |   5 ++-
 python/utils.c      |  11 ++++-
 3 files changed, 63 insertions(+), 56 deletions(-)

diff --git a/generator/Python.ml b/generator/Python.ml
index 9a22f9e..3b86dc0 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -271,7 +271,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }      | BytesIn (n, _) ->
        pr "  Py_buffer %s;\n" n
     | BytesOut (n, count) ->
-       pr "  char *%s;\n" n;
+       pr "  char *%s = NULL;\n" n;
        pr "  Py_ssize_t %s;\n" count
     | BytesPersistIn (n, _)
     | BytesPersistOut (n, _) ->
@@ -279,11 +279,10 @@ let print_python_binding name { args; optargs; ret;
may_set_error }            n;
        pr "  struct py_aio_buffer *%s_buf;\n" n
     | Closure { cbname } ->
-       pr "  struct user_data *%s_user_data = alloc_user_data ();\n"
cbname;
-       pr "  if (%s_user_data == NULL) goto out;\n" cbname;
+       pr "  struct user_data *%s_user_data = NULL;\n" cbname;
+       pr "  PyObject *py_%s_fn;\n" cbname;
        pr "  nbd_%s_callback %s = { .callback = %s_wrapper,\n"
          cbname cbname cbname;
-       pr "                         .user_data = %s_user_data,\n"
cbname;
        pr "                         .free = free_user_data };\n"
     | Enum (n, _) -> pr "  int %s;\n" n
     | Flags (n, _) ->
@@ -316,11 +315,10 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    List.iter (
     function
     | OClosure { cbname } ->
-       pr "  struct user_data *%s_user_data = alloc_user_data ();\n"
cbname;
-       pr "  if (%s_user_data == NULL) goto out;\n" cbname;
+       pr "  struct user_data *%s_user_data = NULL;\n" cbname;
+       pr "  PyObject *py_%s_fn;\n" cbname;
        pr "  nbd_%s_callback %s = { .callback = %s_wrapper,\n"
          cbname cbname cbname;
-       pr "                         .user_data = %s_user_data,\n"
cbname;
        pr "                         .free = free_user_data };\n"
     | OFlags (n, _) ->
        pr "  uint32_t %s_u32;\n" n;
@@ -329,7 +327,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    pr "\n";

   (* Parse the Python parameters. *)
-  pr "  if (!PyArg_ParseTuple (args, (char *) \"O\"";
+  pr "  if (!PyArg_ParseTuple (args, \"O\"";
   List.iter (
     function
     | Bool n -> pr " \"p\""
@@ -364,7 +362,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }      | BytesIn (n, _) | BytesPersistIn (n, _)
     | BytesPersistOut (n, _) -> pr ", &%s" n
     | BytesOut (_, count) -> pr ", &%s" count
-    | Closure { cbname } -> pr ", &%s_user_data->fn" cbname
+    | Closure { cbname } -> pr ", &py_%s_fn" cbname
     | Enum (n, _) -> pr ", &%s" n
     | Flags (n, _) -> pr ", &%s" n
     | Fd n | Int n -> pr ", &%s" n
@@ -379,30 +377,57 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    ) args;
   List.iter (
     function
-    | OClosure { cbname } -> pr ", &%s_user_data->fn"
cbname
+    | OClosure { cbname } -> pr ", &py_%s_fn" cbname
     | OFlags (n, _) -> pr ", &%s" n
   ) optargs;
   pr "))\n";
   pr "    goto out;\n";

   pr "  h = get_handle (py_h);\n";
+  pr "  if (!h) goto out;\n";
+  List.iter (
+    function
+    | OClosure { cbname } ->
+       pr "  %s.user_data = %s_user_data = alloc_user_data ();\n"
cbname cbname;
+       pr "  if (%s_user_data == NULL) goto out;\n" cbname;
+       pr "  if (py_%s_fn != Py_None) {\n" cbname;
+       pr "    if (!PyCallable_Check (py_%s_fn)) {\n" cbname;
+       pr "      PyErr_SetString (PyExc_TypeError,\n";
+       pr "                       \"callback parameter %s is not
callable\");\n" cbname;
+       pr "      goto out;\n";
+       pr "    }\n";
+       pr "    /* Increment refcount since pointer may be saved by libnbd.
*/\n";
+       pr "    Py_INCREF (py_%s_fn);\n" cbname;
+       pr "    %s_user_data->fn = py_%s_fn;\n" cbname cbname;
+       pr "  }\n";
+       pr "  else\n";
+       pr "    %s.callback = NULL; /* we're not going to call it
*/\n" cbname
+    | OFlags (n, _) -> pr "  %s_u32 = %s;\n" n n
+  ) optargs;
   List.iter (
     function
     | Bool _ -> ()
     | BytesIn _ -> ()
     | BytesOut (n, count) ->
-       pr "  %s = malloc (%s);\n" n count
+       pr "  %s = malloc (%s);\n" n count;
+       pr "  if (%s == NULL) { PyErr_NoMemory (); goto out; }\n" n
     | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
-       pr "  %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n
+       pr "  %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n;
+       pr "  if (!%s_buf) goto out;\n" n;
+       pr "  /* Increment refcount since buffer may be saved by libnbd.
*/\n";
+       pr "  Py_INCREF (%s);\n" n;
+       pr "  completion_user_data->buf = %s;\n" n
     | Closure { cbname } ->
-       pr "  /* Increment refcount since pointer may be saved by libnbd.
*/\n";
-       pr "  if (!PyCallable_Check (%s_user_data->fn)) {\n"
cbname;
+       pr "  %s.user_data = %s_user_data = alloc_user_data ();\n"
cbname cbname;
+       pr "  if (%s_user_data == NULL) goto out;\n" cbname;
+       pr "  if (!PyCallable_Check (py_%s_fn)) {\n" cbname;
        pr "    PyErr_SetString (PyExc_TypeError,\n";
        pr "                     \"callback parameter %s is not
callable\");\n" cbname;
-       pr "    %s_user_data->fn = NULL;\n" cbname;
        pr "    goto out;\n";
        pr "  }\n";
-       pr "  Py_INCREF (%s_user_data->fn);\n" cbname
+       pr "  /* Increment refcount since pointer may be saved by libnbd.
*/\n";
+       pr "  Py_INCREF (py_%s_fn);\n" cbname;
+       pr "  %s_user_data->fn = py_%s_fn;\n" cbname cbname
     | Enum _ -> ()
     | Flags (n, _) -> pr "  %s_u32 = %s;\n" n n
     | Fd _ | Int _ -> ()
@@ -420,37 +445,6 @@ let print_python_binding name { args; optargs; ret;
may_set_error }      | UInt32 n -> pr "  %s_u32 = %s;\n" n n
     | UInt64 n -> pr "  %s_u64 = %s;\n" n n
   ) args;
-  List.iter (
-    function
-    | OClosure { cbname } ->
-       pr "  if (%s_user_data->fn != Py_None) {\n" cbname;
-       pr "    /* Increment refcount since pointer may be saved by libnbd.
*/\n";
-       pr "    if (!PyCallable_Check (%s_user_data->fn)) {\n"
cbname;
-       pr "      PyErr_SetString (PyExc_TypeError,\n";
-       pr "                       \"callback parameter %s is not
callable\");\n" cbname;
-       pr "      %s_user_data->fn = NULL;\n" cbname;
-       pr "      goto out;\n";
-       pr "    }\n";
-       pr "    Py_INCREF (%s_user_data->fn);\n" cbname;
-       pr "  }\n";
-       pr "  else\n";
-       pr "    %s.callback = NULL; /* we're not going to call it
*/\n" cbname
-    | OFlags (n, _) -> pr "  %s_u32 = %s;\n" n n
-  ) optargs;
-
-  (* If there is a BytesPersistIn/Out parameter then we need to
-   * increment the refcount and save the pointer into
-   * completion_callback.user_data so we can decrement the
-   * refcount on command completion.
-   *)
-  List.iter (
-    function
-    | BytesPersistIn (n, _) | BytesPersistOut (n, _) ->
-       pr "  /* Increment refcount since buffer may be saved by libnbd.
*/\n";
-       pr "  Py_INCREF (%s);\n" n;
-       pr "  completion_user_data->buf = %s;\n" n;
-    | _ -> ()
-    ) args;
   pr "\n";

   (* Call the underlying C function. *)
@@ -547,9 +541,10 @@ let print_python_binding name { args; optargs; ret;
may_set_error }      function
     | Bool _ -> ()
     | BytesIn (n, _) -> pr "  PyBuffer_Release (&%s);\n" n
-    | BytesPersistIn _ | BytesOut _ | BytesPersistOut _ -> ()
+    | BytesOut (n, _) -> pr "  free (%s);\n" n
+    | BytesPersistIn _ | BytesPersistOut _ -> ()
     | Closure { cbname } ->
-       pr "  if (%s_user_data) free_user_data (%s_user_data);\n"
cbname cbname
+       pr "  free_user_data (%s_user_data);\n" cbname
     | Enum _ -> ()
     | Flags _ -> ()
     | Fd _ | Int _ -> ()
@@ -566,7 +561,7 @@ let print_python_binding name { args; optargs; ret;
may_set_error }    List.iter (
     function
     | OClosure { cbname } ->
-       pr "  if (%s_user_data) free_user_data (%s_user_data);\n"
cbname cbname
+       pr "  free_user_data (%s_user_data);\n" cbname
     | OFlags _ -> ()
   ) optargs;
   pr "  return py_ret;\n";
@@ -613,9 +608,11 @@ let generate_python_methods_c ()    pr "{\n";
   pr "  struct user_data *data = user_data;\n";
   pr "\n";
-  pr "  Py_XDECREF (data->fn);\n";
-  pr "  Py_XDECREF (data->buf);\n";
-  pr "  free (data);\n";
+  pr "  if (data) {\n";
+  pr "    Py_XDECREF (data->fn);\n";
+  pr "    Py_XDECREF (data->buf);\n";
+  pr "    free (data);\n";
+  pr "  }\n";
   pr "}\n";
   pr "\n";

diff --git a/python/handle.c b/python/handle.c
index 408a140..12c9c9c 100644
--- a/python/handle.c
+++ b/python/handle.c
@@ -1,5 +1,5 @@
 /* NBD client library in userspace
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -91,7 +91,8 @@ free_aio_buffer (PyObject *capsule)
 {
   struct py_aio_buffer *buf = PyCapsule_GetPointer (capsule, aio_buffer_name);

-  free (buf->data);
+  if (buf)
+    free (buf->data);
   free (buf);
 }

diff --git a/python/utils.c b/python/utils.c
index e0929dc..0e3164c 100644
--- a/python/utils.c
+++ b/python/utils.c
@@ -60,15 +60,24 @@ nbd_internal_py_get_string_list (PyObject *obj)

   for (i = 0; i < len; ++i) {
     PyObject *bytes = PyUnicode_AsUTF8String (PyList_GetItem (obj, i));
+    if (!bytes)
+      goto err;
     r[i] = strdup (PyBytes_AS_STRING (bytes));
+    Py_DECREF (bytes);
     if (r[i] == NULL) {
       PyErr_NoMemory ();
-      return NULL;
+      goto err;
     }
   }
   r[len] = NULL;

   return r;
+
+ err:
+  while (i--)
+    free (r[i]);
+  free (r);
+  return NULL;
 }

 void
-- 
2.28.0

Thanks for this.  I'll do a 1.4.1 release later and try to
get as much of this into RHEL too.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/