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