Libguestfs - Feb 2020 - [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-then-making-the-dlopen-call).

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-then-making-the-dlopen-call).
> 
> 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