[Libguestfs] [nbdkit PATCH] server: Saner filter .close calls

[Richard W.M. Jones]

On Wed, Sep 18, 2019 at 10:07:22AM -0500, Eric Blake wrote:
> I found a core dump:
> 
> term1$ nbdkit -fv --filter=xz null xz-max-depth=$((0x4000000000000000))
> term1...
> term2$ qemu-nbd --list
> term1...
> nbdkit: debug: null: open readonly=1
> nbdkit: error: calloc: Cannot allocate memory
> nbdkit: debug: xz: close
> Segmentation fault (core dumped)
> 
> (Note that the use of --run or daemonizing nbdkit makes it a bit
> harder to see the core dump, but it is still happening.)
> 
> This happens because xz's .open fails after null's has succeeded, so
> the cleanup code sees that it needs to call the .close chain (based
> solely on whether the plugin's handle is non-NULL).  However, our code
> would call into a filter's .close even if handle was NULL (this is
> because we did not distinguish between a filter that failed .open,
> like xz, vs. a filter that lacks an override for .open).  And calling
> xz.close(NULL) causes a SIGSEGV.  Of course, we could patch the xz
> filter to paper over this by short-circuiting if handle is NULL, but a
> more generic fix to this is to make filters.c always set a non-NULL
> handle on .open success, then only call .close when the handle was
> set, because that's the only time .open succeeded.
> 
> Conversely, if a plugin's .open fails, we skip .close for the entire
> backend chain.  This could be problematic (a memory leak) if any of
> our filters had returned success to .open without calling the
> backend's .open - but fortunately all of our filters called next()
> prior to doing anything locally.  Still, we can ensure that things are
> sane by asserting that if .open succeeded, the backend's handle is
> also set.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  server/backend.c |  9 ++++++++-
>  server/filters.c | 12 ++++++++----
>  2 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/server/backend.c b/server/backend.c
> index a637f754..b918adff 100644
> --- a/server/backend.c
> +++ b/server/backend.c
> @@ -172,6 +172,7 @@ int
>  backend_open (struct backend *b, struct connection *conn, int readonly)
>  {
>    struct b_conn_handle *h = &conn->handles[b->i];
> +  int r;
> 
>    debug ("%s: open readonly=%d", b->name, readonly);
> 
> @@ -179,7 +180,13 @@ backend_open (struct backend *b, struct connection *conn, int readonly)
>    assert (h->can_write == -1);
>    if (readonly)
>      h->can_write = 0;
> -  return b->open (b, conn, readonly);
> +  r = b->open (b, conn, readonly);
> +  if (r == 0) {
> +    assert (h->handle != NULL);
> +    if (b->i)
> +      assert (conn->handles[b->i - 1].handle);
> +  }
> +  return r;
>  }
> 
>  int
> diff --git a/server/filters.c b/server/filters.c
> index 5bdc8aa7..1ee62829 100644
> --- a/server/filters.c
> +++ b/server/filters.c
> @@ -210,10 +210,13 @@ filter_open (struct backend *b, struct connection *conn, int readonly)
>      if (handle == NULL)
>        return -1;
>      backend_set_handle (b, conn, handle);
> -    return 0;
>    }
> -  else
> -    return backend_open (b->next, conn, readonly);
> +  else {
> +    if (backend_open (b->next, conn, readonly) == -1)
> +      return -1;
> +    backend_set_handle (b, conn, NBDKIT_HANDLE_NOT_NEEDED);
> +  }
> +  return 0;
>  }
> 
>  static void
> @@ -224,8 +227,9 @@ filter_close (struct backend *b, struct connection *conn)
> 
>    debug ("%s: close", b->name);
> 
> -  if (f->filter.close)
> +  if (handle && f->filter.close)
>      f->filter.close (handle);
> +  backend_set_handle (b, conn, NULL);
>    b->next->close (b->next, conn);
>  }

Looks good.  Also I ran the tests & valgrind, and I ran
the reproducer on the updated nbdkit which was fine too, so:

ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html