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

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@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);
 }

-- 
2.21.0

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@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