Commit 89af010d ("python: Fix more memory leaks", v1.5.2) tried to patch some python memory leaks on callback error paths. But it missed an obvious one: we are mistakenly calling Py_INCREF on the result of Py_BuildValue, which results in an object that is over-referenced and thus never gets freed, leaking even on successful callbacks. Worse but less likely, if Py_BuildValue fails due to no memory, Py_INCREF(NULL) causes a crash. There are other leaks on early exit paths, all solved by refactoring the callback FOO_wrapper() functions to use an 'out:' label rather than early exits. Finally, note that PyErr_SetObject increments the reference count rather than stealing the object, and thus every time we call raise_exception() (for any API failure, not just from a callback error), we were leaking an object. I tested this by watching the memory usage of: $ nbdkit --no-sr memory 1 $ nbdsh -u nbd://localhost nbd> def f(a, b, c, d, e): ... pass ... nbd> def leak(): ... for i in range(0, 100000): ... try: ... h.block_status(1, 0, f) ... except: ... pass ... then repeatedly using leak(). Fixes: 936488d4 ("python: Implement Callback properly.", v0.1) Fixes: 259d46cb ("python: Raise a custom exception containing error string and errno.", v0.1.6) --- v1: https://listman.redhat.com/archives/libguestfs/2021-November/msg00280.html In v2 - retitle subject, fix more leaks; watching memory usage of nbdsh given the above test now no longer grows in memory as I repeat leak(). generator/Python.ml | 51 ++++++++++++++++++++++++++++++--------------- 1 file changed, 34 insertions(+), 17 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 49281bf..4ab18f6 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -1,6 +1,6 @@ (* hey emacs, this is OCaml code: -*- tuareg -*- *) (* nbd client library in userspace: Python bindings - * Copyright (C) 2013-2020 Red Hat Inc. + * Copyright (C) 2013-2021 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 @@ -61,8 +61,10 @@ raise_exception () { PyObject *args = Py_BuildValue (\"si\", nbd_get_error (), nbd_get_errno ()); - if (args != NULL) + if (args != NULL) { PyErr_SetObject (nbd_internal_py_Error, args); + Py_DECREF (args); + } } "; @@ -161,29 +163,42 @@ let print_python_closure_wrapper { cbname; cbargs } = pr "\n"; pr "{\n"; pr " const struct user_data *data = user_data;\n"; - pr " int ret = 0;\n"; + pr " int ret = -1;\n"; pr "\n"; pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n"; pr " PyObject *py_args, *py_ret;\n"; + List.iter ( + function + | CBArrayAndLen (UInt32 n, _) -> + pr " PyObject *py_%s = NULL;\n" n + | CBMutable (Int n) -> + pr " PyObject *py_%s = NULL;\n" n + | _ -> () + ) cbargs; + pr "\n"; List.iter ( function | CBArrayAndLen (UInt32 n, len) -> - pr " PyObject *py_%s = PyList_New (%s);\n" n len; + pr " py_%s = PyList_New (%s);\n" n len; + pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n; pr " size_t i_%s;\n" n; - pr " for (i_%s = 0; i_%s < %s; ++i_%s)\n" n n len n; - pr " PyList_SET_ITEM (py_%s, i_%s, PyLong_FromUnsignedLong (%s[i_%s]));\n" n n n n + pr " for (i_%s = 0; i_%s < %s; ++i_%s) {\n" n n len n; + pr " PyObject *py_e_%s = PyLong_FromUnsignedLong (%s[i_%s]);\n" n n n; + pr " if (!py_e_%s) { PyErr_PrintEx (0); goto out; }\n" n; + pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n; + pr " }\n" | CBBytesIn _ | CBInt _ | CBInt64 _ -> () | CBMutable (Int n) -> pr " PyObject *py_%s_modname = PyUnicode_FromString (\"ctypes\");\n" n; - pr " if (!py_%s_modname) { PyErr_PrintEx (0); return -1; }\n" n; + pr " if (!py_%s_modname) { PyErr_PrintEx (0); goto out; }\n" n; pr " PyObject *py_%s_mod = PyImport_Import (py_%s_modname);\n" n n; pr " Py_DECREF (py_%s_modname);\n" n; - pr " if (!py_%s_mod) { PyErr_PrintEx (0); return -1; }\n" n; - pr " PyObject *py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\", \"i\", *%s);\n" n n n; + pr " if (!py_%s_mod) { PyErr_PrintEx (0); goto out; }\n" n; + pr " py_%s = PyObject_CallMethod (py_%s_mod, \"c_int\", \"i\", *%s);\n" n n n; pr " Py_DECREF (py_%s_mod);\n" n; - pr " if (!py_%s) { PyErr_PrintEx (0); return -1; }\n" n; + pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n; | CBString _ | CBUInt _ | CBUInt64 _ -> () @@ -216,7 +231,7 @@ let print_python_closure_wrapper { cbname; cbargs } = | CBArrayAndLen _ | CBMutable _ -> assert false ) cbargs; pr ");\n"; - pr " Py_INCREF (py_args);\n"; + pr " if (!py_args) { PyErr_PrintEx (0); goto out; }\n"; pr "\n"; pr " py_save = PyGILState_Ensure ();\n"; pr " py_ret = PyObject_CallObject (data->fn, py_args);\n"; @@ -238,19 +253,21 @@ let print_python_closure_wrapper { cbname; cbargs } = pr " PyErr_Print ();\n"; pr " abort ();\n"; pr " }\n"; - pr " ret = -1;\n"; pr " PyErr_PrintEx (0); /* print exception */\n"; pr " };\n"; pr "\n"; + pr " out:\n"; List.iter ( function | CBArrayAndLen (UInt32 n, _) -> - pr " Py_DECREF (py_%s);\n" n + pr " Py_XDECREF (py_%s);\n" n | CBMutable (Int n) -> - pr " PyObject *py_%s_ret = PyObject_GetAttrString (py_%s, \"value\");\n" n n; - pr " *%s = PyLong_AsLong (py_%s_ret);\n" n n; - pr " Py_DECREF (py_%s_ret);\n" n; - pr " Py_DECREF (py_%s);\n" n + pr " if (py_%s) {\n" n; + pr " PyObject *py_%s_ret = PyObject_GetAttrString (py_%s, \"value\");\n" n n; + pr " *%s = PyLong_AsLong (py_%s_ret);\n" n n; + pr " Py_DECREF (py_%s_ret);\n" n; + pr " Py_DECREF (py_%s);\n" n; + pr " }\n" | CBBytesIn _ | CBInt _ | CBInt64 _ | CBString _ -- 2.33.1