Libguestfs - Feb 2020 - [nbdkit PATCH v6] vddk: Add re-exec with altered environment

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.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

Compared to v5: patch 1 and 2 are completely dropped, patch 3/4 is the
first one applied as-is, then this patch, then an updated version of
4/4 (which I haven't finished updating yet).  Posting this now before
I go to bed.

Still needed: either the testsuite HAS to create its dummy
libvixDiskLib.so.6 under a /lib64 subdirectory, or we should add some
intelligence to probing whether $libdir/*.so or $libdir/lib64/*.so
exists and set prefix accordingly.  But that's minor compared to that
fact that I did confirm that re-execing works to temporarily override
LD_LIBRARY_PATH without any modification to nbdkit proper (other than
a one-liner I already pushed to let /proc/NNN/cmdline be correct for
re-exec purposes).

The diffstat is a bit large, but the fact that it is self-contained to
vddk.c is a plus.  Parsing /proc/self/cmdline is a pain.

 plugins/vddk/vddk.c | 146 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 139 insertions(+), 7 deletions(-)

diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 0abec68e..95940b3b 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

@@ -72,7 +73,8 @@ int vddk_debug_extents;
 #define VDDK_MINOR 1

 static void *dl = NULL;                    /* dlopen handle */
-static int init_called = 0;                /* was InitEx called */
+static bool init_called = false;           /* was InitEx called */
+static char *reexeced = false;             /* did libdir require reexec */

 static char *config = NULL;                /* config */
 static const char *cookie = NULL;          /* cookie */
@@ -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 = 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,89 @@ 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 fail 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;
+  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.
+   */
+  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;
+  }
+  nbdkit_debug ("original argc == %d, adding reexeced_=%s", argc,
prepend);
+  if (asprintf (&reexeced, "reexeced_=%s", prepend) == -1) {
+    nbdkit_debug ("failure to re-exec: %m");
+    return;
+  }
+  argv[argc++] = reexeced;
+  argv[argc] = NULL;
+
+  if (env)
+    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 +355,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 +392,47 @@ 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.
+   * 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");
+    CLEANUP_FREE char *library = NULL;
+    size_t len = strlen (reexeced);
+
+    nbdkit_debug ("cleaning up after re-exec");
+    if (!env || !libdir || strncmp (env, reexeced, len) != 0 ||
+        strncmp (reexeced, libdir, strlen (libdir)) != 0) {
+      nbdkit_error ("'reexeced' set with garbled
environment");
+      return -1;
+    }
+    if (env[len] == ':') {
+      library = strdup (&env[len+1]);
+      if (!library) {
+        nbdkit_error ("strdup: %m");
+        return -1;
+      }
+      if (setenv ("LD_LIBRARY_PATH", library, 1) == -1) {
+        nbdkit_error ("setenv: %m");
+        return -1;
+      }
+    }
+    else if (env[len] != '\0') {
+      nbdkit_error ("'reexeced' set with garbled
environment");
+      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 +479,7 @@ vddk_config_complete (void)
     VDDK_ERROR (err, "VixDiskLib_InitEx");
     exit (EXIT_FAILURE);
   }
-  init_called = 1;
+  init_called = true;

   return 0;
 }
-- 
2.24.1

On Mon, Feb 17, 2020 at 09:34:12PM -0600, Eric Blake
wrote:
> 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.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> Compared to v5: patch 1 and 2 are completely dropped, patch 3/4 is the
> first one applied as-is, then this patch, then an updated version of
> 4/4 (which I haven't finished updating yet).  Posting this now before
> I go to bed.
> 
> Still needed: either the testsuite HAS to create its dummy
> libvixDiskLib.so.6 under a /lib64 subdirectory, or we should add some
> intelligence to probing whether $libdir/*.so or $libdir/lib64/*.so
> exists and set prefix accordingly.  But that's minor compared to that
> fact that I did confirm that re-execing works to temporarily override
> LD_LIBRARY_PATH without any modification to nbdkit proper (other than
> a one-liner I already pushed to let /proc/NNN/cmdline be correct for
> re-exec purposes).
> 
> The diffstat is a bit large, but the fact that it is self-contained to
> vddk.c is a plus.  Parsing /proc/self/cmdline is a pain.
> 
>  plugins/vddk/vddk.c | 146 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 139 insertions(+), 7 deletions(-)
> 
> diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
> index 0abec68e..95940b3b 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
> 
> @@ -72,7 +73,8 @@ int vddk_debug_extents;
>  #define VDDK_MINOR 1
> 
>  static void *dl = NULL;                    /* dlopen handle */
> -static int init_called = 0;                /* was InitEx called */
> +static bool init_called = false;           /* was InitEx called */
> +static char *reexeced = false;             /* did libdir require reexec */
I guess this works, but is "NULL" better than "false"?
> +  /* If load_library caused a re-execution with an expanded
> +   * LD_LIBRARY_PATH, restore it back to its original contents.
> +   * 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.
> +   */
Could we pass the original LD_LIBRARY_PATH on the command line too?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On 2/18/20 4:48 AM, Richard W.M. Jones wrote:
>> 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.
>>
>> @@ -72,7 +73,8 @@ int vddk_debug_extents;
>>   #define VDDK_MINOR 1
>>
>>   static void *dl = NULL;                    /* dlopen handle */
>> -static int init_called = 0;                /* was InitEx called */
>> +static bool init_called = false;           /* was InitEx called */
>> +static char *reexeced = false;             /* did libdir require
reexec */
> 
> I guess this works, but is "NULL" better than "false"?
Rebasing snafu - I originally started with bool reexeced, then realized 
that I need to be smarter about what prefix to strip off of 
LD_LIBRARY_PATH (as pointed out above, we generally want
"$libdir/lib64"
rather than bare "$libdir" as the LD_LIBRARY_PATH entry, since 
VixDiskLib_InitEx() asks for libdir).  Next I added a second config 
parameter (one bool, one the string to strip), got it working, then 
realized I could merge it back into one variable (the string to strip 
behaves as the bool), but forgot to change the initializer to match.

As it is, C guarantees that all static variables are 0-initialized, so = 
NULL, = 0, and = false are all redundant in this section of the file.
> 
>> +  /* If load_library caused a re-execution with an expanded
>> +   * LD_LIBRARY_PATH, restore it back to its original contents.
>> +   * 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.
>> +   */
> 
> Could we pass the original LD_LIBRARY_PATH on the command line too?
Yes, that's an option. Linux in general allows long command lines, so 
we're unlikely to run into command-line length limits by doing so, and 
having the full original LD_LIBRARY_PATH makes it easier to replace the 
extended one.

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