Re: [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(a)redhat.com&gt; > > (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.