server: Saner filter .close calls
commitfd2deeb10de15dff97d07a4f8e4f188f7084b1b4
authorEric Blake <eblake@redhat.com>
Wed, 18 Sep 2019 14:26:35 +0000 (18 09:26 -0500)
committerEric Blake <eblake@redhat.com>
Wed, 18 Sep 2019 20:12:39 +0000 (18 15:12 -0500)
tree4c086a975eec76148eaed921616be48a72416b01
parentc4a95196e77c9c3e06f482107868cefb56e9c325
server: Saner filter .close calls

I found a core dump when forcing nbdkit-xz-filter to fail .open:

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. There are five other
filters that currently override .open: cow, log, partition, rate, and
truncate, but making them fail .open requires getting a malloc()
failure rather than having a user parameter that can be supplied with
a bogusly large value.

The failure happens because xz's .open fails after null's has
succeeded, but the cleanup code that decides whether to call the
.close chain looks solely at 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 that feels dirty compared to never calling .close with a
handle not returned by a successful .open.  Thus, 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.  Doing this changes the handle passed
to filters that don't override .open or which return
NBDKIT_HANDLE_NOT_NEEDED - but such filters shouldn't care.

Conversely, if a plugin's .open fails, we skip .close for the entire
backend chain.  This could potentially 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, and that practice matches
documentation.  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
server/filters.c