[Libguestfs] [libnbd PATCH 1/2] api: Avoid memory leak on certain strict_mode failures

[Eric Blake]

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