[Libguestfs] [nbdkit PATCH v3] vddk: Drive library loading from libdir parameter.

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 Linux provides dlmopen(), which opens a shared library in a new namespace. We first load a shim library into this namespace which overrides the dlopen() present in the main executable, then load the VDDK library. All further dlopen() calls made during VDDK initialization now go through our shim, at which point we can rewrite them to be absolute calls before the real dlopen() kicks in. 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: Heavy modifications to Rich's initial patch, including the idea of a shim library] --- Replaces patches 2-3/3 of v2, but keeps 1/3 of that series applied as-is. Passes 'make check' for me, although we might still need a tweak for how to resolve "nbdkit-shim-dlopen.so" from its installed location. I don't have VDDK installed locally, so I can't guarantee how this works with the REAL vddk library, but the fact that 'make check' passes proves that I got the dlmopen() stuff working correctly. plugins/vddk/Makefile.am | 14 ++++- plugins/vddk/nbdkit-vddk-plugin.pod | 39 +++++++++--- plugins/vddk/shim.c | 92 +++++++++++++++++++++++++++++ plugins/vddk/shim.h | 49 +++++++++++++++ plugins/vddk/vddk.c | 73 +++++++++++++++++++++-- tests/Makefile.am | 1 + tests/dummy-vddk.c | 20 ++++++- tests/test-vddk-real.sh | 12 +--- tests/test-vddk.sh | 22 +++++-- wrapper.c | 10 ++-- 10 files changed, 294 insertions(+), 38 deletions(-) create mode 100644 plugins/vddk/shim.c create mode 100644 plugins/vddk/shim.h diff --git a/plugins/vddk/Makefile.am b/plugins/vddk/Makefile.am index b806a7d9..dbb1cc9c 100644 --- a/plugins/vddk/Makefile.am +++ b/plugins/vddk/Makefile.am @@ -1,5 +1,5 @@ # nbdkit -# Copyright (C) 2013-2018 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 @@ -38,7 +38,7 @@ EXTRA_DIST = \ if HAVE_VDDK -plugin_LTLIBRARIES = nbdkit-vddk-plugin.la +plugin_LTLIBRARIES = nbdkit-vddk-plugin.la libnbdkit-shim-dlopen.la nbdkit_vddk_plugin_la_SOURCES = \ vddk.c \ @@ -63,6 +63,16 @@ nbdkit_vddk_plugin_la_LDFLAGS = \ -Wl,--version-script=$(top_srcdir)/plugins/plugins.syms \ $(NULL) +libnbdkit_shim_dlopen_la_SOURCES = \ + shim.c \ + shim.h \ + $(NULL) +libnbdkit_shim_dlopen_la_CFLAGS = $(WARNINGS_CFLAGS) +libnbdkit_shim_dlopen_la_LDFLAGS = \ + -module -no-undefined -shared -avoid-version -rpath /nowhere \ + -ldl \ + $(NULL) + if HAVE_POD man_MANS = nbdkit-vddk-plugin.1 diff --git a/plugins/vddk/nbdkit-vddk-plugin.pod b/plugins/vddk/nbdkit-vddk-plugin.pod index f34b9fba..f0748def 100644 --- a/plugins/vddk/nbdkit-vddk-plugin.pod +++ b/plugins/vddk/nbdkit-vddk-plugin.pod @@ -230,26 +230,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/shim.c b/plugins/vddk/shim.c new file mode 100644 index 00000000..0819c81a --- /dev/null +++ b/plugins/vddk/shim.c @@ -0,0 +1,92 @@ +/* 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. + */ + +/* This file provides a shim around dlopen, to convert relative + * library requests from VDDK into absolute requests. This library is + * designed to be the first thing loaded into an empty dl namespace, + * in order to override dlopen for all other shared libraries later + * loaded in the same namespace; but because it is in a new namespace, + * it cannot use exported nbdkit functions from the main namespace. + */ + +#include <config.h> + +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +static void *(*orig_dlopen) (const char *filename, int flags); + +/* NULL for normal behavior, or set when we want to override + * relative dlopen()s to instead be an absolute open with prefix. + */ +static char *dir; + +int nbdkit_set_dlopen_prefix (const char *newdir) +{ + free (dir); + if (newdir) { + dir = strdup (newdir); + if (!dir) + return -1; + } + else + dir = NULL; + return 0; +} + +void *dlopen(const char *filename, int flags) +{ + char *tmp = NULL; + void *ret; + + if (dir && ! strchr (filename, '/')) { + /* Best effort: we can't report an error through nbdkit, and can't + * influence dlerror; if allocation fails, just rely on the + * original relative request. + */ + if (asprintf (&tmp, "%s/%s", dir, filename) >= 0) + filename = tmp; + } + + ret = orig_dlopen (filename, flags); + free (tmp); + return ret; +} + +static void constructor (void) __attribute__ ((constructor)); +static void +constructor (void) +{ + orig_dlopen = dlsym (RTLD_NEXT, "dlopen"); +} diff --git a/plugins/vddk/shim.h b/plugins/vddk/shim.h new file mode 100644 index 00000000..a9c202f0 --- /dev/null +++ b/plugins/vddk/shim.h @@ -0,0 +1,49 @@ +/* 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. + */ + +/* This file provides a shim around dlopen, to convert relative library + * requests from VDDK into absolute requests. + */ + +#ifndef NBDKIT_SHIM_H +#define NBDKIT_SHIM_H + +#include <config.h> + +#include <dlfcn.h> +#include <stdio.h> +#include <stdlib.h> + +typedef int (*nbdkit_set_dlopen_prefix_function)(const char *); +extern int nbdkit_set_dlopen_prefix (const char *dir); + +#endif /* NBDKIT_SHIM_H */ diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c index db61c1d8..6deb0a0b 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 @@ -50,6 +51,7 @@ #include "minmax.h" #include "rounding.h" +#include "shim.h" #include "vddk-structs.h" /* Enable extra disk info debugging with: -D vddk.diskinfo=1 */ @@ -71,7 +73,8 @@ int vddk_debug_extents; #define VDDK_MAJOR 5 #define VDDK_MINOR 1 -static void *dl = NULL; /* dlopen handle */ +static void *shimdl = NULL; /* shim dlopen handle */ +static void *dl = NULL; /* vddk dlopen handle */ static int init_called = 0; /* was InitEx called */ static char *config = NULL; /* config */ @@ -153,6 +156,8 @@ vddk_unload (void) } if (dl) dlclose (dl); + if (shimdl) + dlclose (shimdl); free (config); free (libdir); free (password); @@ -248,17 +253,67 @@ load_library (void) { static const char *sonames[] = { /* Prefer the newest library in case multiple exist. */ + "lib64/libvixDiskLib.so.6", "libvixDiskLib.so.6", + "lib64/libvixDiskLib.so.5", "libvixDiskLib.so.5", }; size_t i; CLEANUP_FREE char *orig_error = NULL; + Lmid_t id; + nbdkit_set_dlopen_prefix_function set_prefix; + + /* The main namespace already has a resolved dlopen() symbol (or + * this plugin wouldn't be running), and it's too late to change + * LD_LIBRARY_PATH. However, we can use dlmopen() to create a new + * namespace, where we first load our shim library. Then all + * subsequent libraries loaded in that namespace will call our shim + * dlopen(), which lets us correct the relative dlopen()s performed + * later by VDDK initialization. + */ + shimdl = dlmopen (LM_ID_NEWLM, "libnbdkit-shim-dlopen.so", + RTLD_NOW); + if (!shimdl) { + nbdkit_error("failed to load dlopen shim: %s", dlerror()); + exit (EXIT_FAILURE); + } + if (dlinfo(shimdl, RTLD_DI_LMID, &id)) { + nbdkit_error("failed to learn dlopen shim id: %s", dlerror()); + exit (EXIT_FAILURE); + } + *(void **) (&set_prefix) = dlsym (shimdl, "nbdkit_set_dlopen_prefix"); + if (!set_prefix) { + nbdkit_error ("required dlopen shim symbol \"nbdkit_set_dlopen_prefix\" " + "is missing: %s", dlerror ()); + exit (EXIT_FAILURE); + } + nbdkit_debug("dlopen shim loaded with namespace id %d", (int) id); /* 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 = dlmopen (id, path, RTLD_NOW); + if (dl != NULL) { + /* Inform the shim about our desired prefix of libdir. This may + * modify path in-place, but that's okay. + */ + char *dir = dirname (path); + if (set_prefix (dir) == -1) { + nbdkit_error ("unable to set prefix for dlopen shim: %m"); + exit (EXIT_FAILURE); + } + nbdkit_debug("dlopen shim prefix set to %s", dir); break; + } if (i == 0) { orig_error = dlerror (); if (orig_error) @@ -268,7 +323,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 +386,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/Makefile.am b/tests/Makefile.am index e1284231..8ad4b928 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -758,6 +758,7 @@ libvixDiskLib_la_CXXFLAGS = $(WARNINGS_CFLAGS) # https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html libvixDiskLib_la_LDFLAGS = \ -shared -version-number 6:0:0 -rpath /nowhere \ + -ldl \ $(NULL) endif HAVE_VDDK diff --git a/tests/dummy-vddk.c b/tests/dummy-vddk.c index ed5d3712..dee3773f 100644 --- a/tests/dummy-vddk.c +++ b/tests/dummy-vddk.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018 Red Hat Inc. + * Copyright (C) 2018-2020 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -39,6 +39,8 @@ #include <stdio.h> #include <stdlib.h> #include <stdint.h> +#include <dlfcn.h> +#include <assert.h> #include "vddk-structs.h" @@ -49,6 +51,12 @@ VixDiskLib_InitEx (uint32_t major, uint32_t minor, VixDiskLibGenericLogFunc *panic_function, const char *lib_dir, const char *config_file) { - /* Do nothing, only exit with no error. */ + /* Simulate the fact that real vddk calls dlopen("relative"), to see + * that our shim kicks in and rewrites it. We pass invalid flags, so + * the dlopen fails, but only after going through our shim. + */ + dlopen("nosuch", RTLD_LAZY); + + /* dlopen() should have failed, but if id didn't exit, neither to we. */ return VIX_OK; } diff --git a/tests/test-vddk-real.sh b/tests/test-vddk-real.sh index 52c91232..d9cf3319 100755 --- a/tests/test-vddk-real.sh +++ b/tests/test-vddk-real.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # nbdkit -# Copyright (C) 2018-2019 Red Hat Inc. +# Copyright (C) 2018-2020 Red Hat Inc. # # Redistribution and use in source and binary forms, with or without # modification, are permitted provided that the following conditions are @@ -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" file=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 19b946b6..d99ebf88 100755 --- a/tests/test-vddk.sh +++ b/tests/test-vddk.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # nbdkit -# Copyright (C) 2018 Red Hat Inc. +# Copyright (C) 2018-2020 Red Hat Inc. # # Redistribution and use in source and binary forms, with or without # modification, are permitted provided that the following conditions are @@ -34,12 +34,22 @@ source ./functions.sh set -e set -x -rm -f test-vddk.out -cleanup_fn rm -f test-vddk.out +rm -rf test-vddk.out test-vddk.err +cleanup_fn rm -rf test-vddk.out test-vddk.err -LD_LIBRARY_PATH=.libs:$LD_LIBRARY_PATH \ -LIBRARY_PATH=.libs:$LIBRARY_PATH \ -nbdkit vddk --dump-plugin > test-vddk.out +# --dump-plugin does not initialize VDDK,... +nbdkit vddk libdir=.libs --dump-plugin > test-vddk.out cat test-vddk.out +test ! -f test-vddk.err grep ^vddk_default_libdir= test-vddk.out + +# ...but passing a filename does. Initializing our dummy library causes +# a load that we know will fail, but the important part is that dlopen's +# error message lists an absolute file even though we passed a relative +# name, showing that our shim did adjust it. +nbdkit vddk libdir=.libs \ + file=/dev/null --run ':' 2> test-vddk.err || : +cat test-vddk.err + +grep '/.libs/nosuch' test-vddk.err diff --git a/wrapper.c b/wrapper.c index 6aef81a1..37705608 100644 --- a/wrapper.c +++ b/wrapper.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2017-2019 Red Hat Inc. + * Copyright (C) 2017-2020 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -168,12 +168,14 @@ main (int argc, char *argv[]) } } - /* Needed for plugins written in OCaml. */ + /* Needed for plugins written in OCaml, and for in-tree testing of VDDK. */ s = getenv ("LD_LIBRARY_PATH"); if (s) - r = asprintf (&s, "%s/plugins/ocaml/.libs:%s", builddir, s); + r = asprintf (&s, "%s/plugins/ocaml/.libs:%s/plugins/vddk/.libs:%s", + builddir, builddir, s); else - r = asprintf (&s, "%s/plugins/ocaml/.libs", builddir); + r = asprintf (&s, "%s/plugins/ocaml/.libs:%s/plugins/vddk/.libs", + builddir, builddir); if (r < 0) { perror ("asprintf"); exit (EXIT_FAILURE); -- 2.24.1