From: "Richard W.M. Jones" <rjones@redhat.com> 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 using the re-exec logic added in the previous patch, by computing a directory to temporarily add to LD_LIBRARY_PATH during re-exec based on which file name we were able to successfully dlopen(), using libdir= passed in during .config to seed the search. 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. We can also use our dlopen() probing to simplify our testsuite - even though the real VDDK requires LD_LIBRARY_PATH to point to vmware-vix-disklib-distrib/lib64, the fact that we probe both "lib64/libvixDiskLib.so.6" and "libvixDiskLib.so.6" means that our testsuite's dummy library can live at tests/.libs/libvixDiskLib.so.6, while still covering functionality. Thanks: Dan Berrangé, Ming Xie, Eric Blake. [eblake: Several modifications to Rich's initial patch, mainly to take advantage of re-execing] --- plugins/vddk/nbdkit-vddk-plugin.pod | 39 ++++++++++++---- plugins/vddk/vddk.c | 70 ++++++++++++++++++++++++----- tests/test-vddk-real.sh | 14 ++---- tests/test-vddk.sh | 17 +++++-- 4 files changed, 106 insertions(+), 34 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 5ae41547..1ac996bb 100644 --- a/plugins/vddk/vddk.c +++ b/plugins/vddk/vddk.c @@ -41,6 +41,7 @@ #include <unistd.h> #include <dlfcn.h> #include <fcntl.h> +#include <libgen.h> #define NBDKIT_API_VERSION 2 @@ -254,10 +255,9 @@ vddk_config (const char *key, const char *value) * Thus, no return value is needed. */ static void -perform_reexec (const char *prepend) +perform_reexec (const char *env, const char *prepend) { CLEANUP_FREE char *library = NULL; - const char *env = getenv ("LD_LIBRARY_PATH"); int argc = 0; CLEANUP_FREE char **argv = NULL; int fd; @@ -334,13 +334,39 @@ perform_reexec (const char *prepend) nbdkit_debug ("failure to execvp: %m"); } +/* See if prepend is already in LD_LIBRARY_PATH; if not, re-exec. */ +static void +check_reexec (const char *prepend) +{ + const char *env = getenv ("LD_LIBRARY_PATH"); + CLEANUP_FREE char *haystack = NULL; + CLEANUP_FREE char *needle = NULL; + + if (reexeced) + return; + if (env && asprintf (&haystack, ":%s:", env) >= 0 && + asprintf (&needle, ":%s:", prepend) >= 0 && + strstr (haystack, needle) != NULL) + return; + + perform_reexec (env, prepend); +} + /* Load the VDDK library. */ 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. Check two + * possible directories: the usual VDDK installation puts .so + * files in an arch-specific subdirectory of $libdir (although + * only VDDK 5 supported 32-bit); but in our testsuite is easier + * to write if we point libdir directly to a stub .so. + */ + "lib64/libvixDiskLib.so.6", "libvixDiskLib.so.6", + "lib64/libvixDiskLib.so.5", + "lib32/libvixDiskLib.so.5", "libvixDiskLib.so.5", }; size_t i; @@ -348,9 +374,25 @@ 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, ensure that LD_LIBRARY_PATH + * includes its directory for all future loads. This may modify + * path in-place and/or re-exec nbdkit, but that's okay. + */ + check_reexec (dirname (path)); break; + } if (i == 0) { orig_error = dlerror (); if (orig_error) @@ -358,11 +400,9 @@ load_library (void) } } if (dl == NULL) { - if (!reexeced && libdir) - perform_reexec (libdir); /* TODO: Use correct dir */ 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); @@ -406,7 +446,7 @@ vddk_config_complete (void) char *env = getenv ("LD_LIBRARY_PATH"); nbdkit_debug ("cleaning up after re-exec"); - if (!env || strstr (env, reexeced) != 0 || + if (!env || strstr (env, reexeced) == NULL || (libdir && strncmp (env, libdir, strlen (libdir)) != 0)) { nbdkit_error ("'reexeced_' set with garbled environment"); return -1; @@ -453,18 +493,26 @@ 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. */ DEBUG_CALL ("VixDiskLib_InitEx", "%d, %d, &debug_fn, &error_fn, &error_fn, %s, %s", VDDK_MAJOR, VDDK_MINOR, - libdir ? : VDDK_LIBDIR, config ? : "NULL"); + libdir, config ? : "NULL"); err = VixDiskLib_InitEx (VDDK_MAJOR, VDDK_MINOR, &debug_function, /* log function */ &error_function, /* warn function */ &error_function, /* panic function */ - libdir ? : VDDK_LIBDIR, config); + libdir, config); if (err != VIX_OK) { VDDK_ERROR (err, "VixDiskLib_InitEx"); exit (EXIT_FAILURE); diff --git a/tests/test-vddk-real.sh b/tests/test-vddk-real.sh index 9f29dfba..d0ef6cbe 100755 --- a/tests/test-vddk-real.sh +++ b/tests/test-vddk-real.sh @@ -55,22 +55,16 @@ qemu-img create -f vmdk test-vddk-real.vmdk 100M # not translating errors. export LANG=C -export old_ld_library_path="$LD_LIBRARY_PATH" -export LD_LIBRARY_PATH="$vddkdir/lib64:$LD_LIBRARY_PATH" - +fail=0 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 -' \ - > test-vddk-real.log 2>&1 + --run 'qemu-img convert $nbd -O raw test-vddk-real.out' \ + > test-vddk-real.log 2>&1 || fail=1 # Check the log for missing modules cat test-vddk-real.log if grep 'cannot open shared object file' test-vddk-real.log; then exit 1 fi +exit $fail diff --git a/tests/test-vddk.sh b/tests/test-vddk.sh index ba6788d0..f4a9195e 100755 --- a/tests/test-vddk.sh +++ b/tests/test-vddk.sh @@ -37,13 +37,22 @@ set -x rm -f test-vddk.out cleanup_fn rm -f test-vddk.out -LD_LIBRARY_PATH=.libs:$LD_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 \ -nbdkit -U - vddk libdir=.libs /dev/null --run true +# really open a file. We also ensure that LD_LIBRARY_PATH in the child +# is not further modified, even if nbdkit had to re-exec. It's tricky, +# though: when running uninstalled, our wrapper nbdkit also modifies +# LD_LIBRARY_PATH, so we need to capture an expected value from what +# leaks through an innocuous plugin. +expect_LD_LIBRARY_PATH=$(nbdkit -U - zero --run 'echo "$LD_LIBRARY_PATH"') +export expect_LD_LIBRARY_PATH + +nbdkit -U - vddk libdir=.libs /dev/null \ + --run 'echo "LD_LIBRARY_PATH=$LD_LIBRARY_PATH" + echo "expect_LD_LIBRARY_PATH=$expect_LD_LIBRARY_PATH" + test "$LD_LIBRARY_PATH" = "$expect_LD_LIBRARY_PATH"' -- 2.24.1