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

[Eric Blake]

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

-- 
2.21.0