[PATCH nbdkit v2 2/3] NOT WORKING: vddk: Drive library loading from libdir parameter.
I couldn't get this to work in the end. This is the latest non-working version. This email documents what doesn't work for the permanent record. The central problem is that VDDK InitEx() appears to dlopen() various of its own plugins. Although I wasn't able to capture exactly what dlopen() command it is running, the plugins cannot be loaded because they rely on the recompiled system libraries (libcrypto.so.X etc) and it cannot find those because $LD_LIBRARY_PATH is not set. Setting $LD_LIBRARY_PATH using setenv around the call to InitEx() does not work because glibc only consults $LD_LIBRARY_PATH when the program starts up. Various workarounds have been suggested for this but none of them are pleasant (https://stackoverflow.com/questions/6713692/problems-with-using-setenv-and-t...). So currently we must have nbdkit start up with $LD_LIBRARY_PATH set, in other words how it works at the moment. Rich.
We were previously dlopen-ing it in the load() method. This is very early and in particular means that the only possible way to configure where we find the library is through environment variables and not through config parameters. Also it's not necessary as we don't call any functions from the library (such as VixDiskLib_InitEx) until config_complete. This change is neutral refactoring as currently we _do_ configure the location through an environment variable (LD_LIBRARY_PATH). --- plugins/vddk/vddk.c | 100 ++++++++++++++++++++++---------------------- 1 file changed, 51 insertions(+), 49 deletions(-) diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c index 5d3764d6..db61c1d8 100644 --- a/plugins/vddk/vddk.c +++ b/plugins/vddk/vddk.c @@ -143,54 +143,7 @@ error_function (const char *fs, va_list args) nbdkit_error ("%s", str); } -/* Load and unload the plugin. */ -static void -vddk_load (void) -{ - static const char *sonames[] = { - /* Prefer the newest library in case multiple exist. */ - "libvixDiskLib.so.6", - "libvixDiskLib.so.5", - }; - size_t i; - CLEANUP_FREE char *orig_error = NULL; - - /* Load the library. */ - for (i = 0; i < sizeof sonames / sizeof sonames[0]; ++i) { - dl = dlopen (sonames[i], RTLD_NOW); - if (dl != NULL) - break; - if (i == 0) { - orig_error = dlerror (); - if (orig_error) - orig_error = strdup (orig_error); - } - } - 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" - "See the nbdkit-vddk-plugin(1) man page for details.", - orig_error ? : "(unknown error)", sonames[0]); - exit (EXIT_FAILURE); - } - - /* Load symbols. */ -#define STUB(fn,ret,args) \ - do { \ - fn = dlsym (dl, #fn); \ - if (fn == NULL) { \ - nbdkit_error ("required VDDK symbol \"%s\" is missing: %s", \ - #fn, dlerror ()); \ - exit (EXIT_FAILURE); \ - } \ - } while (0) -#define OPTIONAL_STUB(fn,ret,args) fn = dlsym (dl, #fn) -#include "vddk-stubs.h" -#undef STUB -#undef OPTIONAL_STUB -} - +/* Unload the plugin. */ static void vddk_unload (void) { @@ -289,6 +242,54 @@ vddk_config (const char *key, const char *value) return 0; } +/* Load the VDDK library. */ +static void +load_library (void) +{ + static const char *sonames[] = { + /* Prefer the newest library in case multiple exist. */ + "libvixDiskLib.so.6", + "libvixDiskLib.so.5", + }; + size_t i; + CLEANUP_FREE char *orig_error = NULL; + + /* Load the library. */ + for (i = 0; i < sizeof sonames / sizeof sonames[0]; ++i) { + dl = dlopen (sonames[i], RTLD_NOW); + if (dl != NULL) + break; + if (i == 0) { + orig_error = dlerror (); + if (orig_error) + orig_error = strdup (orig_error); + } + } + 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" + "See the nbdkit-vddk-plugin(1) man page for details.", + orig_error ? : "(unknown error)", sonames[0]); + exit (EXIT_FAILURE); + } + + /* Load symbols. */ +#define STUB(fn,ret,args) \ + do { \ + fn = dlsym (dl, #fn); \ + if (fn == NULL) { \ + nbdkit_error ("required VDDK symbol \"%s\" is missing: %s", \ + #fn, dlerror ()); \ + exit (EXIT_FAILURE); \ + } \ + } while (0) +#define OPTIONAL_STUB(fn,ret,args) fn = dlsym (dl, #fn) +#include "vddk-stubs.h" +#undef STUB +#undef OPTIONAL_STUB +} + static int vddk_config_complete (void) { @@ -330,6 +331,8 @@ vddk_config_complete (void) #undef missing } + load_library (); + /* Initialize VDDK library. */ DEBUG_CALL ("VixDiskLib_InitEx", "%d, %d, &debug_fn, &error_fn, &error_fn, %s, %s", @@ -831,7 +834,6 @@ static struct nbdkit_plugin plugin = { .name = "vddk", .longname = "VMware VDDK plugin", .version = PACKAGE_VERSION, - .load = vddk_load, .unload = vddk_unload, .config = vddk_config, .config_complete = vddk_config_complete, -- 2.25.0
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. Instead rely on a relatively undocumented feature of dlopen which is that when we pass in a full path it will try to load dependent libraries from the same directory. 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. --- plugins/vddk/nbdkit-vddk-plugin.pod | 39 ++++++++++++++++++++------- configure.ac | 1 + plugins/vddk/vddk.c | 42 ++++++++++++++++++++++++++--- tests/test-vddk-real.sh | 12 ++------- tests/test-vddk.sh | 19 ++++++++----- 5 files changed, 85 insertions(+), 28 deletions(-) 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/configure.ac b/configure.ac index fa902945..d71f06e4 100644 --- a/configure.ac +++ b/configure.ac @@ -130,6 +130,7 @@ AC_PROG_INSTALL AC_PROG_CPP AC_CANONICAL_HOST AC_SYS_LARGEFILE +AC_CHECK_SIZEOF([long]) AC_C_PROTOTYPES test "x$U" != "x" && AC_MSG_ERROR([Compiler not ANSI compliant]) diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c index db61c1d8..487ebba6 100644 --- a/plugins/vddk/vddk.c +++ b/plugins/vddk/vddk.c @@ -256,7 +256,18 @@ load_library (void) /* Load the library. */ for (i = 0; i < sizeof sonames / sizeof sonames[0]; ++i) { - dl = dlopen (sonames[i], RTLD_NOW); + 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/lib%d/%s", + libdir ? : VDDK_LIBDIR, 8*SIZEOF_LONG, sonames[i]) == -1) { + nbdkit_error ("asprintf: %m"); + exit (EXIT_FAILURE); + } + + dl = dlopen (path, RTLD_NOW); if (dl != NULL) break; if (i == 0) { @@ -268,7 +279,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); @@ -294,6 +305,8 @@ static int vddk_config_complete (void) { VixError err; + const char *saved_ld_library_path; + CLEANUP_FREE char *ld_library_path; if (filename == NULL) { nbdkit_error ("you must supply the file=<FILENAME> parameter " @@ -333,7 +346,24 @@ vddk_config_complete (void) load_library (); - /* Initialize VDDK library. */ + /* Initialize VDDK library. + * + * We have to set LD_LIBRARY_PATH around this call. This is because + * InitEx() loads plugins like Advanced Transport, but cannot find + * them unless the environment variable is set. + * + * This is OK as we're single-threaded here, but if VDDK decided in + * future to lazily load plugins later on then this would break. + */ + saved_ld_library_path = getenv ("LD_LIBRARY_PATH"); + if (asprintf (&ld_library_path, "%s/lib%d", + libdir ? : VDDK_LIBDIR, 8*SIZEOF_LONG) == -1) { + nbdkit_error ("asprintf: %m"); + exit (EXIT_FAILURE); + } + nbdkit_debug ("setting LD_LIBRARY_PATH=%s", ld_library_path); + setenv ("LD_LIBRARY_PATH", ld_library_path, 1); + DEBUG_CALL ("VixDiskLib_InitEx", "%d, %d, &debug_fn, &error_fn, &error_fn, %s, %s", VDDK_MAJOR, VDDK_MINOR, @@ -349,6 +379,12 @@ vddk_config_complete (void) } init_called = 1; + nbdkit_debug ("restoring LD_LIBRARY_PATH"); + if (saved_ld_library_path) + setenv ("LD_LIBRARY_PATH", saved_ld_library_path, 1); + else + unsetenv ("LD_LIBRARY_PATH"); + return 0; } 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..c2df4d69 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,19 @@ source ./functions.sh set -e set -x -rm -f test-vddk.out -cleanup_fn rm -f test-vddk.out +# Real VDDK only works on x86-64. While this test doesn't test real +# VDDK, we do need to know that we're on a 64 bit architecture +# (because of the need for the lib64 path), so we might as well only +# test x86-64. +requires test $(uname -m) = x86_64 -LD_LIBRARY_PATH=.libs:$LD_LIBRARY_PATH \ -LIBRARY_PATH=.libs:$LIBRARY_PATH \ -nbdkit vddk --dump-plugin > test-vddk.out +rm -rf vmware-vix-disklib-distrib test-vddk.out +cleanup_fn rm -rf vmware-vix-disklib-distrib test-vddk.out + +mkdir -p vmware-vix-disklib-distrib/lib64 +cp .libs/libvixDiskLib.so* vmware-vix-disklib-distrib/lib64/ + +nbdkit vddk libdir=vmware-vix-disklib-distrib --dump-plugin > test-vddk.out cat test-vddk.out grep ^vddk_default_libdir= test-vddk.out -- 2.25.0
--- configure.ac | 5 +++++ plugins/vddk/vddk.c | 4 ++++ 2 files changed, 9 insertions(+) diff --git a/configure.ac b/configure.ac index d71f06e4..57626a76 100644 --- a/configure.ac +++ b/configure.ac @@ -321,6 +321,11 @@ AC_SEARCH_LIBS([dlsym], [dl dld], [ ]) LIBS="$old_LIBS" +old_LIBS="$LIBS" +LIBS="$LIBS -ldl" +AC_CHECK_FUNCS([dlmopen]) +LIBS="$old_LIBS" + dnl Test if <iconv.h> header can build working binaries. dnl dnl On FreeBSD: iconv and libiconv both exist, both can be installed diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c index 487ebba6..30dd6375 100644 --- a/plugins/vddk/vddk.c +++ b/plugins/vddk/vddk.c @@ -267,7 +267,11 @@ load_library (void) exit (EXIT_FAILURE); } +#ifdef HAVE_DLMOPEN + dl = dlmopen (LM_ID_NEWLM, path, RTLD_NOW); +#else dl = dlopen (path, RTLD_NOW); +#endif if (dl != NULL) break; if (i == 0) { -- 2.25.0
On 2/13/20 10:04 AM, Richard W.M. Jones wrote:
I couldn't get this to work in the end. This is the latest non-working version. This email documents what doesn't work for the permanent record.
The central problem is that VDDK InitEx() appears to dlopen() various of its own plugins. Although I wasn't able to capture exactly what dlopen() command it is running, the plugins cannot be loaded because they rely on the recompiled system libraries (libcrypto.so.X etc) and it cannot find those because $LD_LIBRARY_PATH is not set.
Setting $LD_LIBRARY_PATH using setenv around the call to InitEx() does not work because glibc only consults $LD_LIBRARY_PATH when the program starts up. Various workarounds have been suggested for this but none of them are pleasant (https://stackoverflow.com/questions/6713692/problems-with-using-setenv-and-t...).
So currently we must have nbdkit start up with $LD_LIBRARY_PATH set, in other words how it works at the moment.
Here's my rough [untested] idea, after reading https://hackerboss.com/overriding-system-functions-for-fun-and-profit/. Create a shim shared library that implements only one function - a replacement dlopen(), something like: #define _GNU_SOURCE #include <dlfcn.h> 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. */ char *override_dir; void *dlopen(const char *filename, int flags) { if (override_dir && !strchr(filename, '/')) ... code to rewrite filename into override_dir/filename return orig_dlopen (filename, flags); } void _init(void) { orig_dlopen = dlsym(RTLD_NEXT, "dlopen"); } then in our vddk plugin, we dlmopen(LM_ID_NEWLM, "shim", flags) to create a new namespace, dlinfo() to learn the id of that namespace, then override_dir = libdir; dlmopen(id, "vddk", flags) so that vddk gets loaded into that same namespace but where all of its relative loads get rewritten into absolute loads. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Richard W.M. Jones