Laszlo Ersek
2021-Nov-30 12:08 UTC
[Libguestfs] [libnbd PATCH 1/2] api: Avoid memory leak on certain strict_mode failures
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 at 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) { >
Eric Blake
2021-Nov-30 13:20 UTC
[Libguestfs] [libnbd PATCH 1/2] api: Avoid memory leak on certain strict_mode failures
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 at 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