Libguestfs - Feb 2020 - Re: [nbdkit PATCH v7 2/2] vddk: Drive library loading from libdir parameter.

In v7:
everything should work now! The re-exec code is slightly simplified,
with Rich's suggestion to pass the original LD_LIBRARY_PATH rather
than just the prefix being added, and I've now finished wiring up the
initial dlopen() check into code that correctly computes the right
prefix dir to add to LD_LIBRARY_PATH.

Eric Blake (1):
  vddk: Add re-exec with altered environment

Richard W.M. Jones (1):
  vddk: Drive library loading from libdir parameter.

 plugins/vddk/nbdkit-vddk-plugin.pod |  39 +++--
 plugins/vddk/vddk.c                 | 232 ++++++++++++++++++++++++----
 tests/test-vddk-real.sh             |  14 +-
 tests/test-vddk.sh                  |  17 +-
 4 files changed, 248 insertions(+), 54 deletions(-)

-- 
2.24.1

In the next patch, we want to get rid of the requirement for the user
to supply LD_LIBRARY_PATH pointing to vddk libs, if we can derive it
ourselves from libdir.  However, VDDK itself requires LD_LIBRARY_PATH
to be set (because it tries to load libraries that in turn depend on a
bare library name, which no manner of dlopen() hacking can work
around, and implementing la_objsearch() is no better for requiring
LD_AUDIT to be set).  And since ld.so caches the value of
LD_LIBRARY_PATH at startup (for security reasons), the ONLY way to set
it for loading vddk, while clearing it again before --run spawns a
child process, is to re-exec nbdkit with slight alterations.

Since VDDK only runs on Linux, we can assume the presence of
/proc/self/{exe,cmdline}, and parse off everything we need (provided
nbdkit didn't muck with argv[], which we just fixed) to recursively
exec with a munged environment that still has enough breadcrumbs to
undo the munging.

This patch does not quite set LD_LIBRARY_PATH correctly in all cases
(since vddk expects libdir= to point to vmware-vix-disklib-distrib/,
but LD_LIBRARY_PATH to vmware-vix-disklib-distrib/lib64), but that
will be cleaned up in the next patch; in the meantime, the re-exec in
this patch fires but has no ill effects.

While making the change, clean up static variables that were explicitly
zero-initialized to instead rely on .bss initial value.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/vddk/vddk.c | 172 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 147 insertions(+), 25 deletions(-)

diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 0abec68e..5ae41547 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 <fcntl.h>

 #define NBDKIT_API_VERSION 2

@@ -71,25 +72,26 @@ int vddk_debug_extents;
 #define VDDK_MAJOR 5
 #define VDDK_MINOR 1

-static void *dl = NULL;                    /* dlopen handle */
-static int init_called = 0;                /* was InitEx called */
-
-static char *config = NULL;                /* config */
-static const char *cookie = NULL;          /* cookie */
-static const char *filename = NULL;        /* file */
-static char *libdir = NULL;                /* libdir */
-static uint16_t nfc_host_port = 0;         /* nfchostport */
-static char *password = NULL;              /* password */
-static uint16_t port = 0;                  /* port */
-static const char *server_name = NULL;     /* server */
-static bool single_link = false;           /* single-link */
-static const char *snapshot_moref = NULL;  /* snapshot */
-static const char *thumb_print = NULL;     /* thumbprint */
-static const char *transport_modes = NULL; /* transports */
-static bool unbuffered = false;            /* unbuffered */
-static const char *username = NULL;        /* user */
-static const char *vmx_spec = NULL;        /* vm */
-static bool is_remote = false;
+static void *dl;                           /* dlopen handle */
+static bool init_called;                   /* was InitEx called */
+static char *reexeced;                     /* orig LD_LIBRARY_PATH on reexec */
+
+static char *config;                       /* config */
+static const char *cookie;                 /* cookie */
+static const char *filename;               /* file */
+static char *libdir;                       /* libdir */
+static uint16_t nfc_host_port;             /* nfchostport */
+static char *password;                     /* password */
+static uint16_t port;                      /* port */
+static const char *server_name;            /* server */
+static bool single_link;                   /* single-link */
+static const char *snapshot_moref;         /* snapshot */
+static const char *thumb_print;            /* thumbprint */
+static const char *transport_modes;        /* transports */
+static bool unbuffered;                    /* unbuffered */
+static const char *username;               /* user */
+static const char *vmx_spec;               /* vm */
+static bool is_remote;

 #define VDDK_ERROR(err, fs, ...)                                \
   do {                                                          \
@@ -162,6 +164,8 @@ vddk_unload (void)
 static int
 vddk_config (const char *key, const char *value)
 {
+  int r;
+
   if (strcmp (key, "config") == 0) {
     /* See FILENAMES AND PATHS in nbdkit-plugin(3). */
     free (config);
@@ -199,12 +203,15 @@ vddk_config (const char *key, const char *value)
     if (nbdkit_parse_uint16_t ("port", value, &port) == -1)
       return -1;
   }
+  else if (strcmp (key, "reexeced_") == 0) {
+    /* Special name because it is only for internal use. */
+    reexeced = (char *)value;
+  }
   else if (strcmp (key, "server") == 0) {
     server_name = value;
   }
   else if (strcmp (key, "single-link") == 0) {
-    int r = nbdkit_parse_bool (value);
-
+    r = nbdkit_parse_bool (value);
     if (r == -1)
       return -1;
     single_link = r;
@@ -219,8 +226,7 @@ vddk_config (const char *key, const char *value)
     transport_modes = value;
   }
   else if (strcmp (key, "unbuffered") == 0) {
-    int r = nbdkit_parse_bool (value);
-
+    r = nbdkit_parse_bool (value);
     if (r == -1)
       return -1;
     unbuffered = r;
@@ -242,6 +248,92 @@ vddk_config (const char *key, const char *value)
   return 0;
 }

+/* Perform a re-exec that temporarily modifies LD_LIBRARY_PATH.  Does
+ * not return on success; on failure, problems have been logged, but
+ * the caller prefers to proceed as if this had not been attempted.
+ * Thus, no return value is needed.
+ */
+static void
+perform_reexec (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;
+  size_t len = 0, buflen = 512;
+  CLEANUP_FREE char *buf = NULL;
+
+  /* In order to re-exec, we need our original command line.  The
+   * Linux kernel does not make it easy to know in advance how large
+   * it was, so we just slurp in the whole file, doubling our reads
+   * until we get a short read.  This assumes nbdkit did not alter its
+   * original argv[].
+   */
+  fd = open ("/proc/self/cmdline", O_RDONLY);
+  if (fd == -1) {
+    nbdkit_debug ("failure to parse original argv: %m");
+    return;
+  }
+
+  do {
+    char *p = realloc (buf, buflen * 2);
+    ssize_t r;
+
+    if (!p) {
+      nbdkit_debug ("failure to parse original argv: %m");
+      return;
+    }
+    buf = p;
+    buflen *= 2;
+    r = read (fd, buf + len, buflen - len);
+    if (r == -1) {
+      nbdkit_debug ("failure to parse original argv: %m");
+      return;
+    }
+    len += r;
+  } while (len == buflen);
+  nbdkit_debug ("original command line occupies %zu bytes", len);
+
+  /* Split cmdline into argv, then append one more arg. */
+  buflen = len;
+  len = 0;
+  while (len < buflen) {
+    char **tmp = realloc (argv, sizeof *argv * (argc + 2));
+
+    if (!tmp) {
+      nbdkit_debug ("failure to parse original argv: %m");
+      return;
+    }
+    argv = tmp;
+    argv[argc++] = buf + len;
+    len += strlen (buf + len) + 1;
+  }
+  if (!env)
+    env = "";
+  nbdkit_debug ("original argc == %d, adding reexeced_=%s", argc,
env);
+  if (asprintf (&reexeced, "reexeced_=%s", env) == -1) {
+    nbdkit_debug ("failure to re-exec: %m");
+    return;
+  }
+  argv[argc++] = reexeced;
+  argv[argc] = NULL;
+
+  if (env[0])
+    asprintf (&library, "%s:%s", prepend, env);
+  else
+    library = strdup (prepend);
+  if (!library || setenv ("LD_LIBRARY_PATH", library, 1) == -1) {
+    nbdkit_debug ("failure to set LD_LIBRARY_PATH: %m");
+    return;
+  }
+
+  nbdkit_debug ("re-executing with updated LD_LIBRARY_PATH=%s",
library);
+  fflush (NULL);
+  execvp ("/proc/self/exe", argv);
+  nbdkit_debug ("failure to execvp: %m");
+}
+
 /* Load the VDDK library. */
 static void
 load_library (void)
@@ -266,6 +358,8 @@ 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"
@@ -301,6 +395,34 @@ vddk_config_complete (void)
     return -1;
   }

+  /* If load_library caused a re-execution with an expanded
+   * LD_LIBRARY_PATH, restore it back to its original contents, passed
+   * as the value of "reexeced_".  dlopen uses the value of
+   * LD_LIBRARY_PATH cached at program startup; our change is for the
+   * sake of child processes (such as --run) to see the same
+   * environment as the original nbdkit saw before re-exec.
+   */
+  if (reexeced) {
+    char *env = getenv ("LD_LIBRARY_PATH");
+
+    nbdkit_debug ("cleaning up after re-exec");
+    if (!env || strstr (env, reexeced) != 0 ||
+        (libdir && strncmp (env, libdir, strlen (libdir)) != 0)) {
+      nbdkit_error ("'reexeced_' set with garbled
environment");
+      return -1;
+    }
+    if (reexeced[0]) {
+      if (setenv ("LD_LIBRARY_PATH", reexeced, 1) == -1) {
+        nbdkit_error ("setenv: %m");
+        return -1;
+      }
+    }
+    else if (unsetenv ("LD_LIBRARY_PATH") == -1) {
+      nbdkit_error ("unsetenv: %m");
+      return -1;
+    }
+  }
+
   /* For remote connections, check all the parameters have been
    * passed.  Note that VDDK will segfault if parameters that it
    * expects are NULL (and there's no real way to tell what parameters
@@ -347,7 +469,7 @@ vddk_config_complete (void)
     VDDK_ERROR (err, "VixDiskLib_InitEx");
     exit (EXIT_FAILURE);
   }
-  init_called = 1;
+  init_called = true;

   return 0;
 }
-- 
2.24.1

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

On 2/18/20 11:05 AM, Eric Blake wrote:
> 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:
> 
> @@ -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 ||
Oops, this line needs to be squashed in 1/2.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Tue, Feb 18, 2020 at 11:05:11AM -0600, Eric Blake
wrote:
>    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",
Should we put #ifdef around the lib64/ and lib32/ versions?

Another alternative might be to get rid of the lib32/ version
entirely, only compile the plugin for x86-64, and set the minimum
version of VDDK in the docs to 5.5.5 (see nbdkit-vddk-plugin.pod
section "SUPPORTED VERSIONS OF VDDK").

But the series looks good now, so

ACK

Thanks for the tremendous amount of effort that went into the
apparently simple change :-)

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org