Eric Blake
2020-Feb-17 17:14 UTC
[Libguestfs] [nbdkit PATCH v5 0/4] vddk: Drive library loading from libdir parameter.
Differences from v4: Patch 1 is simplified: I realized that since we already use -rdynamic for nbdkit (after all, we WANT our dlopen()d plugins to be able to call our nbdkit_* exports), it is only a matter of adding dlopen to the set of symbols that we export. With that done, there is no separate shared library needed; our dlopen shim is now part of nbdkit proper, and we don't have to tweak the wrapper script or install a separate file. Patches 2 and 4 had minor rebase churn, patch 3 remains untouched. 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 | 3 +- server/nbdkit.syms | 5 +- server/shim.c | 97 ++++++++++++++++++++ tests/Makefile.am | 25 +++++ tests/test-dlopen-plugin.c | 106 ++++++++++++++++++++++ tests/test-dlopen.sh | 54 +++++++++++ tests/test-vddk-real.sh | 10 +- tests/test-vddk.sh | 6 +- 12 files changed, 422 insertions(+), 75 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
Eric Blake
2020-Feb-17 17:14 UTC
[Libguestfs] [nbdkit PATCH v5 1/4] server: Export nbdkit_set_dlopen_prefix function
At least the VDDK plugin needs a way to load .so files with a custom
directory prepended. We have previously documented that a user can do
this by overriding LD_LIBRARY_PATH in the environment prior to
starting nbdkit, but that is rather strong (that variable leaks into
the child process created by --run, and makes the user think about
issues that we'd rather cover automatically; and while vddk's .load
could call setenv() prior to when the --run child gets forked to scrub
LD_LIBRARY_PATH, it is hard to discern if scrubbing it would go
against the user's intentions to have it pass unmodified to the
child). So a better option is to make it possible to perform a
dynamic override of dlopen() where we can limit the scope of the
override to JUST the nbdkit process, without the user having to mess
with environment variables.
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
dlopen() (either by a shared library linked in before -ldl, or by
using -rdynamic - thankfully, we are already using -rdynamic, as shown
by nbdkit.syms). But it does mean that if we are going to provide
dlopen() enhancement functionality, we have to do so from nbdkit
proper.
Signed-off-by: Eric Blake <eblake@redhat.com>
---
docs/nbdkit-plugin.pod | 14 ++++++
include/nbdkit-common.h | 2 +
server/Makefile.am | 3 +-
server/nbdkit.syms | 5 ++-
server/shim.c | 97 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 119 insertions(+), 2 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..e184c66e 100644
--- a/server/Makefile.am
+++ b/server/Makefile.am
@@ -59,6 +59,7 @@ nbdkit_SOURCES = \
protocol-handshake-newstyle.c \
public.c \
quit.c \
+ shim.c \
signals.c \
socket-activation.c \
sockets.c \
@@ -116,7 +117,7 @@ endif
BUILT_SOURCES = synopsis.c
EXTRA_DIST += synopsis.c
-EXTRA_nbdkit_DEPENDENCIES = synopsis.c
+EXTRA_nbdkit_DEPENDENCIES = synopsis.c nbdkit.syms
CLEANFILES += synopsis.c
main.c: synopsis.c
synopsis.c: $(top_srcdir)/docs/synopsis.txt
diff --git a/server/nbdkit.syms b/server/nbdkit.syms
index 96c22c07..0255168e 100644
--- a/server/nbdkit.syms
+++ b/server/nbdkit.syms
@@ -36,8 +36,10 @@
# to export.
{
- # The functions we want plugins and filters to call.
+ # The functions we want plugins and filters to call. The export of
+ # dlopen is needed for nbdkit_set_dlopen_prefix (see shim.c).
global:
+ dlopen;
nbdkit_absolute_path;
nbdkit_add_extent;
nbdkit_debug;
@@ -63,6 +65,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..dc8bdbc7
--- /dev/null
+++ b/server/shim.c
@@ -0,0 +1,97 @@
+/* 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. For subsequent shared
+ * libraries to see our shim rather than the original dlopen (from
+ * libdl on Linux or libc on BSD), we have to export it from
+ * nbdkit.syms.
+ */
+
+#include <config.h>
+
+#include <dlfcn.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "internal.h"
+#include "cleanup.h"
+
+static void *(*orig_dlopen) (const char *filename, int flags);
+
+/* NULL for normal behavior, or set when we want to override a
+ * relative dlopen() to instead use dir to form an absolute name.
+ */
+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)
+{
+ CLEANUP_FREE char *tmp = NULL;
+
+ 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);
+ }
+
+ return orig_dlopen (filename, flags);
+}
+
+static void constructor (void) __attribute__ ((constructor));
+static void
+constructor (void)
+{
+ orig_dlopen = dlsym (RTLD_NEXT, "dlopen");
+}
--
2.24.1
Eric Blake
2020-Feb-17 17:14 UTC
[Libguestfs] [nbdkit PATCH v5 2/4] tests: Add coverage of new nbdkit_set_dlopen_prefix
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 | 106 +++++++++++++++++++++++++++++++++++++
tests/test-dlopen.sh | 54 +++++++++++++++++++
3 files changed, 185 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..5c81800b
--- /dev/null
+++ b/tests/test-dlopen-plugin.c
@@ -0,0 +1,106 @@
+/* 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 <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 (0);
+}
+
+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..2a13cb65
--- /dev/null
+++ b/tests/test-dlopen.sh
@@ -0,0 +1,54 @@
+#!/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 -U - $libs/test-dlopen-plugin.so --run 'true' \
+ 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 -U - $libs/test-dlopen-plugin.so libdir="$PWD" --run
'true' \
+ 2> test-dlopen.err
+cat test-dlopen.err
+grep -F "as expected: $PWD/no_one_has" test-dlopen.err
--
2.24.1
Eric Blake
2020-Feb-17 17:14 UTC
[Libguestfs] [nbdkit PATCH v5 3/4] vddk: Delay loading VDDK until config_complete.
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 ad2b01f1..0abec68e 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
Eric Blake
2020-Feb-17 17:14 UTC
[Libguestfs] [nbdkit PATCH v5 4/4] vddk: Drive library loading from libdir parameter.
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 | 10 +------
tests/test-vddk.sh | 6 +---
4 files changed, 71 insertions(+), 28 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 0abec68e..d3c4483e 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 3a888eac..f155025d 100755
--- a/tests/test-vddk-real.sh
+++ b/tests/test-vddk-real.sh
@@ -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" 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 9ee46aa8..927e29fd 100755
--- a/tests/test-vddk.sh
+++ b/tests/test-vddk.sh
@@ -37,15 +37,11 @@ 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
# Also test our magic file= handling, even though the dummy driver doesn't
# really open a file.
-LD_LIBRARY_PATH=.libs:$LD_LIBRARY_PATH \
-LIBRARY_PATH=.libs:$LIBRARY_PATH \
nbdkit -U - vddk libdir=.libs /dev/null --run true
--
2.24.1
Richard W.M. Jones
2020-Feb-17 18:17 UTC
Re: [Libguestfs] [nbdkit PATCH v5 4/4] vddk: Drive library loading from libdir parameter.
Unfortunately this series doesn't work with the real VDDK library :-(
I see:
nbdkit: debug: vddk: config_complete
nbdkit: debug: dlopen shim prefix set to
/home/rjones/tmp/vddk-6.7.3/vmware-vix-disklib-distrib/lib64
Then a little bit later during VDDK initialization it fails with:
nbdkit: debug: VixDiskLib: Could not load default plugins from
/home/rjones/tmp/vddk-6.7.3/vmware-vix-disklib-distrib/lib64/libdiskLibPlugin.so:
Cannot open library: libcrypto.so.1.0.2: cannot open shared object file: No such
file or directory.
I was curious if the replacement dlopen() was being called at all, and
it is certainly called from plugins/vddk/vddk.c:
nbdkit: debug: dlopen:
/home/rjones/tmp/vddk-6.7.3/vmware-vix-disklib-distrib/lib64/libvixDiskLib.so.6
flags=2
It's also called from VDDK itself:
nbdkit: debug: dlopen:
/home/rjones/tmp/vddk-6.7.3/vmware-vix-disklib-distrib/lib64/libdiskLibPlugin.so
flags=257
but it's not being explicitly called for libcrypto.so because
immediately after the above message I see the error:
nbdkit: debug: VixDiskLib: Could not load default plugins from
/home/rjones/tmp/vddk-6.7.3/vmware-vix-disklib-distrib/lib64/libdiskLibPlugin.so:
Cannot open library: libcrypto.so.1.0.2: cannot open shared object file: No such
file or directory.
So I guess libcrypto is actually loaded from a DT_NEEDED entry:
$ eu-readelf -d
/home/rjones/tmp/vddk-6.7.3/vmware-vix-disklib-distrib/lib64/libdiskLibPlugin.so
| grep NEEDED
NEEDED Shared library: [libpthread.so.0]
NEEDED Shared library: [libstdc++.so.6]
NEEDED Shared library: [libdl.so.2]
NEEDED Shared library: [libcrypto.so.1.0.2]
NEEDED Shared library: [libssl.so.1.0.2]
NEEDED Shared library: [libexpat.so]
NEEDED Shared library: [libz.so.1]
NEEDED Shared library: [libvmacore.so]
NEEDED Shared library: [libvmomi.so]
NEEDED Shared library: [libvim-types.so]
NEEDED Shared library: [librt.so.1]
NEEDED Shared library: [libgcc_s.so.1]
NEEDED Shared library: [libc.so.6]
NEEDED Shared library: [ld-linux-x86-64.so.2]
which I suppose doesn't actually go through dlopen ...
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines. Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v
Reasonably Related Threads
- [nbdkit PATCH v4 0/4] vddk: Drive library loading from libdir parameter.
- [PATCH nbdkit v2 2/3] NOT WORKING: vddk: Drive library loading from libdir parameter.
- [PATCH nbdkit 1/2] vddk: Delay loading VDDK until config_complete.
- [nbdkit PATCH v7 0/2] vddk: Drive library loading from libdir parameter.
- Re: [nbdkit PATCH v5 4/4] vddk: Drive library loading from libdir parameter.