[Libguestfs] [nbdkit PATCH 2/2] ext2: Supply .list_exports and .default_export
Richard W.M. Jones
rjones at redhat.com
Tue Sep 1 14:32:01 UTC 2020
On Thu, Aug 27, 2020 at 05:03:46PM -0500, Eric Blake wrote:
> When using ext2file=exportname to pick the file to server from the
> client's export name, we do not want to leak the underlying plugin's
> export list. While touching this, take advantage of string lifetimes
> via nbdkit_strdup_intern and similar for less cleanup bookkeeping.
>
> Note that we don't actually implement a full .list_exports; doing that
> with NBD_OPT_LIST is likely prohibitive (it's likely the disk contains
> LOTS of files), and would require that we can do next_ops->pread prior
> to the client reaching our .open. For the former issue, it would be
> better if the NBD protocol adds a new option NBD_OPT_LIST_HIER that
> has replies that differentiate between containers and leaves, and only
> lists elements within a container supplied as an argument. For the
> latter, we already know we want to tweak filters to be able to open a
> plugin independently of a client's connection. And for now we punt,
> and leave the advertised list empty.
>
> It's also high time we had some testsuite coverage of
> ext2file=exportname. Although ext2.img doesn't technically depend on
> the new test, including the dependency ensures the file gets rebuilt
> in an incremental build.
>
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
> filters/ext2/nbdkit-ext2-filter.pod | 3 +-
> tests/Makefile.am | 16 +++++--
> filters/ext2/ext2.c | 73 ++++++++++++++++++++---------
> tests/test-ext2-exportname.sh | 73 +++++++++++++++++++++++++++++
> 4 files changed, 138 insertions(+), 27 deletions(-)
> create mode 100755 tests/test-ext2-exportname.sh
>
> diff --git a/filters/ext2/nbdkit-ext2-filter.pod b/filters/ext2/nbdkit-ext2-filter.pod
> index 1aef9c2e..c15a2fcb 100644
> --- a/filters/ext2/nbdkit-ext2-filter.pod
> +++ b/filters/ext2/nbdkit-ext2-filter.pod
> @@ -67,7 +67,8 @@ a security risk in some situations.
>
> At present, when using this mode, the server does not advertise any
> particular exports; however, you may use
> -L<nbdkit-exportname-filter(1)> to perform that task.
> +L<nbdkit-exportname-filter(1)> to perform that task. Similarly, the
> +underlying plugin must support the default export name, C<"">.
>
> =back
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 8a799ccf..911fa2b3 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -1373,23 +1373,31 @@ EXTRA_DIST += test-exportname.sh
> # ext2 filter test.
> if HAVE_MKE2FS_WITH_D
> if HAVE_EXT2
> -if HAVE_GUESTFISH
>
> -LIBGUESTFS_TESTS += test-ext2
> +TESTS += test-ext2-exportname.sh
> +EXTRA_DIST += test-ext2-exportname.sh
> +
> check_DATA += ext2.img
> CLEANFILES += ext2.img
>
> -ext2.img: disk
> +ext2.img: disk test-ext2-exportname.sh
> rm -f $@ $@-t
> + echo /disks/disk.img > manifest
> guestfish \
> sparse $@-t 2G : \
> run : \
> mkfs ext4 /dev/sda : \
> mount /dev/sda / : \
> mkdir /disks : \
> - upload $< /disks/disk.img
> + upload $< /disks/disk.img : \
> + upload manifest /manifest
> + rm manifest
> mv $@-t $@
>
> +if HAVE_GUESTFISH
> +
> +LIBGUESTFS_TESTS += test-ext2
> +
> test_ext2_SOURCES = test-ext2.c test.h
> test_ext2_CFLAGS = $(WARNINGS_CFLAGS) $(LIBGUESTFS_CFLAGS)
> test_ext2_LDADD = libtest.la $(LIBGUESTFS_LIBS)
> diff --git a/filters/ext2/ext2.c b/filters/ext2/ext2.c
> index 7ad4a005..cfc03a3c 100644
> --- a/filters/ext2/ext2.c
> +++ b/filters/ext2/ext2.c
> @@ -50,8 +50,10 @@
> #include "cleanup.h"
> #include "io.h"
>
> -/* Filename parameter, or NULL to honor export name. */
> -static char *file;
> +/* Filename parameter, or NULL to honor export name. Using the export
> + * name is opt-in (see ext2_config_complete).
> + */
> +static const char *file;
>
> static void
> ext2_load (void)
> @@ -59,12 +61,6 @@ ext2_load (void)
> initialize_ext2_error_table ();
> }
>
> -static void
> -ext2_unload (void)
> -{
> - free (file);
> -}
> -
> static int
> ext2_config (nbdkit_next_config *next, void *nxdata,
> const char *key, const char *value)
> @@ -74,11 +70,7 @@ ext2_config (nbdkit_next_config *next, void *nxdata,
> nbdkit_error ("ext2file parameter specified more than once");
> return -1;
> }
> - file = strdup (value);
> - if (file == NULL) {
> - nbdkit_error ("strdup: %m");
> - return -1;
> - }
> + file = value;
> return 0;
> }
> else
> @@ -94,10 +86,8 @@ ext2_config_complete (nbdkit_next_config_complete *next, void *nxdata)
> return -1;
> }
>
> - if (strcmp (file, "exportname") == 0) {
> - free (file);
> + if (strcmp (file, "exportname") == 0)
> file = NULL;
> - }
> else if (file[0] != '/') {
> nbdkit_error ("the file parameter must refer to an absolute path");
> return -1;
> @@ -112,13 +102,54 @@ ext2_config_complete (nbdkit_next_config_complete *next, void *nxdata)
>
> /* The per-connection handle. */
> struct handle {
> - char *exportname; /* Client export name. */
> + const char *exportname; /* Client export name. */
> ext2_filsys fs; /* Filesystem handle. */
> ext2_ino_t ino; /* Inode of open file. */
> ext2_file_t file; /* File handle. */
> struct nbdkit_next next; /* "name" parameter to ext2fs_open. */
> };
>
> +/* Export list. */
> +static int
> +ext2_list_exports (nbdkit_next_list_exports *next, void *nxdata,
> + int readonly, int is_tls, struct nbdkit_exports *exports)
> +{
> + /* If we are honoring export names, the default export "" won't
> + * work, and we must not leak export names from the underlying
> + * plugin. Advertising all filenames within the ext2 image could be
> + * huge, and even if we wanted to, it would require that we could
> + * open the plugin prior to the client reaching our .open. So leave
> + * the list empty instead.
> + */
> + if (!file)
> + return 0;
> +
> + /* If we are serving a specific ext2file, we don't care what export
> + * name the user passes, but the underlying plugin might; there's no
> + * harm in advertising that list.
> + */
> + return next (nxdata, readonly, exports);
> +}
> +
> +/* Default export. */
> +static const char *
> +ext2_default_export (nbdkit_next_default_export *next, void *nxdata,
> + int readonly, int is_tls)
> +{
> + /* If we are honoring exports, "" will fail (even if we resolve to
> + * the inode of embedded "/", we can't serve directories), and we
> + * don't really have a sane default. XXX picking the largest
> + * embedded file might be in interesting knob to add.
> + */
> + if (file)
> + return NULL;
> +
> + /* Otherwise, we don't care about export name, so keeping things at
> + * "" is fine, regardless of the underlying plugin's default.
> + */
> + return "";
> +}
> +
> /* Create the per-connection handle. */
> static void *
> ext2_open (nbdkit_next_open *next, void *nxdata,
> @@ -133,9 +164,8 @@ ext2_open (nbdkit_next_open *next, void *nxdata,
> }
>
> /* Save the client exportname in the handle. */
> - h->exportname = strdup (exportname);
> + h->exportname = nbdkit_strdup_intern (exportname);
> if (h->exportname == NULL) {
> - nbdkit_error ("strdup: %m");
> free (h);
> return NULL;
> }
> @@ -147,7 +177,6 @@ ext2_open (nbdkit_next_open *next, void *nxdata,
>
> /* Request write access to the underlying plugin, for journal replay. */
> if (next (nxdata, 0, exportname) == -1) {
> - free (h->exportname);
> free (h);
> return NULL;
> }
> @@ -260,7 +289,6 @@ ext2_close (void *handle)
> ext2fs_file_close (h->file);
> ext2fs_close (h->fs);
> }
> - free (h->exportname);
> free (h);
> }
>
> @@ -434,11 +462,12 @@ static struct nbdkit_filter filter = {
> .name = "ext2",
> .longname = "nbdkit ext2 filter",
> .load = ext2_load,
> - .unload = ext2_unload,
> .config = ext2_config,
> .config_complete = ext2_config_complete,
> .config_help = ext2_config_help,
> .thread_model = ext2_thread_model,
> + .list_exports = ext2_list_exports,
> + .default_export = ext2_default_export,
> .open = ext2_open,
> .prepare = ext2_prepare,
> .close = ext2_close,
> diff --git a/tests/test-ext2-exportname.sh b/tests/test-ext2-exportname.sh
> new file mode 100755
> index 00000000..b30aa870
> --- /dev/null
> +++ b/tests/test-ext2-exportname.sh
> @@ -0,0 +1,73 @@
> +#!/usr/bin/env bash
> +# nbdkit
> +# Copyright (C) 2019-2020 Red Hat Inc.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#
> +# * Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the distribution.
> +#
> +# * Neither the name of Red Hat nor the names of its contributors may be
> +# used to endorse or promote products derived from this software without
> +# specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND
> +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
> +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> +# SUCH DAMAGE.
> +
> +source ./functions.sh
> +set -e
> +set -x
> +
> +requires_plugin file
> +requires nbdinfo --version
> +requires nbdsh -c 'print (h.set_full_info)'
> +
> +sock=`mktemp -u`
> +files="$sock ext2-exportname.pid ext2-exportname.out"
> +rm -f $files
> +cleanup_fn rm -f $files
> +
> +# Set up a long-running server responsive to the client's export name
> +start_nbdkit -P ext2-exportname.pid -U $sock --filter=ext2 \
> + --filter=exportname file ext2.img ext2file=exportname \
> + exportname-list=explicit exportname=hidden
> +
> +# Test that when serving by exportname, our description varies according
> +# to the client's request.
> +nbdinfo nbd+unix:///manifest\?socket=$sock > ext2-exportname.out
> +cat ext2-exportname.out
> +grep manifest ext2-exportname.out
> +grep 'content.*ASCII' ext2-exportname.out
> +
> +nbdinfo nbd+unix:///disks/disk.img\?socket=$sock > ext2-exportname.out
> +cat ext2-exportname.out
> +grep disk.img ext2-exportname.out
> +grep 'content.*MBR' ext2-exportname.out
> +
> +if nbdinfo nbd+unix://?socket=$sock > ext2-exportname.out; then
> + echo "unexpected success"
> + exit 1
> +fi
> +
> +# Test that there is no export list advertised
> +nbdinfo --list --json nbd+unix://?socket=$sock > ext2-exportname.out
> +cat ext2-exportname.out
> +grep '"exports": \[]' ext2-exportname.out
> +
> +:
> --
ACK
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine. Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/
More information about the Libguestfs
mailing list