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

We were previously dlopen-ing it in the load() method.  This is very
early and in particular means that the only possible way to configure
where we find the library is through environment variables and not
through config parameters.  Also it's not necessary as we don't call
any functions from the library (such as VixDiskLib_InitEx) until
config_complete.

This change is neutral refactoring as currently we _do_ configure the
location through an environment variable (LD_LIBRARY_PATH).
---
 plugins/vddk/vddk.c | 100 ++++++++++++++++++++++----------------------
 1 file changed, 51 insertions(+), 49 deletions(-)

diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 5d3764d6..db61c1d8 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -143,54 +143,7 @@ error_function (const char *fs, va_list args)
   nbdkit_error ("%s", str);
 }
 
-/* Load and unload the plugin. */
-static void
-vddk_load (void)
-{
-  static const char *sonames[] = {
-    /* Prefer the newest library in case multiple exist. */
-    "libvixDiskLib.so.6",
-    "libvixDiskLib.so.5",
-  };
-  size_t i;
-  CLEANUP_FREE char *orig_error = NULL;
-
-  /* Load the library. */
-  for (i = 0; i < sizeof sonames / sizeof sonames[0]; ++i) {
-    dl = dlopen (sonames[i], RTLD_NOW);
-    if (dl != NULL)
-      break;
-    if (i == 0) {
-      orig_error = dlerror ();
-      if (orig_error)
-        orig_error = strdup (orig_error);
-    }
-  }
-  if (dl == NULL) {
-    nbdkit_error ("%s\n\n"
-                  "If '%s' is located on a non-standard path you
may need to\n"
-                  "set $LD_LIBRARY_PATH or edit /etc/ld.so.conf.\n\n"
-                  "See the nbdkit-vddk-plugin(1) man page for
details.",
-                  orig_error ? : "(unknown error)", sonames[0]);
-    exit (EXIT_FAILURE);
-  }
-
-  /* Load symbols. */
-#define STUB(fn,ret,args)                                         \
-  do {                                                            \
-    fn = dlsym (dl, #fn);                                         \
-    if (fn == NULL) {                                             \
-      nbdkit_error ("required VDDK symbol \"%s\" is missing:
%s", \
-                    #fn, dlerror ());                             \
-      exit (EXIT_FAILURE);                                        \
-    }                                                             \
-  } while (0)
-#define OPTIONAL_STUB(fn,ret,args) fn = dlsym (dl, #fn)
-#include "vddk-stubs.h"
-#undef STUB
-#undef OPTIONAL_STUB
-}
-
+/* Unload the plugin. */
 static void
 vddk_unload (void)
 {
@@ -289,6 +242,54 @@ vddk_config (const char *key, const char *value)
   return 0;
 }
 
+/* Load the VDDK library. */
+static void
+load_library (void)
+{
+  static const char *sonames[] = {
+    /* Prefer the newest library in case multiple exist. */
+    "libvixDiskLib.so.6",
+    "libvixDiskLib.so.5",
+  };
+  size_t i;
+  CLEANUP_FREE char *orig_error = NULL;
+
+  /* Load the library. */
+  for (i = 0; i < sizeof sonames / sizeof sonames[0]; ++i) {
+    dl = dlopen (sonames[i], RTLD_NOW);
+    if (dl != NULL)
+      break;
+    if (i == 0) {
+      orig_error = dlerror ();
+      if (orig_error)
+        orig_error = strdup (orig_error);
+    }
+  }
+  if (dl == NULL) {
+    nbdkit_error ("%s\n\n"
+                  "If '%s' is located on a non-standard path you
may need to\n"
+                  "set $LD_LIBRARY_PATH or edit /etc/ld.so.conf.\n\n"
+                  "See the nbdkit-vddk-plugin(1) man page for
details.",
+                  orig_error ? : "(unknown error)", sonames[0]);
+    exit (EXIT_FAILURE);
+  }
+
+  /* Load symbols. */
+#define STUB(fn,ret,args)                                         \
+  do {                                                            \
+    fn = dlsym (dl, #fn);                                         \
+    if (fn == NULL) {                                             \
+      nbdkit_error ("required VDDK symbol \"%s\" is missing:
%s", \
+                    #fn, dlerror ());                             \
+      exit (EXIT_FAILURE);                                        \
+    }                                                             \
+  } while (0)
+#define OPTIONAL_STUB(fn,ret,args) fn = dlsym (dl, #fn)
+#include "vddk-stubs.h"
+#undef STUB
+#undef OPTIONAL_STUB
+}
+
 static int
 vddk_config_complete (void)
 {
@@ -330,6 +331,8 @@ vddk_config_complete (void)
 #undef missing
   }
 
+  load_library ();
+
   /* Initialize VDDK library. */
   DEBUG_CALL ("VixDiskLib_InitEx",
               "%d, %d, &debug_fn, &error_fn, &error_fn, %s,
%s",
@@ -831,7 +834,6 @@ static struct nbdkit_plugin plugin = {
   .name              = "vddk",
   .longname          = "VMware VDDK plugin",
   .version           = PACKAGE_VERSION,
-  .load              = vddk_load,
   .unload            = vddk_unload,
   .config            = vddk_config,
   .config_complete   = vddk_config_complete,
-- 
2.25.0

Do not use LD_LIBRARY_PATH to locate the VDDK library.  Setting this
always causes problems because VDDK comes bundled with broken
replacements for system libraries, such as libcrypto.so and
libstdc++.so.  Two problems this causes which we have seen in the real
world:

(1) User does ‘export LD_LIBRARY_PATH=vmware-vix-disklib-distrib’ and
that breaks lots of ordinary utilities on their system.

(2) nbdkit vddk --run subcommand inherits the LD_LIBRARY_PATH
environment variable from nbdkit, and common commands such as
'qemu-img' break, relying on complex workarounds like saving and
restoring the original LD_LIBRARY_PATH in the subcommand.

Instead rely on a relatively undocumented feature of dlopen which is
that when we pass in a full path it will try to load dependent
libraries from the same directory.

Note this may break some callers who are not using libdir and
expecting LD_LIBRARY_PATH to work, so it's a change in behaviour which
we will have to highlight prominently in the 1.18 release notes.

Thanks: Dan Berrange, Ming Xie, Eric Blake.
---
 plugins/vddk/nbdkit-vddk-plugin.pod | 29 ++++++++++++++++++++---------
 configure.ac                        |  1 +
 plugins/vddk/vddk.c                 | 15 +++++++++++++--
 tests/test-vddk-real.sh             | 12 ++----------
 tests/test-vddk.sh                  | 19 +++++++++++++------
 5 files changed, 49 insertions(+), 27 deletions(-)

diff --git a/plugins/vddk/nbdkit-vddk-plugin.pod
b/plugins/vddk/nbdkit-vddk-plugin.pod
index f34b9fba..5f32988b 100644
--- a/plugins/vddk/nbdkit-vddk-plugin.pod
+++ b/plugins/vddk/nbdkit-vddk-plugin.pod
@@ -230,17 +230,22 @@ 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 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 can two choices: 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
+
+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
@@ -250,6 +255,12 @@ configuration file, usually including
F</etc/vmware/config> and
 F<$HOME/.vmware/config>, but you can override this using the
C<config>
 parameter.
 
+=head2 No need to set C<LD_LIBRARY_PATH>
+
+In nbdkit E<le> 1.16 you had to set the environment variable
+C<LD_LIBRARY_PATH> when using this plugin.  In nbdkit E<ge> 1.18
this
+is I<not> recommended.
+
 =head1 FILE PARAMETER
 
 The C<file> parameter can either be a local file, in which case it
diff --git a/configure.ac b/configure.ac
index fa902945..d71f06e4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -130,6 +130,7 @@ AC_PROG_INSTALL
 AC_PROG_CPP
 AC_CANONICAL_HOST
 AC_SYS_LARGEFILE
+AC_CHECK_SIZEOF([long])
 
 AC_C_PROTOTYPES
 test "x$U" != "x" && AC_MSG_ERROR([Compiler not
ANSI compliant])
diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index db61c1d8..c49eebcd 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -256,7 +256,18 @@ load_library (void)
 
   /* Load the library. */
   for (i = 0; i < sizeof sonames / sizeof sonames[0]; ++i) {
-    dl = dlopen (sonames[i], RTLD_NOW);
+    CLEANUP_FREE char *path;
+
+    /* Set the full path so that dlopen will preferentially load the
+     * system libraries from the same directory.
+     */
+    if (asprintf (&path, "%s/lib%d/%s",
+                  libdir ? : VDDK_LIBDIR, 8*SIZEOF_LONG, sonames[i]) == -1) {
+      nbdkit_error ("asprintf: %m");
+      exit (EXIT_FAILURE);
+    }
+
+    dl = dlopen (path, RTLD_NOW);
     if (dl != NULL)
       break;
     if (i == 0) {
@@ -268,7 +279,7 @@ load_library (void)
   if (dl == NULL) {
     nbdkit_error ("%s\n\n"
                   "If '%s' is located on a non-standard path you
may need to\n"
-                  "set $LD_LIBRARY_PATH or edit /etc/ld.so.conf.\n\n"
+                  "set
libdir=/path/to/vmware-vix-disklib-distrib.\n\n"
                   "See the nbdkit-vddk-plugin(1) man page for
details.",
                   orig_error ? : "(unknown error)", sonames[0]);
     exit (EXIT_FAILURE);
diff --git a/tests/test-vddk-real.sh b/tests/test-vddk-real.sh
index 52c91232..d9cf3319 100755
--- a/tests/test-vddk-real.sh
+++ b/tests/test-vddk-real.sh
@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 # nbdkit
-# Copyright (C) 2018-2019 Red Hat Inc.
+# Copyright (C) 2018-2020 Red Hat Inc.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
@@ -51,15 +51,7 @@ cleanup_fn rm -f $files
 
 qemu-img create -f vmdk test-vddk-real.vmdk 100M
 
-export old_ld_library_path="$LD_LIBRARY_PATH"
-export LD_LIBRARY_PATH="$vddkdir/lib64:$LD_LIBRARY_PATH"
-
 nbdkit -f -v -U - \
        --filter=readahead \
        vddk libdir="$vddkdir" file=test-vddk-real.vmdk \
-       --run '
-       # VDDK library path breaks qemu-img, we must restore the
-       # original path here.
-       export LD_LIBRARY_PATH="$old_ld_library_path"
-       qemu-img convert $nbd -O raw test-vddk-real.out
-'
+       --run 'qemu-img convert $nbd -O raw test-vddk-real.out'
diff --git a/tests/test-vddk.sh b/tests/test-vddk.sh
index 19b946b6..c2df4d69 100755
--- a/tests/test-vddk.sh
+++ b/tests/test-vddk.sh
@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 # nbdkit
-# Copyright (C) 2018 Red Hat Inc.
+# Copyright (C) 2018-2020 Red Hat Inc.
 #
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
@@ -34,12 +34,19 @@ source ./functions.sh
 set -e
 set -x
 
-rm -f test-vddk.out
-cleanup_fn rm -f test-vddk.out
+# Real VDDK only works on x86-64.  While this test doesn't test real
+# VDDK, we do need to know that we're on a 64 bit architecture
+# (because of the need for the lib64 path), so we might as well only
+# test x86-64.
+requires test $(uname -m) = x86_64
 
-LD_LIBRARY_PATH=.libs:$LD_LIBRARY_PATH \
-LIBRARY_PATH=.libs:$LIBRARY_PATH \
-nbdkit vddk --dump-plugin > test-vddk.out
+rm -rf vmware-vix-disklib-distrib test-vddk.out
+cleanup_fn rm -rf vmware-vix-disklib-distrib test-vddk.out
+
+mkdir -p vmware-vix-disklib-distrib/lib64
+cp .libs/libvixDiskLib.so* vmware-vix-disklib-distrib/lib64/
+
+nbdkit vddk libdir=vmware-vix-disklib-distrib --dump-plugin > test-vddk.out
 cat test-vddk.out
 
 grep ^vddk_default_libdir= test-vddk.out
-- 
2.25.0

On 2/13/20 8:01 AM, Richard W.M. Jones wrote:
> Do not use LD_LIBRARY_PATH to locate the VDDK library.  Setting this
> always causes problems because VDDK comes bundled with broken
> replacements for system libraries, such as libcrypto.so and
> libstdc++.so.  Two problems this causes which we have seen in the real
> world:
> 
> (1) User does ‘export LD_LIBRARY_PATH=vmware-vix-disklib-distrib’ and
> that breaks lots of ordinary utilities on their system.
> 
> (2) nbdkit vddk --run subcommand inherits the LD_LIBRARY_PATH
> environment variable from nbdkit, and common commands such as
> 'qemu-img' break, relying on complex workarounds like saving and
> restoring the original LD_LIBRARY_PATH in the subcommand.
> 
> Instead rely on a relatively undocumented feature of dlopen which is
> that when we pass in a full path it will try to load dependent
> libraries from the same directory.
> 
> Note this may break some callers who are not using libdir and
> expecting LD_LIBRARY_PATH to work, so it's a change in behaviour which
> we will have to highlight prominently in the 1.18 release notes.
> 
> Thanks: Dan Berrange, Ming Xie, Eric Blake.
Do you want to use Dan's preferred UTF-8 spelling?
> ---
>   plugins/vddk/nbdkit-vddk-plugin.pod | 29 ++++++++++++++++++++---------
>   configure.ac                        |  1 +
>   plugins/vddk/vddk.c                 | 15 +++++++++++++--
>   tests/test-vddk-real.sh             | 12 ++----------
>   tests/test-vddk.sh                  | 19 +++++++++++++------
>   5 files changed, 49 insertions(+), 27 deletions(-)
> 
> diff --git a/plugins/vddk/nbdkit-vddk-plugin.pod
b/plugins/vddk/nbdkit-vddk-plugin.pod
> index f34b9fba..5f32988b 100644
> --- a/plugins/vddk/nbdkit-vddk-plugin.pod
> +++ b/plugins/vddk/nbdkit-vddk-plugin.pod
> @@ -230,17 +230,22 @@ 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 conflict with system libraries.
Maybe "that can conflict", since we saw the issues on RHEL8 but not 
RHEL7 (so whether there is a conflict depends on what version VDDK was 
compiled against, rather than an unconditional event)
>   
> -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 can two choices: Place VDDK in the default libdir which is
s/can/have/
> +compiled into this plugin, for example:
> +
> + $ nbdkit vddk --dump-plugin | grep ^vddk_default_libdir
> + vddk_default_libdir=/usr/lib64/vmware-vix-disklib
> +
> +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
> @@ -250,6 +255,12 @@ configuration file, usually including
F</etc/vmware/config> and
>   F<$HOME/.vmware/config>, but you can override this using the
C<config>
>   parameter.
>   
> +=head2 No need to set C<LD_LIBRARY_PATH>
> +
> +In nbdkit E<le> 1.16 you had to set the environment variable
> +C<LD_LIBRARY_PATH> when using this plugin.  In nbdkit E<ge>
1.18 this
> +is I<not> recommended.
> +
>   =head1 FILE PARAMETER
>   
>   The C<file> parameter can either be a local file, in which case it
> diff --git a/configure.ac b/configure.ac
> index fa902945..d71f06e4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -130,6 +130,7 @@ AC_PROG_INSTALL
>   AC_PROG_CPP
>   AC_CANONICAL_HOST
>   AC_SYS_LARGEFILE
> +AC_CHECK_SIZEOF([long])
Is this strictly necessary, or...
>   
>   AC_C_PROTOTYPES
>   test "x$U" != "x" && AC_MSG_ERROR([Compiler
not ANSI compliant])
> diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
> index db61c1d8..c49eebcd 100644
> --- a/plugins/vddk/vddk.c
> +++ b/plugins/vddk/vddk.c
> @@ -256,7 +256,18 @@ load_library (void)
>   
>     /* Load the library. */
>     for (i = 0; i < sizeof sonames / sizeof sonames[0]; ++i) {
> -    dl = dlopen (sonames[i], RTLD_NOW);
> +    CLEANUP_FREE char *path;
> +
> +    /* Set the full path so that dlopen will preferentially load the
> +     * system libraries from the same directory.
> +     */
> +    if (asprintf (&path, "%s/lib%d/%s",
> +                  libdir ? : VDDK_LIBDIR, 8*SIZEOF_LONG, sonames[i]) ==
-1) {
could you just spell this sizeof(long)*CHAR_BITS?

I'm guessing that the vddk files always ship with a dir/lib32/xxx.so and 
a dir/lib64/xxx.so convention?

Or, can we just hard-code the name '/lib64/', since...

> +++ b/tests/test-vddk.sh
> @@ -1,6 +1,6 @@
>   #!/usr/bin/env bash
>   # nbdkit
> -# Copyright (C) 2018 Red Hat Inc.
> +# Copyright (C) 2018-2020 Red Hat Inc.
>   #
>   # Redistribution and use in source and binary forms, with or without
>   # modification, are permitted provided that the following conditions are
> @@ -34,12 +34,19 @@ source ./functions.sh
>   set -e
>   set -x
>   
> -rm -f test-vddk.out
> -cleanup_fn rm -f test-vddk.out
> +# Real VDDK only works on x86-64.  While this test doesn't test real
> +# VDDK, we do need to know that we're on a 64 bit architecture
> +# (because of the need for the lib64 path), so we might as well only
> +# test x86-64.
> +requires test $(uname -m) = x86_64
...that's all the more we support?
>   
> -LD_LIBRARY_PATH=.libs:$LD_LIBRARY_PATH \
> -LIBRARY_PATH=.libs:$LIBRARY_PATH \
> -nbdkit vddk --dump-plugin > test-vddk.out
> +rm -rf vmware-vix-disklib-distrib test-vddk.out
> +cleanup_fn rm -rf vmware-vix-disklib-distrib test-vddk.out
> +
> +mkdir -p vmware-vix-disklib-distrib/lib64
> +cp .libs/libvixDiskLib.so* vmware-vix-disklib-distrib/lib64/
> +
> +nbdkit vddk libdir=vmware-vix-disklib-distrib --dump-plugin >
test-vddk.out
>   cat test-vddk.out
>   
>   grep ^vddk_default_libdir= test-vddk.out
> 
But the idea looks sane to me. I don't have VDDK libraries installed so 
I can't readily test it, but assume this helps.

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