Eric Blake
2021-Nov-30 15:06 UTC
[Libguestfs] [libnbd PATCH v2] python: Fix more callback memory leaks
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
Richard W.M. Jones
2021-Nov-30 15:42 UTC
[Libguestfs] [libnbd PATCH v2] python: Fix more callback memory leaks
ACK, thanks. We likely need this in RHEL 9 ... I'll take care of that once it's been pushed. 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/