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