[libnbd PATCH 0/2] Memory leak fixes
While working on 64-bit block status, I tried using nbdsh to run a known-failing h.block_status() in a loop, and was surprised that memory ran away, even though I remember having worked on plugging python memory leaks a while back. So here's a couple more places that need plugging. Patch 1 is ready to go. Patch 2 helps, but is incomplete; I'm posting it now because it's my bedtime. Eric Blake (2): api: Avoid memory leak on certain strict_mode failures python: Fix memory leak on callback error generator/Python.ml | 4 ++-- lib/rw.c | 4 ++-- tests/errors.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 48 insertions(+), 5 deletions(-) -- 2.33.1
Commit e57681fa ("generator: Free closures on failure", v1.5.2) makes sure that once we copy a callback out of the caller's variable, then we ensure it is cleaned up on all error paths. But I was developing two patch series in parallel, and due to botched rebasing on my part, shortly after, commit 76bc0f0b ("api: Add STRICT_BOUNDS/ZERO_SIZE to nbd_set_strict_mode", v1.5.3) accidentally re-introduced a return -1 instead of a goto err on one of its two added error checks in the common handler, and that mistake was then copy-pasted into ba86bfd1 ("api: Add STRICT_ALIGN to nbd_set_strict_mode", v1.5.3). As penance for reintroducing a leak so quickly back then, I'm now adding some testsuite coverage, which fails without the rw.c changes. Fixes: 76bc0f0b Fixes: ba86bfd1 --- lib/rw.c | 4 ++-- tests/errors.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 3 deletions(-) diff --git a/lib/rw.c b/lib/rw.c index cb25dbf..4ade750 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -195,13 +195,13 @@ nbd_internal_command_common (struct nbd_handle *h, if ((h->strict & LIBNBD_STRICT_BOUNDS) && (offset > h->exportsize || count > h->exportsize - offset)) { set_error (count_err, "request out of bounds"); - return -1; + goto err; } if (h->block_minimum && (h->strict & LIBNBD_STRICT_ALIGN) && (offset | count) & (h->block_minimum - 1)) { set_error (EINVAL, "request is unaligned"); - return -1; + goto err; } } diff --git a/tests/errors.c b/tests/errors.c index f099c7c..2c800c7 100644 --- a/tests/errors.c +++ b/tests/errors.c @@ -1,5 +1,5 @@ /* NBD client library in userspace - * 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 @@ -103,6 +103,29 @@ check_server_fail (struct nbd_handle *h, int64_t cookie, check (experr, "nbd_aio_command_completed: "); } +static bool chunk_clean; /* whether check_chunk has been called */ +static bool completion_clean; /* whether check_completion has been called */ + +static void +check_chunk (void *data) { + if (chunk_clean) { + fprintf (stderr, "%s: test failed: " + "chunk callback invoked multiple times\n", progname); + exit (EXIT_FAILURE); + } + chunk_clean = true; +} + +static void +check_completion (void *data) { + if (completion_clean) { + fprintf (stderr, "%s: test failed: " + "completion callback invoked multiple times\n", progname); + exit (EXIT_FAILURE); + } + completion_clean = true; +} + int main (int argc, char *argv[]) { @@ -277,6 +300,26 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } check (EINVAL, "nbd_aio_pread: "); + + /* We guarantee callbacks will be freed even on all error paths. */ + if (nbd_aio_pread_structured (nbd, buf, 512, -1, + (nbd_chunk_callback) { .free = check_chunk, }, + (nbd_completion_callback) { + .free = check_completion, }, + 0) != -1) { + fprintf (stderr, "%s: test failed: " + "nbd_aio_pread_structured did not fail with bogus offset\n", + argv[0]); + exit (EXIT_FAILURE); + } + check (EINVAL, "nbd_aio_pread_structured: "); + if (!chunk_clean || !completion_clean) { + fprintf (stderr, "%s: test failed: " + "callbacks not freed on nbd_aio_pread_structured failure\n", + argv[0]); + exit (EXIT_FAILURE); + } + /* Read from an invalid offset, server-side */ strict &= ~LIBNBD_STRICT_BOUNDS; if (nbd_set_strict_mode (nbd, strict) == -1) { -- 2.33.1
On 11/30/21 04:15, Eric Blake wrote:
Commit e57681fa ("generator: Free closures on failure", v1.5.2) makes sure that once we copy a callback out of the caller's variable, then we ensure it is cleaned up on all error paths. But I was developing two patch series in parallel, and due to botched rebasing on my part, shortly after, commit 76bc0f0b ("api: Add STRICT_BOUNDS/ZERO_SIZE to nbd_set_strict_mode", v1.5.3) accidentally re-introduced a return -1 instead of a goto err on one of its two added error checks in the common handler, and that mistake was then copy-pasted into ba86bfd1 ("api: Add STRICT_ALIGN to nbd_set_strict_mode", v1.5.3).
As penance for reintroducing a leak so quickly back then, I'm now adding some testsuite coverage, which fails without the rw.c changes.
Fixes: 76bc0f0b Fixes: ba86bfd1 --- lib/rw.c | 4 ++-- tests/errors.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/lib/rw.c b/lib/rw.c index cb25dbf..4ade750 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -195,13 +195,13 @@ nbd_internal_command_common (struct nbd_handle *h, if ((h->strict & LIBNBD_STRICT_BOUNDS) && (offset > h->exportsize || count > h->exportsize - offset)) { set_error (count_err, "request out of bounds"); - return -1; + goto err; }
if (h->block_minimum && (h->strict & LIBNBD_STRICT_ALIGN) && (offset | count) & (h->block_minimum - 1)) { set_error (EINVAL, "request is unaligned"); - return -1; + goto err; } }
One of those (rare) cases where I think that "ownership transfer on error" is justified. Acked-by: Laszlo Ersek <lersek@redhat.com> (It would be nice though if "lib/internal.h" documented the unconditional ownership transfer in nbd_internal_command_common(). Or is that clear already from another part of the documentation, or a header file?) Thanks, Laszlo
diff --git a/tests/errors.c b/tests/errors.c index f099c7c..2c800c7 100644 --- a/tests/errors.c +++ b/tests/errors.c @@ -1,5 +1,5 @@ /* NBD client library in userspace - * 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 @@ -103,6 +103,29 @@ check_server_fail (struct nbd_handle *h, int64_t cookie, check (experr, "nbd_aio_command_completed: "); }
+static bool chunk_clean; /* whether check_chunk has been called */ +static bool completion_clean; /* whether check_completion has been called */ + +static void +check_chunk (void *data) { + if (chunk_clean) { + fprintf (stderr, "%s: test failed: " + "chunk callback invoked multiple times\n", progname); + exit (EXIT_FAILURE); + } + chunk_clean = true; +} + +static void +check_completion (void *data) { + if (completion_clean) { + fprintf (stderr, "%s: test failed: " + "completion callback invoked multiple times\n", progname); + exit (EXIT_FAILURE); + } + completion_clean = true; +} + int main (int argc, char *argv[]) { @@ -277,6 +300,26 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } check (EINVAL, "nbd_aio_pread: "); + + /* We guarantee callbacks will be freed even on all error paths. */ + if (nbd_aio_pread_structured (nbd, buf, 512, -1, + (nbd_chunk_callback) { .free = check_chunk, }, + (nbd_completion_callback) { + .free = check_completion, }, + 0) != -1) { + fprintf (stderr, "%s: test failed: " + "nbd_aio_pread_structured did not fail with bogus offset\n", + argv[0]); + exit (EXIT_FAILURE); + } + check (EINVAL, "nbd_aio_pread_structured: "); + if (!chunk_clean || !completion_clean) { + fprintf (stderr, "%s: test failed: " + "callbacks not freed on nbd_aio_pread_structured failure\n", + argv[0]); + exit (EXIT_FAILURE); + } + /* Read from an invalid offset, server-side */ strict &= ~LIBNBD_STRICT_BOUNDS; if (nbd_set_strict_mode (nbd, strict) == -1) {
On Tue, Nov 30, 2021 at 01:08:24PM +0100, Laszlo Ersek wrote:
On 11/30/21 04:15, Eric Blake wrote:
Commit e57681fa ("generator: Free closures on failure", v1.5.2) makes sure that once we copy a callback out of the caller's variable, then we ensure it is cleaned up on all error paths. But I was developing two patch series in parallel, and due to botched rebasing on my part, shortly after, commit 76bc0f0b ("api: Add STRICT_BOUNDS/ZERO_SIZE to nbd_set_strict_mode", v1.5.3) accidentally re-introduced a return -1 instead of a goto err on one of its two added error checks in the common handler, and that mistake was then copy-pasted into ba86bfd1 ("api: Add STRICT_ALIGN to nbd_set_strict_mode", v1.5.3).
As penance for reintroducing a leak so quickly back then, I'm now adding some testsuite coverage, which fails without the rw.c changes.
Fixes: 76bc0f0b Fixes: ba86bfd1 --- lib/rw.c | 4 ++-- tests/errors.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/lib/rw.c b/lib/rw.c index cb25dbf..4ade750 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -195,13 +195,13 @@ nbd_internal_command_common (struct nbd_handle *h, if ((h->strict & LIBNBD_STRICT_BOUNDS) && (offset > h->exportsize || count > h->exportsize - offset)) { set_error (count_err, "request out of bounds"); - return -1; + goto err; }
if (h->block_minimum && (h->strict & LIBNBD_STRICT_ALIGN) && (offset | count) & (h->block_minimum - 1)) { set_error (EINVAL, "request is unaligned"); - return -1; + goto err; } }
One of those (rare) cases where I think that "ownership transfer on error" is justified.
Acked-by: Laszlo Ersek <lersek@redhat.com>
(It would be nice though if "lib/internal.h" documented the unconditional ownership transfer in nbd_internal_command_common().
It sort of does (in the function body, rather than as a contract at the declaration): https://gitlab.com/nbdkit/libnbd/-/blob/master/lib/rw.c#L283 | err: | /* Since we did not queue the command, we must free the callbacks. */
Or is that clear already from another part of the documentation, or a header file?)
It is also in the documentation: https://gitlab.com/nbdkit/libnbd/-/blob/master/docs/libnbd.pod#L815 |=head2 Callback lifetimes | |You can associate an optional free function with callbacks. Libnbd |will call this function when the callback will not be called again by |libnbd, including in the case where the API fails. So I've pushed this one as commit 76f4d5af5c1c, with a bit more verbosity in the commit message. I'll backport it to stable branches and probably cut a minor release later today, once I have the python leak fixed as well. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On 11/30/21 14:20, Eric Blake wrote:
On Tue, Nov 30, 2021 at 01:08:24PM +0100, Laszlo Ersek wrote:
On 11/30/21 04:15, Eric Blake wrote:
Commit e57681fa ("generator: Free closures on failure", v1.5.2) makes sure that once we copy a callback out of the caller's variable, then we ensure it is cleaned up on all error paths. But I was developing two patch series in parallel, and due to botched rebasing on my part, shortly after, commit 76bc0f0b ("api: Add STRICT_BOUNDS/ZERO_SIZE to nbd_set_strict_mode", v1.5.3) accidentally re-introduced a return -1 instead of a goto err on one of its two added error checks in the common handler, and that mistake was then copy-pasted into ba86bfd1 ("api: Add STRICT_ALIGN to nbd_set_strict_mode", v1.5.3).
As penance for reintroducing a leak so quickly back then, I'm now adding some testsuite coverage, which fails without the rw.c changes.
Fixes: 76bc0f0b Fixes: ba86bfd1 --- lib/rw.c | 4 ++-- tests/errors.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/lib/rw.c b/lib/rw.c index cb25dbf..4ade750 100644 --- a/lib/rw.c +++ b/lib/rw.c @@ -195,13 +195,13 @@ nbd_internal_command_common (struct nbd_handle *h, if ((h->strict & LIBNBD_STRICT_BOUNDS) && (offset > h->exportsize || count > h->exportsize - offset)) { set_error (count_err, "request out of bounds"); - return -1; + goto err; }
if (h->block_minimum && (h->strict & LIBNBD_STRICT_ALIGN) && (offset | count) & (h->block_minimum - 1)) { set_error (EINVAL, "request is unaligned"); - return -1; + goto err; } }
One of those (rare) cases where I think that "ownership transfer on error" is justified.
Acked-by: Laszlo Ersek <lersek@redhat.com>
(It would be nice though if "lib/internal.h" documented the unconditional ownership transfer in nbd_internal_command_common().
It sort of does (in the function body, rather than as a contract at the declaration): https://gitlab.com/nbdkit/libnbd/-/blob/master/lib/rw.c#L283 | err: | /* Since we did not queue the command, we must free the callbacks. */
Or is that clear already from another part of the documentation, or a header file?)
It is also in the documentation: https://gitlab.com/nbdkit/libnbd/-/blob/master/docs/libnbd.pod#L815 |=head2 Callback lifetimes | |You can associate an optional free function with callbacks. Libnbd |will call this function when the callback will not be called again by |libnbd, including in the case where the API fails.
Perfect!
So I've pushed this one as commit 76f4d5af5c1c, with a bit more verbosity in the commit message.
I'll backport it to stable branches and probably cut a minor release later today, once I have the python leak fixed as well.
Thanks! Laszlo
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. Worse but less likely, if Py_BuildValue fails due to no memory, Py_INCREF(NULL) causes a crash. 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) --- Hmm, I'm still seeing a leak even with this applied, so I'm missing yet another problem. generator/Python.ml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 49281bf..cb6c6b8 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 @@ -216,7 +216,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); return -1; }\n"; pr "\n"; pr " py_save = PyGILState_Ensure ();\n"; pr " py_ret = PyObject_CallObject (data->fn, py_args);\n"; -- 2.33.1
On 11/30/21 04:15, Eric Blake wrote:
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. Worse but less likely, if Py_BuildValue fails due to no memory, Py_INCREF(NULL) causes a crash.
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) ---
Hmm, I'm still seeing a leak even with this applied, so I'm missing yet another problem.
I feel I know too little to review this patch, plus if there is another known-to-exist leak in the python binding, shouldn't those patches both become their own series? (Of course if Rich ACKs this, I have nothing against it.) Thanks, Laszlo
generator/Python.ml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml index 49281bf..cb6c6b8 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 @@ -216,7 +216,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); return -1; }\n"; pr "\n"; pr " py_save = PyGILState_Ensure ();\n"; pr " py_ret = PyObject_CallObject (data->fn, py_args);\n";
participants (2)
-
Eric Blake -
Laszlo Ersek