Eric Blake
2021-Nov-30 03:15 UTC
[Libguestfs] [libnbd PATCH 1/2] api: Avoid memory leak on certain strict_mode failures
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
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) { >