Libguestfs - Feb 2020 - [nbdkit PATCH v4 1/4] server: Export nbdkit_set_dlopen_prefix function

I'm a lot happier with this version: no mucking with dlmopen(). It
does add a bit more coupling between nbdkit proper and the vddk plugin
(namely, nbdkit now exports a new function that the vddk plugin relies
on), but patch 2 adds testsuite coverage of the new function so we
shouldn't regress.

Patch 1 and 2 are new, patch 3 is unchanged from when Rich posted it
in v2, and patch 4 is simplified somewhat from what I did in v3 (in
particular, I separated the testing of our dlopen shim into a separate
test in patch 2, rather than cramming into test-vddk.sh).

As before, I don't have the actual proprietary VDDK library installed,
so I'm still waiting on Rich's verdict that test-vddk-real.sh passes.

Eric Blake (2):
  server: Export nbdkit_set_dlopen_prefix function
  tests: Add coverage of new nbdkit_set_dlopen_prefix

Richard W.M. Jones (2):
  vddk: Delay loading VDDK until config_complete.
  vddk: Drive library loading from libdir parameter.

 docs/nbdkit-plugin.pod              |  14 +++
 include/nbdkit-common.h             |   2 +
 plugins/vddk/nbdkit-vddk-plugin.pod |  39 ++++++--
 plugins/vddk/vddk.c                 | 136 ++++++++++++++++++----------
 server/Makefile.am                  |  13 +++
 server/nbdkit.syms                  |   1 +
 server/shim.c                       |  99 ++++++++++++++++++++
 tests/Makefile.am                   |  25 +++++
 tests/test-dlopen-plugin.c          | 107 ++++++++++++++++++++++
 tests/test-dlopen.sh                |  52 +++++++++++
 tests/test-nbdkit-backend-debug.sh  |  26 +++---
 tests/test-vddk-real.sh             |  12 +--
 tests/test-vddk.sh                  |   6 +-
 13 files changed, 446 insertions(+), 86 deletions(-)
 create mode 100644 server/shim.c
 create mode 100644 tests/test-dlopen-plugin.c
 create mode 100755 tests/test-dlopen.sh

-- 
2.24.1

At least the VDDK plugin needs a way to load .so files with a custom
directory prepended.  Asking the user to set LD_LIBRARY_PATH is too
strong (that would leak into the child process created by --run, and
makes the user think about issues that we'd rather cover
automatically).  But overriding dlopen() to make it easier to prepend
a directory name under our control requires either a separate
namespace via dlmopen() (which itself is annoying - it messes up gdb
debugging), or that the override occur prior to any other library that
also depends on -ldl.  Which means that if we are going to provide
this functionality, we have to do so from nbdkit proper.

Note that properly injecting a dlopen shim for all subsequent shared
library loads requires that the override itself be in a shared
library.  Thus, nbdkit has to pull it in via a shared library link,
rather than merely a .o file, and we now have to install something in
pkglibdir.  What's more, now that we link against a shared library
instead of just libtool convenience libraries, libtool now creates a
wrapper app named lt-nbdkit so that it can run an in-tree build that
still locates the uninstalled shared library; this affects some
expected output in tests.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-plugin.pod             | 14 +++++
 include/nbdkit-common.h            |  2 +
 server/Makefile.am                 | 13 ++++
 server/nbdkit.syms                 |  1 +
 server/shim.c                      | 99 ++++++++++++++++++++++++++++++
 tests/test-nbdkit-backend-debug.sh | 26 ++++----
 6 files changed, 142 insertions(+), 13 deletions(-)
 create mode 100644 server/shim.c

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 41bffb7f..f3825067 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -1207,6 +1207,20 @@ and returns C<NULL>.

 The returned string must be freed by the caller.

+=head2 C<nbdkit_set_dlopen_prefix>
+
+ int nbdkit_set_dlopen_prefix (const char *prefix);
+
+Some plugins load a shared library that in turn uses L<dlopen(3)> to
+load further libraries.  If these libraries reside in non-standard
+locations, it may be necessary to prefix any bare library names with
+the desired directory to load them from.  Rather than requiring a user
+to amend C<LD_LIBRARY_PATH> in the calling environment (which would
+have knock-on effects to nbdkit and any child process spawned by the
+B<--run> argument), the plugin can request that nbdkit rewrites all
+dlopen requests that lack a slash character to instead attempt a
+dlopen of a file residing in C<prefix>.
+
 =head2 umask

 All plugins will see a L<umask(2)> of C<0022>.
diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index 50f3dd4f..44abce5e 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -83,6 +83,8 @@ extern void nbdkit_debug (const char *msg, ...)
ATTRIBUTE_FORMAT_PRINTF (1, 2);
 extern void nbdkit_vdebug (const char *msg, va_list args)
   ATTRIBUTE_FORMAT_PRINTF (1, 0);

+extern int nbdkit_set_dlopen_prefix (const char *newdir);
+
 extern char *nbdkit_absolute_path (const char *path);
 extern int64_t nbdkit_parse_size (const char *str);
 extern int nbdkit_parse_bool (const char *str);
diff --git a/server/Makefile.am b/server/Makefile.am
index 9351fefc..b6728511 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -33,6 +33,7 @@ include $(top_srcdir)/common-rules.mk

 EXTRA_DIST = nbdkit.syms

+pkglib_LTLIBRARIES = nbdkit-shim-dlopen.la
 sbin_PROGRAMS = nbdkit

 nbdkit_SOURCES = \
@@ -71,6 +72,17 @@ if ENABLE_LIBFUZZER
 nbdkit_SOURCES += fuzzer.c
 endif

+nbdkit_shim_dlopen_la_SOURCES = shim.c
+nbdkit_shim_dlopen_la_CPPFLAGS = \
+	-I$(top_srcdir)/common/utils \
+	$(NULL)
+nbdkit_shim_dlopen_la_CFLAGS = $(WARNINGS_CFLAGS)
+nbdkit_shim_dlopen_la_LIBADD = $(DL_LIBS)
+nbdkit_shim_dlopen_la_LDFLAGS = \
+	-module -no-undefined -shared -avoid-version \
+	$(DL_LDFLAGS) \
+	$(NULL)
+
 nbdkit_CPPFLAGS = \
 	-Dbindir=\"$(bindir)\" \
 	-Dlibdir=\"$(libdir)\" \
@@ -92,6 +104,7 @@ nbdkit_CFLAGS = \
 	$(VALGRIND_CFLAGS) \
 	$(NULL)
 nbdkit_LDADD = \
+	nbdkit-shim-dlopen.la \
 	$(GNUTLS_LIBS) \
 	$(LIBSELINUX_LIBS) \
 	$(DL_LIBS) \
diff --git a/server/nbdkit.syms b/server/nbdkit.syms
index 96c22c07..d20e0784 100644
--- a/server/nbdkit.syms
+++ b/server/nbdkit.syms
@@ -63,6 +63,7 @@
     nbdkit_peer_name;
     nbdkit_read_password;
     nbdkit_realpath;
+    nbdkit_set_dlopen_prefix;
     nbdkit_set_error;
     nbdkit_vdebug;
     nbdkit_verror;
diff --git a/server/shim.c b/server/shim.c
new file mode 100644
index 00000000..a2682820
--- /dev/null
+++ b/server/shim.c
@@ -0,0 +1,99 @@
+/* nbdkit
+ * Copyright (C) 2020 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+/* This file provides a shim around dlopen, to aid plugins such as
+ * vddk that want to convert relative shared library requests from
+ * subsequent code into absolute requests.  This library has to be a
+ * load-time dependency of the main nbdkit executable prior to the
+ * library providing the real dlopen (libdl on Linux, libc on BSD).
+ */
+
+#include <config.h>
+
+#include <dlfcn.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "internal.h"
+
+static 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.
+ */
+static char *dir;
+
+int
+nbdkit_set_dlopen_prefix (const char *newdir)
+{
+  free (dir);
+  if (newdir) {
+    dir = strdup (newdir);
+    if (!dir) {
+      nbdkit_error ("strdup: %m");
+      return -1;
+    }
+  }
+  else
+    dir = NULL;
+  return 0;
+}
+
+void *
+dlopen (const char *filename, int flags)
+{
+  /* Using CLEANUP_FREE would make this shared library bigger. */
+  char *tmp = NULL;
+  void *ret;
+
+  if (dir && ! strchr (filename, '/')) {
+    /* Caller expects failure to set dlerror, so we still have to call
+     * dlopen; the best we can do is log our allocation failure.
+     */
+    if (asprintf (&tmp, "%s/%s", dir, filename) >= 0)
+      filename = tmp;
+    else
+      nbdkit_debug ("dlopen: failed to allocate replacement for %s",
filename);
+  }
+
+  ret = orig_dlopen (filename, flags);
+  free (tmp);
+  return ret;
+}
+
+static void constructor (void) __attribute__ ((constructor));
+static void
+constructor (void)
+{
+  orig_dlopen = dlsym (RTLD_NEXT, "dlopen");
+}
diff --git a/tests/test-nbdkit-backend-debug.sh
b/tests/test-nbdkit-backend-debug.sh
index 3a28b756..8fe78b48 100755
--- a/tests/test-nbdkit-backend-debug.sh
+++ b/tests/test-nbdkit-backend-debug.sh
@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 # nbdkit
-# Copyright (C) 2019 Red Hat Inc.
+# Copyright (C) 2019-2020 Red Hat Inc.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
@@ -49,10 +49,10 @@ nbdkit -U - \
        --run "qemu-img convert \$nbd $out" |& tee $debug

 # Should contain all debugging messages.
-grep '^nbdkit:.*debug: nofilter: open' $debug
-grep '^nbdkit:.*debug: memory: open' $debug
-grep '^nbdkit:.*debug: nofilter: pread' $debug
-grep '^nbdkit:.*debug: memory: pread' $debug
+grep 'nbdkit:.*debug: nofilter: open' $debug
+grep 'nbdkit:.*debug: memory: open' $debug
+grep 'nbdkit:.*debug: nofilter: pread' $debug
+grep 'nbdkit:.*debug: memory: pread' $debug

 nbdkit -U - \
        -v -D nbdkit.backend.controlpath=0 \
@@ -61,10 +61,10 @@ nbdkit -U - \
        --run "qemu-img convert \$nbd $out" |& tee $debug

 # Should contain only datapath messages.
-grep -v '^nbdkit:.*debug: nofilter: open' $debug
-grep -v '^nbdkit:.*debug: memory: open' $debug
-grep '^nbdkit:.*debug: nofilter: pread' $debug
-grep '^nbdkit:.*debug: memory: pread' $debug
+grep -v 'nbdkit:.*debug: nofilter: open' $debug
+grep -v 'nbdkit:.*debug: memory: open' $debug
+grep 'nbdkit:.*debug: nofilter: pread' $debug
+grep 'nbdkit:.*debug: memory: pread' $debug

 nbdkit -U - \
        -v -D nbdkit.backend.datapath=0 \
@@ -73,7 +73,7 @@ nbdkit -U - \
        --run "qemu-img convert \$nbd $out" |& tee $debug

 # Should contain only controlpath messages.
-grep '^nbdkit:.*debug: nofilter: open' $debug
-grep '^nbdkit:.*debug: memory: open' $debug
-grep -v '^nbdkit:.*debug: nofilter: pread' $debug
-grep -v '^nbdkit:.*debug: memory: pread' $debug
+grep 'nbdkit:.*debug: nofilter: open' $debug
+grep 'nbdkit:.*debug: memory: open' $debug
+grep -v 'nbdkit:.*debug: nofilter: pread' $debug
+grep -v 'nbdkit:.*debug: memory: pread' $debug
-- 
2.24.1

Although the real reason for adding the new interface is the vddk
plugin, that one is harder to test (not everyone configures it to be
built); so adding a standalone plugin that does the bare minimum to
validate our code is worthwhile.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/Makefile.am          |  25 +++++++++
 tests/test-dlopen-plugin.c | 107 +++++++++++++++++++++++++++++++++++++
 tests/test-dlopen.sh       |  52 ++++++++++++++++++
 3 files changed, 184 insertions(+)
 create mode 100644 tests/test-dlopen-plugin.c
 create mode 100755 tests/test-dlopen.sh

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 8b1638ef..369c0e6f 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -100,6 +100,7 @@ EXTRA_DIST = \
 	test-data-extents.sh \
 	test-data-file.sh \
 	test-data-raw.sh \
+	test-dlopen.sh \
 	test-debug-flags.sh \
 	test-dump-config.sh \
 	test-dump-plugin.sh \
@@ -374,6 +375,30 @@ test_cxx_filter_la_LDFLAGS = \
 	$(NULL)
 endif HAVE_CXX

+# This builds a plugin that calls dlopen, to test our dlopen override
+# that supplies a prefix.
+TESTS += test-dlopen.sh
+# check_LTLIBRARIES won't build a shared library (see automake manual).
+# So we have to do this and add a dependency.
+noinst_LTLIBRARIES += test-dlopen-plugin.la
+test-dlopen.sh: test-dlopen-plugin.la
+
+test_dlopen_plugin_la_SOURCES = \
+	test-dlopen-plugin.c \
+	$(top_srcdir)/include/nbdkit-plugin.h \
+	$(NULL)
+test_dlopen_plugin_la_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	$(NULL)
+test_dlopen_plugin_la_CFLAGS = $(WARNINGS_CFLAGS)
+test_dlopen_plugin_la_LIBADD = $(DL_LIBS)
+# For use of the -rpath option, see:
+# https://lists.gnu.org/archive/html/libtool/2007-07/msg00067.html
+test_dlopen_plugin_la_LDFLAGS = \
+	-module -avoid-version -shared -rpath /nowhere \
+	$(DL_LDFLAGS) \
+	$(NULL)
+
 # Exit with parent test.
 check_PROGRAMS += test-exit-with-parent
 TESTS += test-exit-with-parent
diff --git a/tests/test-dlopen-plugin.c b/tests/test-dlopen-plugin.c
new file mode 100644
index 00000000..86f09e12
--- /dev/null
+++ b/tests/test-dlopen-plugin.c
@@ -0,0 +1,107 @@
+/* nbdkit
+ * Copyright (C) 2020 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <assert.h>
+#include <dlfcn.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <nbdkit-plugin.h>
+
+#define THREAD_MODEL NBDKIT_THREAD_MODEL_PARALLEL
+
+static const char *libdir;
+
+static int
+dlopen_config (const char *key, const char *value)
+{
+  if (strcmp (key, "libdir") == 0)
+    libdir = value;
+  return 0;
+}
+
+static int
+dlopen_config_complete (void)
+{
+  char *msg;
+
+  if (libdir) {
+    if (nbdkit_set_dlopen_prefix (libdir) == -1)
+      return -1;
+  }
+
+  /* The main goal of this plugin is to show that we can hook dlopen.
+   * Test this by requesting a library that doesn't exist, where the
+   * difference in the error message shows if we were successful.
+   */
+  dlopen ("no_one_has_a_library_by_this_name", RTLD_NOW);
+  msg = dlerror ();
+  assert (msg);
+  nbdkit_error ("dlopen failed as expected: %s\n", msg);
+
+  return 0;
+}
+
+/* These must be provided, but we don't really need to use them. */
+static void *
+dlopen_open (int readonly)
+{
+  return NBDKIT_HANDLE_NOT_NEEDED;
+}
+
+static int64_t
+dlopen_get_size (void *handle)
+{
+  return 0;
+}
+
+static int
+dlopen_pread (void *handle, void *buf, uint32_t count, uint64_t offset)
+{
+  assert (false);
+}
+
+static struct nbdkit_plugin plugin = {
+  .name              = "dlopenplugin",
+  .version           = PACKAGE_VERSION,
+  .config            = dlopen_config,
+  .config_complete   = dlopen_config_complete,
+  .open              = dlopen_open,
+  .get_size          = dlopen_get_size,
+  .pread             = dlopen_pread,
+};
+
+NBDKIT_REGISTER_PLUGIN(plugin)
diff --git a/tests/test-dlopen.sh b/tests/test-dlopen.sh
new file mode 100755
index 00000000..85db3172
--- /dev/null
+++ b/tests/test-dlopen.sh
@@ -0,0 +1,52 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2020 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+source ./functions.sh
+set -e
+set -x
+
+files="test-dlopen.err"
+rm -f $files
+cleanup_fn rm -f $files
+
+libs=./.libs
+
+# First: show that without using the new function, dlopen complains
+# about a relative request.
+nbdkit $libs/test-dlopen-plugin.so --run : 2> test-dlopen.err
+cat test-dlopen.err
+grep "as expected: no_one_has" test-dlopen.err
+
+# Now, show that we can alter the message based on our config parameter.
+nbdkit $libs/test-dlopen-plugin.so libdir="$PWD" --run : 2>
test-dlopen.err
+cat test-dlopen.err
+grep -F "as expected: $PWD/no_one_has" test-dlopen.err
-- 
2.24.1

From: "Richard W.M. Jones" <rjones@redhat.com>

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).
Message-Id: <20200213160449.732936-2-rjones@redhat.com>
---
 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.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 relying on the fact that nbdkit
just added a way to request a prefix be added to any relative dlopen()
requests, feeding it the libdir= parameter learned during .config
after we confirm that directory worked for loading VDDK.

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.
[eblake: Several modifications to Rich's initial patch, mainly to take
advantage of the new nbdkit_set_dlopen_prefix]
---
 plugins/vddk/nbdkit-vddk-plugin.pod | 39 +++++++++++++++++++------
 plugins/vddk/vddk.c                 | 44 +++++++++++++++++++++++++----
 tests/test-vddk-real.sh             | 12 ++------
 tests/test-vddk.sh                  |  6 ++--
 4 files changed, 73 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/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index db61c1d8..bad38120 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 <libgen.h>

 #define NBDKIT_API_VERSION 2

@@ -247,8 +248,14 @@ 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.  This list
+     * intentionally allows a user to point to either the top-level
+     * installation directory, or the lib64/ subdirectory thereof, in
+     * part because it makes our test setup easier.
+     */
+    "lib64/libvixDiskLib.so.6",
     "libvixDiskLib.so.6",
+    "lib64/libvixDiskLib.so.5",
     "libvixDiskLib.so.5",
   };
   size_t i;
@@ -256,9 +263,28 @@ 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, inform nbdkit's dlopen shim
+       * about our desired prefix based on libdir for all future
+       * loads.  This may modify path in-place, but that's okay.
+       */
+      char *dir = dirname (path);
+      if (nbdkit_set_dlopen_prefix (dir) == -1)
+        exit (EXIT_FAILURE);
+      nbdkit_debug("dlopen shim prefix set to %s", dir);
       break;
+    }
     if (i == 0) {
       orig_error = dlerror ();
       if (orig_error)
@@ -268,7 +294,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);
@@ -331,6 +357,14 @@ 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. */
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..637d5442 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
@@ -37,9 +37,7 @@ set -x
 rm -f test-vddk.out
 cleanup_fn rm -f test-vddk.out

-LD_LIBRARY_PATH=.libs:$LD_LIBRARY_PATH \
-LIBRARY_PATH=.libs:$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
-- 
2.24.1

On 2/15/20 10:22 PM, Eric Blake wrote:
> At least the VDDK plugin needs a way to load .so files with a custom
> directory prepended.  Asking the user to set LD_LIBRARY_PATH is too
> strong (that would leak into the child process created by --run, and
> makes the user think about issues that we'd rather cover
> automatically).  But overriding dlopen() to make it easier to prepend
> a directory name under our control requires either a separate
> namespace via dlmopen() (which itself is annoying - it messes up gdb
> debugging), or that the override occur prior to any other library that
> also depends on -ldl.  Which means that if we are going to provide
> this functionality, we have to do so from nbdkit proper.
> 
> Note that properly injecting a dlopen shim for all subsequent shared
> library loads requires that the override itself be in a shared
> library.  Thus, nbdkit has to pull it in via a shared library link,
> rather than merely a .o file, and we now have to install something in
> pkglibdir.  What's more, now that we link against a shared library
> instead of just libtool convenience libraries, libtool now creates a
> wrapper app named lt-nbdkit so that it can run an in-tree build that
> still locates the uninstalled shared library; this affects some
> expected output in tests.
The wrapper script also needs a tweak thanks to libtool's behavior; I'll
have to squash this in:

diff --git i/wrapper.c w/wrapper.c
index 6aef81a1..b0b9c158 100644
--- i/wrapper.c
+++ w/wrapper.c
@@ -1,5 +1,5 @@
  /* nbdkit
- * Copyright (C) 2017-2019 Red Hat Inc.
+ * Copyright (C) 2017-2020 Red Hat Inc.
   *
   * Redistribution and use in source and binary forms, with or without
   * modification, are permitted provided that the following conditions are
@@ -148,6 +148,8 @@ main (int argc, char *argv[])
     */
    s = getenv ("NBDKIT_VALGRIND");
    if (s && strcmp (s, "1") == 0) {
+    passthru ("libtool");
+    passthru ("execute");
      passthru (VALGRIND);
      passthru ("--vgdb=no");
      passthru ("--leak-check=full");
@@ -163,6 +165,8 @@ main (int argc, char *argv[])
    else {
      s = getenv ("NBDKIT_GDB");
      if (s && strcmp (s, "1") == 0) {
+      passthru ("libtool");
+      passthru ("execute");
        passthru ("gdb");
        passthru ("--args");
      }



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

On 2/15/20 10:22 PM, Eric Blake wrote:
> Although the real reason for adding the new interface is the vddk
> plugin, that one is harder to test (not everyone configures it to be
> built); so adding a standalone plugin that does the bare minimum to
> validate our code is worthwhile.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> +++ b/tests/test-dlopen.sh
> +
> +# First: show that without using the new function, dlopen complains
> +# about a relative request.
> +nbdkit $libs/test-dlopen-plugin.so --run : 2> test-dlopen.err
Needs to use -U -, otherwise this fails if port 10809 is in use.
> +cat test-dlopen.err
> +grep "as expected: no_one_has" test-dlopen.err
> +
> +# Now, show that we can alter the message based on our config parameter.
> +nbdkit $libs/test-dlopen-plugin.so libdir="$PWD" --run : 2>
test-dlopen.err
> +cat test-dlopen.err
> +grep -F "as expected: $PWD/no_one_has" test-dlopen.err
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org