Although the real reason for adding the new interface is the vddk plugin, that one is harder to test (not everyone configures it to be built); so adding a standalone plugin that does the bare minimum to validate our code is worthwhile. Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- tests/Makefile.am | 25 +++++++++ tests/test-dlopen-plugin.c | 106 +++++++++++++++++++++++++++++++++++++ tests/test-dlopen.sh | 54 +++++++++++++++++++ 3 files changed, 185 insertions(+) create mode 100644 tests/test-dlopen-plugin.c create mode 100755 tests/test-dlopen.sh diff --git a/tests/Makefile.am b/tests/Makefile.am index 8b1638ef..369c0e6f 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -100,6 +100,7 @@ EXTRA_DIST = \ test-data-extents.sh \ test-data-file.sh \ test-data-raw.sh \ + test-dlopen.sh \ test-debug-flags.sh \ test-dump-config.sh \ test-dump-plugin.sh \ @@ -374,6 +375,30 @@ test_cxx_filter_la_LDFLAGS = \ $(NULL) endif HAVE_CXX +# This builds a plugin that calls dlopen, to test our dlopen override +# that supplies a prefix. +TESTS += test-dlopen.sh +# check_LTLIBRARIES won't build a shared library (see automake manual). +# So we have to do this and add a dependency. +noinst_LTLIBRARIES += test-dlopen-plugin.la +test-dlopen.sh: test-dlopen-plugin.la + +test_dlopen_plugin_la_SOURCES = \ + test-dlopen-plugin.c \ + $(top_srcdir)/include/nbdkit-plugin.h \ + $(NULL) +test_dlopen_plugin_la_CPPFLAGS = \ + -I$(top_srcdir)/include \ + $(NULL) +test_dlopen_plugin_la_CFLAGS = $(WARNINGS_CFLAGS) +test_dlopen_plugin_la_LIBADD = $(DL_LIBS) +# For use of the -rpath option, see: +# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html +test_dlopen_plugin_la_LDFLAGS = \ + -module -avoid-version -shared -rpath /nowhere \ + $(DL_LDFLAGS) \ + $(NULL) + # Exit with parent test. check_PROGRAMS += test-exit-with-parent TESTS += test-exit-with-parent diff --git a/tests/test-dlopen-plugin.c b/tests/test-dlopen-plugin.c new file mode 100644 index 00000000..5c81800b --- /dev/null +++ b/tests/test-dlopen-plugin.c @@ -0,0 +1,106 @@ +/* nbdkit + * Copyright (C) 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. + */ + +#include <config.h> + +#include <assert.h> +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#include <nbdkit-plugin.h> + +#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL + +static const char *libdir; + +static int +dlopen_config (const char *key, const char *value) +{ + if (strcmp (key, "libdir") == 0) + libdir = value; + return 0; +} + +static int +dlopen_config_complete (void) +{ + char *msg; + + if (libdir) { + if (nbdkit_set_dlopen_prefix (libdir) == -1) + return -1; + } + + /* The main goal of this plugin is to show that we can hook dlopen. + * Test this by requesting a library that doesn't exist, where the + * difference in the error message shows if we were successful. + */ + dlopen ("no_one_has_a_library_by_this_name", RTLD_NOW); + msg = dlerror (); + assert (msg); + nbdkit_error ("dlopen failed as expected: %s\n", msg); + + return 0; +} + +/* These must be provided, but we don't really need to use them. */ +static void * +dlopen_open (int readonly) +{ + return NBDKIT_HANDLE_NOT_NEEDED; +} + +static int64_t +dlopen_get_size (void *handle) +{ + return 0; +} + +static int +dlopen_pread (void *handle, void *buf, uint32_t count, uint64_t offset) +{ + assert (0); +} + +static struct nbdkit_plugin plugin = { + .name = "dlopenplugin", + .version = PACKAGE_VERSION, + .config = dlopen_config, + .config_complete = dlopen_config_complete, + .open = dlopen_open, + .get_size = dlopen_get_size, + .pread = dlopen_pread, +}; + +NBDKIT_REGISTER_PLUGIN(plugin) diff --git a/tests/test-dlopen.sh b/tests/test-dlopen.sh new file mode 100755 index 00000000..2a13cb65 --- /dev/null +++ b/tests/test-dlopen.sh @@ -0,0 +1,54 @@ +#!/usr/bin/env bash +# nbdkit +# Copyright (C) 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 + +files="test-dlopen.err" +rm -f $files +cleanup_fn rm -f $files + +libs=./.libs + +# First: show that without using the new function, dlopen complains +# about a relative request. +nbdkit -U - $libs/test-dlopen-plugin.so --run 'true' \ + 2> test-dlopen.err +cat test-dlopen.err +grep "as expected: no_one_has" test-dlopen.err + +# Now, show that we can alter the message based on our config parameter. +nbdkit -U - $libs/test-dlopen-plugin.so libdir="$PWD" --run 'true' \ + 2> test-dlopen.err +cat test-dlopen.err +grep -F "as expected: $PWD/no_one_has" test-dlopen.err -- 2.24.1

From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; Do not use LD_LIBRARY_PATH to locate the VDDK library. Setting this always causes problems because VDDK comes bundled with broken replacements for system libraries, such as libcrypto.so and libstdc++.so. Two problems this causes which we have seen in the real world: (1) User does ‘export LD_LIBRARY_PATH=vmware-vix-disklib-distrib’ and that breaks lots of ordinary utilities on their system. (2) nbdkit vddk --run subcommand inherits the LD_LIBRARY_PATH environment variable from nbdkit, and common commands such as 'qemu-img' break, relying on complex workarounds like saving and restoring the original LD_LIBRARY_PATH in the subcommand. While dlopen() _does_ allow us to pass in an absolute path name to a library (which picks up all immediate dependencies of that library from the same directory, regardless of LD_LIBRARY_PATH), that only catches immediate dependencies; but VDDK itself calls a subsequent dlopen() with a relative name, and that subsequent load no longer searches the directory we supplied explicitly. However, we can't call setenv() to change LD_LIBRARY_PATH dynamically, since (for security reasons) ld.so uses only a value of the environment variable cached prior to main(). Instead, we can fix the problem by relying on the fact that nbdkit just added a way to request a prefix be added to any relative dlopen() requests, feeding it the libdir= parameter learned during .config after we confirm that directory worked for loading VDDK. Note this may break some callers who are not using libdir and expecting LD_LIBRARY_PATH to work, so it's a change in behaviour which we will have to highlight prominently in the 1.18 release notes. Thanks: Dan Berrangé, Ming Xie, Eric Blake. [eblake: Several modifications to Rich's initial patch, mainly to take advantage of the new nbdkit_set_dlopen_prefix] --- plugins/vddk/nbdkit-vddk-plugin.pod | 39 +++++++++++++++++++------ plugins/vddk/vddk.c | 44 +++++++++++++++++++++++++---- tests/test-vddk-real.sh | 10 +------ tests/test-vddk.sh | 6 +--- 4 files changed, 71 insertions(+), 28 deletions(-) diff --git a/plugins/vddk/nbdkit-vddk-plugin.pod b/plugins/vddk/nbdkit-vddk-plugin.pod index a7300e94..11d12c3f 100644 --- a/plugins/vddk/nbdkit-vddk-plugin.pod +++ b/plugins/vddk/nbdkit-vddk-plugin.pod @@ -233,26 +233,47 @@ This parameter is ignored for backwards compatibility. =head1 LIBRARY AND CONFIG FILE LOCATIONS -If the VDDK library (F<libvixDiskLib.so.I<N>>) is located on a -non-standard path, you may need to set C<LD_LIBRARY_PATH> or modify -F</etc/ld.so.conf> before this plugin will work. In addition you may -want to set the C<libdir> parameter so that the VDDK library can load -plugins like Advanced Transport. +The VDDK library should not be placed on a system library path such as +F</usr/lib>. The reason for this is that the VDDK library is shipped +with recompiled libraries like F<libcrypto.so> and F<libstdc++.so> +that can conflict with system libraries. -Usually the VDDK distribution directory should be passed as the -C<libdir> parameter and set C<LD_LIBRARY_PATH> to the F<lib64> -subdirectory: +You have two choices: + +=over 4 + +=item * + +Place VDDK in the default libdir which is compiled into this plugin, +for example: + + $ nbdkit vddk --dump-plugin | grep ^vddk_default_libdir + vddk_default_libdir=/usr/lib64/vmware-vix-disklib + +=item * + +But the most common way is to set the C<libdir> parameter to point to +F<vmware-vix-disklib-distrib/> (which you can unpack anywhere you +like), and this plugin will find the VDDK library from there. For +example: - LD_LIBRARY_PATH=/path/to/vmware-vix-disklib-distrib/lib64 \ nbdkit vddk \ libdir=/path/to/vmware-vix-disklib-distrib \ file=file.vmdk +=back + VDDK itself looks in a few default locations for the optional configuration file, usually including F</etc/vmware/config> and F<$HOME/.vmware/config>, but you can override this using the C<config> parameter. +=head2 No need to set C<LD_LIBRARY_PATH> + +In nbdkit E<le> 1.16 you had to set the environment variable +C<LD_LIBRARY_PATH> when using this plugin. In nbdkit E<ge> 1.18 this +is I<not> recommended. + =head1 FILE PARAMETER The C<file> parameter can either be a local file, in which case it diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c index 0abec68e..d3c4483e 100644 --- a/plugins/vddk/vddk.c +++ b/plugins/vddk/vddk.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2020 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -40,6 +40,7 @@ #include <string.h> #include <unistd.h> #include <dlfcn.h> +#include <libgen.h> #define NBDKIT_API_VERSION 2 @@ -247,8 +248,14 @@ static void load_library (void) { static const char *sonames[] = { - /* Prefer the newest library in case multiple exist. */ + /* Prefer the newest library in case multiple exist. This list + * intentionally allows a user to point to either the top-level + * installation directory, or the lib64/ subdirectory thereof, in + * part because it makes our test setup easier. + */ + "lib64/libvixDiskLib.so.6", "libvixDiskLib.so.6", + "lib64/libvixDiskLib.so.5", "libvixDiskLib.so.5", }; size_t i; @@ -256,9 +263,28 @@ load_library (void) /* Load the library. */ for (i = 0; i < sizeof sonames / sizeof sonames[0]; ++i) { - dl = dlopen (sonames[i], RTLD_NOW); - if (dl != NULL) + CLEANUP_FREE char *path; + + /* Set the full path so that dlopen will preferentially load the + * system libraries from the same directory. + */ + if (asprintf (&path, "%s/%s", libdir, sonames[i]) == -1) { + nbdkit_error ("asprintf: %m"); + exit (EXIT_FAILURE); + } + + dl = dlopen (path, RTLD_NOW); + if (dl != NULL) { + /* Now that we found the library, inform nbdkit's dlopen shim + * about our desired prefix based on libdir for all future + * loads. This may modify path in-place, but that's okay. + */ + char *dir = dirname (path); + if (nbdkit_set_dlopen_prefix (dir) == -1) + exit (EXIT_FAILURE); + nbdkit_debug("dlopen shim prefix set to %s", dir); break; + } if (i == 0) { orig_error = dlerror (); if (orig_error) @@ -268,7 +294,7 @@ load_library (void) if (dl == NULL) { nbdkit_error ("%s\n\n" "If '%s' is located on a non-standard path you may need to\n" - "set $LD_LIBRARY_PATH or edit /etc/ld.so.conf.\n\n" + "set libdir=/path/to/vmware-vix-disklib-distrib.\n\n" "See the nbdkit-vddk-plugin(1) man page for details.", orig_error ? : "(unknown error)", sonames[0]); exit (EXIT_FAILURE); @@ -331,6 +357,14 @@ vddk_config_complete (void) #undef missing } + if (!libdir) { + libdir = strdup (VDDK_LIBDIR); + if (!libdir) { + nbdkit_error ("strdup: %m"); + return -1; + } + } + load_library (); /* Initialize VDDK library. */ diff --git a/tests/test-vddk-real.sh b/tests/test-vddk-real.sh index 3a888eac..f155025d 100755 --- a/tests/test-vddk-real.sh +++ b/tests/test-vddk-real.sh @@ -51,15 +51,7 @@ cleanup_fn rm -f $files qemu-img create -f vmdk test-vddk-real.vmdk 100M -export old_ld_library_path="$LD_LIBRARY_PATH" -export LD_LIBRARY_PATH="$vddkdir/lib64:$LD_LIBRARY_PATH" - nbdkit -f -v -U - \ --filter=readahead \ vddk libdir="$vddkdir" test-vddk-real.vmdk \ - --run ' - # VDDK library path breaks qemu-img, we must restore the - # original path here. - export LD_LIBRARY_PATH="$old_ld_library_path" - qemu-img convert $nbd -O raw test-vddk-real.out -' + --run 'qemu-img convert $nbd -O raw test-vddk-real.out' diff --git a/tests/test-vddk.sh b/tests/test-vddk.sh index 9ee46aa8..927e29fd 100755 --- a/tests/test-vddk.sh +++ b/tests/test-vddk.sh @@ -37,15 +37,11 @@ set -x rm -f test-vddk.out cleanup_fn rm -f test-vddk.out -LD_LIBRARY_PATH=.libs:$LD_LIBRARY_PATH \ -LIBRARY_PATH=.libs:$LIBRARY_PATH \ -nbdkit vddk --dump-plugin > test-vddk.out +nbdkit vddk libdir=.libs --dump-plugin > test-vddk.out cat test-vddk.out grep ^vddk_default_libdir= test-vddk.out # Also test our magic file= handling, even though the dummy driver doesn't # really open a file. -LD_LIBRARY_PATH=.libs:$LD_LIBRARY_PATH \ -LIBRARY_PATH=.libs:$LIBRARY_PATH \ nbdkit -U - vddk libdir=.libs /dev/null --run true -- 2.24.1