7:15 p.m.
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