[Libguestfs] [nbdkit PATCH v7 2/2] vddk: Drive library loading from libdir parameter.
Eric Blake
eblake at redhat.com
Tue Feb 18 17:05:11 UTC 2020
From: "Richard W.M. Jones" <rjones at 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
More information about the Libguestfs
mailing list