Eric Blake
2020-Feb-18 03:34 UTC
[Libguestfs] [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
Richard W.M. Jones
2020-Feb-18 10:48 UTC
Re: [Libguestfs] [nbdkit PATCH v6] vddk: Add re-exec with altered environment
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
Eric Blake
2020-Feb-18 12:30 UTC
Re: [Libguestfs] [nbdkit PATCH v6] vddk: Add re-exec with altered environment
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
Possibly Parallel Threads
- [nbdkit PATCH v7 0/2] vddk: Drive library loading from libdir parameter.
- [PATCH nbdkit 2/5] vddk: Move reexec code to a new file.
- [PATCH nbdkit v2 2/3] NOT WORKING: vddk: Drive library loading from libdir parameter.
- Re: [nbdkit PATCH v6] vddk: Add re-exec with altered environment
- [nbdkit PATCH v7 2/2] vddk: Drive library loading from libdir parameter.