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 <ebl...@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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://www.redhat.com/mailman/listinfo/libguestfs