Libguestfs - Jun 2020 - [PATCH nbdkit 3/5] vddk: Miscellaneous improvements to reexec code.

Probably needs a bit of cleanup, but seems like it is generally the
right direction.

One thing I've noticed is that the expect test randomly (but rarely)
hangs :-( I guess something is racey but I don't know what at the
moment.

Rich.

This reverts commit cc1cd671a03cd6d4528f2c859ece870f1486d85b.
---
 plugins/vddk/nbdkit-vddk-plugin.pod | 7 ++++++-
 plugins/vddk/vddk.c                 | 4 ----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/plugins/vddk/nbdkit-vddk-plugin.pod
b/plugins/vddk/nbdkit-vddk-plugin.pod
index 96a665b6..73316dd1 100644
--- a/plugins/vddk/nbdkit-vddk-plugin.pod
+++ b/plugins/vddk/nbdkit-vddk-plugin.pod
@@ -7,7 +7,8 @@ nbdkit-vddk-plugin - nbdkit VMware VDDK plugin
  nbdkit vddk [file=]FILENAME
              [config=FILENAME] [cookie=COOKIE] [libdir=LIBRARY]
              [nfchostport=PORT] [single-link=true]
-             [password=PASSWORD | password=+FILENAME | password=-FD]
+             [password=PASSWORD | password=- | password=+FILENAME
+             | password=-FD]
              [port=PORT] [server=HOSTNAME] [snapshot=MOREF]
              [thumbprint=THUMBPRINT] [transports=MODE:MODE:...]
              [unbuffered=true] [user=USERNAME] [vm=moref=ID]
@@ -158,6 +159,10 @@ Set the password to use when connecting to the remote
server.
 Note that passing this on the command line is not secure on shared
 machines.
 
+=item B<password=->
+
+Ask for the password (interactively) when nbdkit starts up.
+
 =item B<password=+>FILENAME
 
 Read the password from the named file.  This is a secure method
diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 54a6e019..4815a43e 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -240,10 +240,6 @@ vddk_config (const char *key, const char *value)
       return -1;
   }
   else if (strcmp (key, "password") == 0) {
-    if (strcmp (value, "-") == 0) {
-      nbdkit_error ("password=- is not supported with the VDDK
plugin");
-      return -1;
-    }
     free (password);
     if (nbdkit_read_password (value, &password) == -1)
       return -1;
-- 
2.25.0

Pure refactoring.  Just decouples the complicated reexec code from the
rest.
---
 plugins/vddk/Makefile.am |   2 +
 plugins/vddk/vddk.h      |  42 +++++++++
 plugins/vddk/reexec.c    | 196 +++++++++++++++++++++++++++++++++++++++
 plugins/vddk/vddk.c      | 151 ++----------------------------
 4 files changed, 246 insertions(+), 145 deletions(-)

diff --git a/plugins/vddk/Makefile.am b/plugins/vddk/Makefile.am
index 740c42d6..d4b123e9 100644
--- a/plugins/vddk/Makefile.am
+++ b/plugins/vddk/Makefile.am
@@ -42,6 +42,8 @@ plugin_LTLIBRARIES = nbdkit-vddk-plugin.la
 
 nbdkit_vddk_plugin_la_SOURCES = \
 	vddk.c \
+	vddk.h \
+	reexec.c \
 	vddk-structs.h \
 	vddk-stubs.h \
 	$(top_srcdir)/include/nbdkit-plugin.h \
diff --git a/plugins/vddk/vddk.h b/plugins/vddk/vddk.h
new file mode 100644
index 00000000..e229e55e
--- /dev/null
+++ b/plugins/vddk/vddk.h
@@ -0,0 +1,42 @@
+/* nbdkit
+ * 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
+ * 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.
+ */
+
+#ifndef NBDKIT_VDDK_H
+#define NBDKIT_VDDK_H
+
+extern char *libdir;
+extern char *reexeced;
+
+extern void reexec_if_needed (const char *prepend);
+extern int restore_ld_library_path (void);
+
+#endif /* NBDKIT_VDDK_H */
diff --git a/plugins/vddk/reexec.c b/plugins/vddk/reexec.c
new file mode 100644
index 00000000..5a5e9844
--- /dev/null
+++ b/plugins/vddk/reexec.c
@@ -0,0 +1,196 @@
+/* nbdkit
+ * 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
+ * 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 <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <fcntl.h>
+
+#define NBDKIT_API_VERSION 2
+#include <nbdkit-plugin.h>
+
+#include "cleanup.h"
+#include "vector.h"
+
+#include "vddk.h"
+
+char *reexeced;                 /* orig LD_LIBRARY_PATH on reexec */
+
+DEFINE_VECTOR_TYPE(string_vector, char *);
+
+#define CLEANUP_FREE_STRING_VECTOR \
+  __attribute__((cleanup (cleanup_free_string_vector)))
+
+static void
+cleanup_free_string_vector (string_vector *v)
+{
+  string_vector_iter (v, (void *) free);
+  free (v->ptr);
+}
+
+/* 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 proceed as if this had not been attempted.
+ * Thus, no return value is needed.
+ */
+static void
+perform_reexec (const char *env, const char *prepend)
+{
+  CLEANUP_FREE char *library = NULL;
+  CLEANUP_FREE_STRING_VECTOR string_vector argv = empty_vector;
+  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.  This assumes nbdkit did not alter its
+   * original argv[].
+   */
+  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) {
+    if (string_vector_append (&argv, buf + len) == -1) {
+    argv_realloc_fail:
+      nbdkit_debug ("argv: realloc: %m");
+      return;
+    }
+    len += strlen (buf + len) + 1;
+  }
+  if (!env)
+    env = "";
+  nbdkit_debug ("adding reexeced_=%s", env);
+  if (asprintf (&reexeced, "reexeced_=%s", env) == -1)
+    goto argv_realloc_fail;
+  if (string_vector_append (&argv, reexeced) == -1)
+    goto argv_realloc_fail;
+  if (string_vector_append (&argv, NULL) == -1)
+    goto argv_realloc_fail;
+
+  if (env[0]) {
+    if (asprintf (&library, "%s:%s", prepend, env) == -1)
+      assert (library == NULL);
+  }
+  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.ptr);
+  nbdkit_debug ("failure to execvp: %m");
+}
+
+/* See if prepend is already in LD_LIBRARY_PATH; if not, re-exec. */
+void
+reexec_if_needed (const char *prepend)
+{
+  const char *env = getenv ("LD_LIBRARY_PATH");
+  CLEANUP_FREE char *haystack = NULL;
+  CLEANUP_FREE char *needle = NULL;
+
+  if (reexeced)
+    return;
+  if (env && asprintf (&haystack, ":%s:", env) >= 0
&&
+      asprintf (&needle, ":%s:", prepend) >= 0 &&
+      strstr (haystack, needle) != NULL)
+    return;
+
+  perform_reexec (env, prepend);
+}
+
+/* If load_library caused a re-execution with an expanded
+ * LD_LIBRARY_PATH, restore it back to its original contents, passed
+ * as the value of "reexeced_".  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.
+ */
+int
+restore_ld_library_path (void)
+{
+  if (reexeced) {
+    char *env = getenv ("LD_LIBRARY_PATH");
+
+    nbdkit_debug ("cleaning up after re-exec");
+    if (!env || strstr (env, reexeced) == NULL ||
+        (libdir && strncmp (env, libdir, strlen (libdir)) != 0)) {
+      nbdkit_error ("'reexeced_' set with garbled
environment");
+      return -1;
+    }
+    if (reexeced[0]) {
+      if (setenv ("LD_LIBRARY_PATH", reexeced, 1) == -1) {
+        nbdkit_error ("setenv: %m");
+        return -1;
+      }
+    }
+    else if (unsetenv ("LD_LIBRARY_PATH") == -1) {
+      nbdkit_error ("unsetenv: %m");
+      return -1;
+    }
+  }
+
+  return 0;
+}
diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index 4815a43e..eb865cc2 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -40,7 +40,6 @@
 #include <string.h>
 #include <unistd.h>
 #include <dlfcn.h>
-#include <fcntl.h>
 #include <libgen.h>
 
 #include <pthread.h>
@@ -52,8 +51,8 @@
 #include "isaligned.h"
 #include "minmax.h"
 #include "rounding.h"
-#include "vector.h"
 
+#include "vddk.h"
 #include "vddk-structs.h"
 
 /* Debug flags. */
@@ -76,13 +75,12 @@ int vddk_debug_datapath = 1;
 
 static void *dl;                           /* dlopen handle */
 static bool init_called;                   /* was InitEx called */
-static char *reexeced;                     /* orig LD_LIBRARY_PATH on reexec */
 static pthread_key_t error_suppression;    /* threadlocal error suppression */
 
 static char *config;                       /* config */
 static const char *cookie;                 /* cookie */
 static const char *filename;               /* file */
-static char *libdir;                       /* libdir */
+char *libdir;                              /* libdir */
 static uint16_t nfc_host_port;             /* nfchostport */
 static char *password;                     /* password */
 static uint16_t port;                      /* port */
@@ -293,119 +291,6 @@ 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 proceed as if this had not been attempted.
- * Thus, no return value is needed.
- */
-DEFINE_VECTOR_TYPE(string_vector, char *);
-
-#define CLEANUP_FREE_STRING_VECTOR \
-  __attribute__((cleanup (cleanup_free_string_vector)))
-
-static void
-cleanup_free_string_vector (string_vector *v)
-{
-  string_vector_iter (v, (void *) free);
-  free (v->ptr);
-}
-
-static void
-perform_reexec (const char *env, const char *prepend)
-{
-  CLEANUP_FREE char *library = NULL;
-  CLEANUP_FREE_STRING_VECTOR string_vector argv = empty_vector;
-  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.  This assumes nbdkit did not alter its
-   * original argv[].
-   */
-  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) {
-    if (string_vector_append (&argv, buf + len) == -1) {
-    argv_realloc_fail:
-      nbdkit_debug ("argv: realloc: %m");
-      return;
-    }
-    len += strlen (buf + len) + 1;
-  }
-  if (!env)
-    env = "";
-  nbdkit_debug ("adding reexeced_=%s", env);
-  if (asprintf (&reexeced, "reexeced_=%s", env) == -1)
-    goto argv_realloc_fail;
-  if (string_vector_append (&argv, reexeced) == -1)
-    goto argv_realloc_fail;
-  if (string_vector_append (&argv, NULL) == -1)
-    goto argv_realloc_fail;
-
-  if (env[0]) {
-    if (asprintf (&library, "%s:%s", prepend, env) == -1)
-      assert (library == NULL);
-  }
-  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.ptr);
-  nbdkit_debug ("failure to execvp: %m");
-}
-
-/* See if prepend is already in LD_LIBRARY_PATH; if not, re-exec. */
-static void
-check_reexec (const char *prepend)
-{
-  const char *env = getenv ("LD_LIBRARY_PATH");
-  CLEANUP_FREE char *haystack = NULL;
-  CLEANUP_FREE char *needle = NULL;
-
-  if (reexeced)
-    return;
-  if (env && asprintf (&haystack, ":%s:", env) >= 0
&&
-      asprintf (&needle, ":%s:", prepend) >= 0 &&
-      strstr (haystack, needle) != NULL)
-    return;
-
-  perform_reexec (env, prepend);
-}
-
 /* Load the VDDK library. */
 static void
 load_library (bool load_error_is_fatal)
@@ -453,7 +338,7 @@ load_library (bool load_error_is_fatal)
        * includes its directory for all future loads.  This may modify
        * path in-place and/or re-exec nbdkit, but that's okay.
        */
-      check_reexec (dirname (path));
+      reexec_if_needed (dirname (path));
       break;
     }
     if (i == 0) {
@@ -498,33 +383,9 @@ 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, passed
-   * as the value of "reexeced_".  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");
-
-    nbdkit_debug ("cleaning up after re-exec");
-    if (!env || strstr (env, reexeced) == NULL ||
-        (libdir && strncmp (env, libdir, strlen (libdir)) != 0)) {
-      nbdkit_error ("'reexeced_' set with garbled
environment");
-      return -1;
-    }
-    if (reexeced[0]) {
-      if (setenv ("LD_LIBRARY_PATH", reexeced, 1) == -1) {
-        nbdkit_error ("setenv: %m");
-        return -1;
-      }
-    }
-    else if (unsetenv ("LD_LIBRARY_PATH") == -1) {
-      nbdkit_error ("unsetenv: %m");
-      return -1;
-    }
-  }
+  /* Restore original LD_LIBRARY_PATH after reexec. */
+  if (restore_ld_library_path () == -1)
+    return -1;
 
   /* For remote connections, check all the parameters have been
    * passed.  Note that VDDK will segfault if parameters that it
-- 
2.25.0

Use an extensible buffer (a vector<char>) when reading
/proc/self/cmdline.

Tidy up some error messages.
---
 plugins/vddk/reexec.c | 57 ++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 22 deletions(-)

diff --git a/plugins/vddk/reexec.c b/plugins/vddk/reexec.c
index 5a5e9844..9641ee8c 100644
--- a/plugins/vddk/reexec.c
+++ b/plugins/vddk/reexec.c
@@ -48,6 +48,19 @@
 
 char *reexeced;                 /* orig LD_LIBRARY_PATH on reexec */
 
+/* Extensible buffer (string). */
+DEFINE_VECTOR_TYPE(buffer, char);
+
+#define CLEANUP_FREE_BUFFER \
+  __attribute__((cleanup (cleanup_free_buffer)))
+
+static void
+cleanup_free_buffer (buffer *v)
+{
+  free (v->ptr);
+}
+
+/* List of strings. */
 DEFINE_VECTOR_TYPE(string_vector, char *);
 
 #define CLEANUP_FREE_STRING_VECTOR \
@@ -68,11 +81,13 @@ cleanup_free_string_vector (string_vector *v)
 static void
 perform_reexec (const char *env, const char *prepend)
 {
+  static const char cmdline_file[] = "/proc/self/cmdline";
+  static const char exe_file[] = "/proc/self/exe";
   CLEANUP_FREE char *library = NULL;
+  CLEANUP_FREE_BUFFER buffer buf = empty_vector;
   CLEANUP_FREE_STRING_VECTOR string_vector argv = empty_vector;
   int fd;
-  size_t len = 0, buflen = 512;
-  CLEANUP_FREE char *buf = NULL;
+  size_t len;
 
   /* 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
@@ -80,42 +95,40 @@ perform_reexec (const char *env, const char *prepend)
    * until we get a short read.  This assumes nbdkit did not alter its
    * original argv[].
    */
-  fd = open ("/proc/self/cmdline", O_RDONLY);
+  fd = open (cmdline_file, O_RDONLY|O_CLOEXEC);
   if (fd == -1) {
-    nbdkit_debug ("failure to parse original argv: %m");
+    nbdkit_debug ("open: %s: %m", cmdline_file);
     return;
   }
 
-  do {
-    char *p = realloc (buf, buflen * 2);
+  for (;;) {
     ssize_t r;
 
-    if (!p) {
-      nbdkit_debug ("failure to parse original argv: %m");
+    if (buffer_reserve (&buf, 512) == -1) {
+      nbdkit_debug ("realloc: %m");
       return;
     }
-    buf = p;
-    buflen *= 2;
-    r = read (fd, buf + len, buflen - len);
+    r = read (fd, buf.ptr + buf.size, buf.alloc - buf.size);
     if (r == -1) {
-      nbdkit_debug ("failure to parse original argv: %m");
+      nbdkit_debug ("read: %s: %m", cmdline_file);
       return;
     }
-    len += r;
-  } while (len == buflen);
-  nbdkit_debug ("original command line occupies %zu bytes", len);
+    if (r == 0)
+      break;
+    buf.size += r;
+  }
+  close (fd);
+  nbdkit_debug ("original command line occupies %zu bytes",
buf.size);
 
   /* Split cmdline into argv, then append one more arg. */
-  buflen = len;
-  len = 0;
-  while (len < buflen) {
-    if (string_vector_append (&argv, buf + len) == -1) {
+  for (len = 0; len < buf.size; len += strlen (buf.ptr + len) + 1) {
+    if (string_vector_append (&argv, buf.ptr + len) == -1) {
     argv_realloc_fail:
       nbdkit_debug ("argv: realloc: %m");
       return;
     }
-    len += strlen (buf + len) + 1;
   }
+
   if (!env)
     env = "";
   nbdkit_debug ("adding reexeced_=%s", env);
@@ -139,8 +152,8 @@ perform_reexec (const char *env, const char *prepend)
 
   nbdkit_debug ("re-executing with updated LD_LIBRARY_PATH=%s",
library);
   fflush (NULL);
-  execvp ("/proc/self/exe", argv.ptr);
-  nbdkit_debug ("failure to execvp: %m");
+  execvp (exe_file, argv.ptr);
+  nbdkit_debug ("execvp: %s: %m", exe_file);
 }
 
 /* See if prepend is already in LD_LIBRARY_PATH; if not, re-exec. */
-- 
2.25.0

Make it behave more or less like a real VDDK.  Of course it ignores
all parameters passed and emulates a blank disk, but that is
sufficient to do some more realistic testing.
---
 plugins/vddk/vddk-stubs.h |   8 ---
 tests/dummy-vddk.c        | 115 ++++++++++++++++++++++++++++++++++----
 2 files changed, 103 insertions(+), 20 deletions(-)

diff --git a/plugins/vddk/vddk-stubs.h b/plugins/vddk/vddk-stubs.h
index 62615af1..c5a68430 100644
--- a/plugins/vddk/vddk-stubs.h
+++ b/plugins/vddk/vddk-stubs.h
@@ -44,10 +44,6 @@
  * which is the earliest version of VDDK that we support.
  */
 
-/* We treat these two stubs slightly specially for the benefit of
- * tests/dummy-vddk.c.
- */
-#ifndef NO_INITEX_STUB
 STUB (VixDiskLib_InitEx,
       VixError,
       (uint32_t major, uint32_t minor,
@@ -56,13 +52,9 @@ STUB (VixDiskLib_InitEx,
        VixDiskLibGenericLogFunc *panic_function,
        const char *lib_dir,
        const char *config_file));
-#endif
-#ifndef NO_EXIT_STUB
 STUB (VixDiskLib_Exit,
       void,
       (void));
-#endif
-
 STUB (VixDiskLib_GetErrorText,
       char *,
       (VixError err, const char *unused));
diff --git a/tests/dummy-vddk.c b/tests/dummy-vddk.c
index ed5d3712..13be492d 100644
--- a/tests/dummy-vddk.c
+++ b/tests/dummy-vddk.c
@@ -30,18 +30,24 @@
  * SUCH DAMAGE.
  */
 
-/* This file pretends to be libvixDiskLib.so.6.
- *
- * However it only implements --dump-plugin support so our stubs don't
- * need to do anything.
- */
+/* This file pretends to be libvixDiskLib.so.6. */
 
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
+#include <string.h>
 
 #include "vddk-structs.h"
 
+#define STUB(fn,ret,args) extern ret fn args;
+#define OPTIONAL_STUB(fn,ret,args)
+#include "vddk-stubs.h"
+#undef STUB
+#undef OPTIONAL_STUB
+
+#define CAPACITY 1024 /* sectors */
+static char disk[CAPACITY * VIXDISKLIB_SECTOR_SIZE];
+
 VixError
 VixDiskLib_InitEx (uint32_t major, uint32_t minor,
                    VixDiskLibGenericLogFunc *log_function,
@@ -59,10 +65,95 @@ VixDiskLib_Exit (void)
   /* Do nothing. */
 }
 
-#define NO_INITEX_STUB
-#define NO_EXIT_STUB
-#define STUB(fn,ret,args) void fn (void) { abort (); }
-#define OPTIONAL_STUB(fn,ret,args)
-#include "vddk-stubs.h"
-#undef STUB
-#undef OPTIONAL_STUB
+char *
+VixDiskLib_GetErrorText (VixError err, const char *unused)
+{
+  return strdup ("dummy-vddk: error message");
+}
+
+void
+VixDiskLib_FreeErrorText (char *text)
+{
+  free (text);
+}
+
+void
+VixDiskLib_FreeConnectParams (VixDiskLibConnectParams *params)
+{
+  /* never called since we don't define optional AllocateConnectParams */
+  abort ();
+}
+
+VixError
+VixDiskLib_ConnectEx (const VixDiskLibConnectParams *params,
+                      char read_only,
+                      const char *snapshot_ref,
+                      const char *transport_modes,
+                      VixDiskLibConnection *connection)
+{
+  return VIX_OK;
+}
+
+VixError
+VixDiskLib_Open (const VixDiskLibConnection connection,
+                 const char *path,
+                 uint32_t flags,
+                 VixDiskLibHandle *handle)
+{
+  return VIX_OK;
+}
+
+const char *
+VixDiskLib_GetTransportMode (VixDiskLibHandle handle)
+{
+  return "file";
+}
+
+VixError
+VixDiskLib_Close (VixDiskLibHandle handle)
+{
+  return VIX_OK;
+}
+
+VixError
+VixDiskLib_Disconnect (VixDiskLibConnection connection)
+{
+  return VIX_OK;
+}
+
+VixError
+VixDiskLib_GetInfo (VixDiskLibHandle handle,
+                    VixDiskLibInfo **info)
+{
+  *info = calloc (1, sizeof (struct VixDiskLibInfo));
+  (*info)->capacity = CAPACITY;
+  return VIX_OK;
+}
+
+void
+VixDiskLib_FreeInfo (VixDiskLibInfo *info)
+{
+  free (info);
+}
+
+VixError
+VixDiskLib_Read (VixDiskLibHandle handle,
+                 uint64_t start_sector, uint64_t nr_sectors,
+                 unsigned char *buf)
+{
+  size_t offset = start_sector * VIXDISKLIB_SECTOR_SIZE;
+
+  memcpy (buf, disk + offset, nr_sectors * VIXDISKLIB_SECTOR_SIZE);
+  return VIX_OK;
+}
+
+VixError
+VixDiskLib_Write (VixDiskLibHandle handle,
+                  uint64_t start_sector, uint64_t nr_sectors,
+                  const unsigned char *buf)
+{
+  size_t offset = start_sector * VIXDISKLIB_SECTOR_SIZE;
+
+  memcpy (disk + offset, buf, nr_sectors * VIXDISKLIB_SECTOR_SIZE);
+  return VIX_OK;
+}
-- 
2.25.0

See this thread:
https://www.redhat.com/archives/libguestfs/2020-June/thread.html#00012

This commit also adds a regression test of vddk password=- and
password=-FD.
---
 tests/Makefile.am                       |  4 ++
 plugins/vddk/vddk.h                     |  1 +
 plugins/vddk/reexec.c                   | 43 ++++++++++++-
 plugins/vddk/vddk.c                     |  2 +-
 tests/test-vddk-password-fd.sh          | 86 +++++++++++++++++++++++++
 tests/test-vddk-password-interactive.sh | 77 ++++++++++++++++++++++
 tests/dummy-vddk.c                      |  6 ++
 7 files changed, 217 insertions(+), 2 deletions(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 2b16122b..7cecf873 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -860,6 +860,8 @@ noinst_LTLIBRARIES += libvixDiskLib.la
 TESTS += \
 	test-vddk.sh \
 	test-vddk-dump-plugin.sh \
+	test-vddk-password-fd.sh \
+	test-vddk-password-interactive.sh \
 	test-vddk-real.sh \
 	test-vddk-real-dump-plugin.sh \
 	$(NULL)
@@ -881,6 +883,8 @@ endif HAVE_VDDK
 EXTRA_DIST += \
 	test-vddk.sh \
 	test-vddk-dump-plugin.sh \
+	test-vddk-password-fd.sh \
+	test-vddk-password-interactive.sh \
 	test-vddk-real.sh \
 	test-vddk-real-dump-plugin.sh \
 	$(NULL)
diff --git a/plugins/vddk/vddk.h b/plugins/vddk/vddk.h
index e229e55e..0f2d0050 100644
--- a/plugins/vddk/vddk.h
+++ b/plugins/vddk/vddk.h
@@ -34,6 +34,7 @@
 #define NBDKIT_VDDK_H
 
 extern char *libdir;
+extern char *password;
 extern char *reexeced;
 
 extern void reexec_if_needed (const char *prepend);
diff --git a/plugins/vddk/reexec.c b/plugins/vddk/reexec.c
index 9641ee8c..43d4e1b7 100644
--- a/plugins/vddk/reexec.c
+++ b/plugins/vddk/reexec.c
@@ -37,6 +37,7 @@
 #include <string.h>
 #include <unistd.h>
 #include <fcntl.h>
+#include <sys/types.h>
 
 #define NBDKIT_API_VERSION 2
 #include <nbdkit-plugin.h>
@@ -122,7 +123,47 @@ perform_reexec (const char *env, const char *prepend)
 
   /* Split cmdline into argv, then append one more arg. */
   for (len = 0; len < buf.size; len += strlen (buf.ptr + len) + 1) {
-    if (string_vector_append (&argv, buf.ptr + len) == -1) {
+    char *arg = buf.ptr + len;  /* Next \0-terminated argument. */
+
+    /* password parameter requires special handling for reexec.  For
+     * password=- and password=-FD, after reexec we might try to
+     * reread these, but stdin has gone away and FD has been consumed
+     * already so that won't work.  Even password=+FILE is a little
+     * problematic since the file will be read twice, which may break
+     * for special files.
+     *
+     * However we may write the password to a temporary file and
+     * substitute password=-<FD> of the opened temporary file here.
+     * The trick is described by Eric Blake here:
+     * https://www.redhat.com/archives/libguestfs/2020-June/msg00021.html
+     *
+     * (RHBZ#1842440)
+     */
+    if (strncmp (arg, "password=", 9) == 0) {
+      char tmpfile[] = "/tmp/XXXXXX";
+      char *password_fd;
+
+      /* These operations should never fail, so exit on error. */
+      fd = mkstemp (tmpfile);
+      if (fd == -1) {
+        nbdkit_error ("mkstemp: %m");
+        exit (EXIT_FAILURE);
+      }
+      unlink (tmpfile);
+      if (write (fd, password, strlen (password)) != strlen (password)) {
+        nbdkit_error ("write: %m");
+        exit (EXIT_FAILURE);
+      }
+      lseek (fd, 0, SEEK_SET);
+      if (asprintf (&password_fd, "password=-%d", fd) == -1) {
+        nbdkit_error ("asprintf: %m");
+        exit (EXIT_FAILURE);
+      }
+      /* Leaked here but cleaned up when we exec below. */
+      arg = password_fd;
+    }
+
+    if (string_vector_append (&argv, arg) == -1) {
     argv_realloc_fail:
       nbdkit_debug ("argv: realloc: %m");
       return;
diff --git a/plugins/vddk/vddk.c b/plugins/vddk/vddk.c
index eb865cc2..f2696fa6 100644
--- a/plugins/vddk/vddk.c
+++ b/plugins/vddk/vddk.c
@@ -82,7 +82,7 @@ static const char *cookie;                 /* cookie */
 static const char *filename;               /* file */
 char *libdir;                              /* libdir */
 static uint16_t nfc_host_port;             /* nfchostport */
-static char *password;                     /* password */
+char *password;                            /* password */
 static uint16_t port;                      /* port */
 static const char *server_name;            /* server */
 static bool single_link;                   /* single-link */
diff --git a/tests/test-vddk-password-fd.sh b/tests/test-vddk-password-fd.sh
new file mode 100755
index 00000000..6386fa11
--- /dev/null
+++ b/tests/test-vddk-password-fd.sh
@@ -0,0 +1,86 @@
+#!/usr/bin/env bash
+# nbdkit
+# 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
+# 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.
+
+# password=-FD was broken in the VDDK plugin in nbdkit 1.18 through
+# 1.20.2.  This was because the reexec code caused
+# nbdkit_read_password to be called twice, second time with stdin
+# reopened on /dev/null.  Since we have now fixed this bug, this is a
+# regression test.
+
+source ./functions.sh
+set -e
+set -x
+
+# Setting LD_LIBRARY_PATH breaks valgrind.
+if [ "x$NBDKIT_VALGRIND" = "x1" ]; then
+    echo "$0: skipped under valgrind"
+    exit 77
+fi
+
+requires qemu-img --version
+
+f=test-vddk-password-fd.file
+out=test-vddk-password-fd.out
+cleanup_fn rm -f $f $out
+
+# Get dummy-vddk to print the password to stderr.
+export DUMMY_VDDK_PRINT_PASSWORD=1
+
+# Password -FD.
+echo 123 > $f
+exec 3< $f
+nbdkit -fv -U - vddk \
+       libdir=.libs \
+       server=noserver.example.com thumbprint=ab \
+       vm=novm /nofile \
+       user=root password=-3 \
+       --run 'qemu-img info $nbd' \
+       >&$out ||:
+exec 3<&-
+cat $out
+
+grep "password=123$" $out
+
+# Password -FD, zero length.
+: > $f
+exec 3< $f
+nbdkit -fv -U - vddk \
+       libdir=.libs \
+       server=noserver.example.com thumbprint=ab \
+       vm=novm /nofile \
+       user=root password=-3 \
+       --run 'qemu-img info $nbd' \
+       >&$out ||:
+exec 3<&-
+cat $out
+
+grep "password=$" $out
diff --git a/tests/test-vddk-password-interactive.sh
b/tests/test-vddk-password-interactive.sh
new file mode 100755
index 00000000..b7e0af26
--- /dev/null
+++ b/tests/test-vddk-password-interactive.sh
@@ -0,0 +1,77 @@
+#!/usr/bin/env bash
+# nbdkit
+# 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
+# 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.
+
+# password=- was broken in the VDDK plugin in nbdkit 1.18 through
+# 1.20.2.  This was because the reexec code caused
+# nbdkit_read_password to be called twice, second time with stdin
+# reopened on /dev/null.  Since we have now fixed this bug, this is a
+# regression test.
+
+source ./functions.sh
+set -e
+set -x
+
+# The tests break under valgrind for a couple of reasons:
+# (1) Setting LD_LIBRARY_PATH breaks valgrind.
+# (2) Valgrinded nbdkit hangs under expect.
+if [ "x$NBDKIT_VALGRIND" = "x1" ]; then
+    echo "$0: skipped under valgrind"
+    exit 77
+fi
+
+requires qemu-img --version
+requires expect -v
+
+out=test-vddk-password-interactive.out
+cleanup_fn rm -f $out
+
+# Get dummy-vddk to print the password to stderr.
+export DUMMY_VDDK_PRINT_PASSWORD=1
+
+export out
+expect -f - <<'EOF'
+  spawn sh -c "
+    nbdkit -fv -U - \
+           vddk \
+           libdir=.libs \
+           server=noserver.example.com thumbprint=ab \
+           vm=novm /nofile \
+           user=root password=- \
+           --run 'qemu-img info \$nbd' \
+           2>$env(out)"
+  expect "ssword:"
+  send "abc\r"
+  wait
+EOF
+cat $out
+
+grep "password=abc$" $out
diff --git a/tests/dummy-vddk.c b/tests/dummy-vddk.c
index 13be492d..a2377944 100644
--- a/tests/dummy-vddk.c
+++ b/tests/dummy-vddk.c
@@ -91,6 +91,12 @@ VixDiskLib_ConnectEx (const VixDiskLibConnectParams *params,
                       const char *transport_modes,
                       VixDiskLibConnection *connection)
 {
+  /* Used when regression testing password= parameter. */
+  if (getenv ("DUMMY_VDDK_PRINT_PASSWORD") &&
+      params->credType == VIXDISKLIB_CRED_UID &&
+      params->creds.uid.password != NULL)
+    fprintf (stderr, "dummy-vddk: password=%s\n",
params->creds.uid.password);
+
   return VIX_OK;
 }
 
-- 
2.25.0

On 6/2/20 7:27 AM, Richard W.M. Jones wrote:
> Pure refactoring.  Just decouples the complicated reexec code from the
> rest.
> ---
>   plugins/vddk/Makefile.am |   2 +
>   plugins/vddk/vddk.h      |  42 +++++++++
>   plugins/vddk/reexec.c    | 196 +++++++++++++++++++++++++++++++++++++++
>   plugins/vddk/vddk.c      | 151 ++----------------------------
>   4 files changed, 246 insertions(+), 145 deletions(-)
> 
> +
> +/* If load_library caused a re-execution with an expanded
> + * LD_LIBRARY_PATH, restore it back to its original contents, passed
> + * as the value of "reexeced_".  dlopen uses the value of
Comment may need a tweak, since the .config parsing of reexeced_ is now 
in a different file, and this file is going solely off the global reexeced.
> + * 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.
> + */
> +int
> +restore_ld_library_path (void)
> +{
> +  if (reexeced) {
> +    char *env = getenv ("LD_LIBRARY_PATH");
> +
> +    nbdkit_debug ("cleaning up after re-exec");
> +    if (!env || strstr (env, reexeced) == NULL ||
> +        (libdir && strncmp (env, libdir, strlen (libdir)) != 0)) {
> +      nbdkit_error ("'reexeced_' set with garbled
environment");
Then again, we are outputting an error message based on what .config saw.

At any rate, the split makes sense.

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

On 6/2/20 7:27 AM, Richard W.M. Jones wrote:
> Use an extensible buffer (a vector<char>) when reading
> /proc/self/cmdline.
> 
> Tidy up some error messages.
> ---
>   plugins/vddk/reexec.c | 57 ++++++++++++++++++++++++++-----------------
>   1 file changed, 35 insertions(+), 22 deletions(-)
> 
> @@ -80,42 +95,40 @@ perform_reexec (const char *env, const char *prepend)
>      * until we get a short read.  This assumes nbdkit did not alter its
>      * original argv[].
>      */
> -  fd = open ("/proc/self/cmdline", O_RDONLY);
> +  fd = open (cmdline_file, O_RDONLY|O_CLOEXEC);
>     if (fd == -1) {
> -    nbdkit_debug ("failure to parse original argv: %m");
> +    nbdkit_debug ("open: %s: %m", cmdline_file);
>       return;
>     }
>   
> -  do {
> -    char *p = realloc (buf, buflen * 2);
> +  for (;;) {
>       ssize_t r;
>   
> -    if (!p) {
> -      nbdkit_debug ("failure to parse original argv: %m");
> +    if (buffer_reserve (&buf, 512) == -1) {
> +      nbdkit_debug ("realloc: %m");
>         return;
>       }
Pre-existing bug, which you did not fix here.  If we failed here, we are 
leaking fd.  You slightly improved the situation by marking the leaked 
fd O_CLOEXEC, but that really doesn't matter if we properly fix the code 
to close(fd) before any early return, at which point the lifetime of fd 
is only during single-threaded execution and O_CLOEXEC doesn't matter.

Rest of the patch looks fine.

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

On 6/2/20 7:27 AM, Richard W.M. Jones wrote:
> Make it behave more or less like a real VDDK.  Of course it ignores
> all parameters passed and emulates a blank disk, but that is
> sufficient to do some more realistic testing.
> ---
>   plugins/vddk/vddk-stubs.h |   8 ---
>   tests/dummy-vddk.c        | 115 ++++++++++++++++++++++++++++++++++----
>   2 files changed, 103 insertions(+), 20 deletions(-)
I'm less familiar with the vddk API itself, but this looks reasonable 
for improved test coverage.

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

On 6/2/20 7:27 AM, Richard W.M. Jones wrote:
> See this thread:
> https://www.redhat.com/archives/libguestfs/2020-June/thread.html#00012
> 
> This commit also adds a regression test of vddk password=- and
> password=-FD.
> ---
>   tests/Makefile.am                       |  4 ++
>   plugins/vddk/vddk.h                     |  1 +
>   plugins/vddk/reexec.c                   | 43 ++++++++++++-
>   plugins/vddk/vddk.c                     |  2 +-
>   tests/test-vddk-password-fd.sh          | 86 +++++++++++++++++++++++++
>   tests/test-vddk-password-interactive.sh | 77 ++++++++++++++++++++++
>   tests/dummy-vddk.c                      |  6 ++
>   7 files changed, 217 insertions(+), 2 deletions(-)
> 
> @@ -122,7 +123,47 @@ perform_reexec (const char *env, const char *prepend)
>   
>     /* Split cmdline into argv, then append one more arg. */
>     for (len = 0; len < buf.size; len += strlen (buf.ptr + len) + 1) {
> -    if (string_vector_append (&argv, buf.ptr + len) == -1) {
> +    char *arg = buf.ptr + len;  /* Next \0-terminated argument. */
> +
> +    /* password parameter requires special handling for reexec.  For
> +     * password=- and password=-FD, after reexec we might try to
> +     * reread these, but stdin has gone away and FD has been consumed
> +     * already so that won't work.  Even password=+FILE is a little
> +     * problematic since the file will be read twice, which may break
> +     * for special files.
> +     *
> +     * However we may write the password to a temporary file and
> +     * substitute password=-<FD> of the opened temporary file here.
> +     * The trick is described by Eric Blake here:
> +     * https://www.redhat.com/archives/libguestfs/2020-June/msg00021.html
> +     *
> +     * (RHBZ#1842440)
> +     */
Absolutely essential comment :)  I think it accurately captures what the 
reader needs to know.
> +    if (strncmp (arg, "password=", 9) == 0) {
> +      char tmpfile[] = "/tmp/XXXXXX";
> +      char *password_fd;
> +
> +      /* These operations should never fail, so exit on error. */
> +      fd = mkstemp (tmpfile);
> +      if (fd == -1) {
> +        nbdkit_error ("mkstemp: %m");
> +        exit (EXIT_FAILURE);
> +      }
> +      unlink (tmpfile);
> +      if (write (fd, password, strlen (password)) != strlen (password)) {
Is it worth assert(password) prior to this point?  (We are guaranteed 
that if we found a "password=..." parameter, the caller already parsed
it into the password variable).

Slight inefficiency: right now, .config allows 'password=' more than 
once, and the last one wins.  Our re-exec opens more than one fd in that 
case, and we still end up with last one wins, but it might be nicer if 
we only passed through the last password= rather than all of them. 
Perhaps you could do that by a tweak to the logic - the loop through the 
argv blindly skips all password= arguments, then after the loop, if 
password is non-NULL, you open the temp file and append a password=FD 
argument just once.  Doing this re-arrangement would also help address 
the fd leak...
> +        nbdkit_error ("write: %m");
> +        exit (EXIT_FAILURE);
> +      }
> +      lseek (fd, 0, SEEK_SET);
> +      if (asprintf (&password_fd, "password=-%d", fd) == -1)
{
> +        nbdkit_error ("asprintf: %m");
> +        exit (EXIT_FAILURE);
> +      }
> +      /* Leaked here but cleaned up when we exec below. */
> +      arg = password_fd;
> +    }
> +
> +    if (string_vector_append (&argv, arg) == -1) {
>       argv_realloc_fail:
>         nbdkit_debug ("argv: realloc: %m");
>         return;
... if execvp fails, we now leak password fd; we should close it before 
returning.
> +++ b/tests/test-vddk-password-fd.sh
> +# Get dummy-vddk to print the password to stderr.
> +export DUMMY_VDDK_PRINT_PASSWORD=1
> +
> +# Password -FD.
> +echo 123 > $f
> +exec 3< $f
> +nbdkit -fv -U - vddk \
> +       libdir=.libs \
> +       server=noserver.example.com thumbprint=ab \
> +       vm=novm /nofile \
> +       user=root password=-3 \
> +       --run 'qemu-img info $nbd' \
> +       >&$out ||:
> +exec 3<&-
> +cat $out
Slick.
> +++ b/tests/test-vddk-password-interactive.sh
> +# password=- was broken in the VDDK plugin in nbdkit 1.18 through
> +# 1.20.2.  This was because the reexec code caused
> +# nbdkit_read_password to be called twice, second time with stdin
> +# reopened on /dev/null.  Since we have now fixed this bug, this is a
> +# regression test.
> +
> +requires qemu-img --version
> +requires expect -v
As far as I can tell, this is our first use of expect.  README should be 
updated to mention the optional dependency.
> +
> +out=test-vddk-password-interactive.out
> +cleanup_fn rm -f $out
> +
> +# Get dummy-vddk to print the password to stderr.
> +export DUMMY_VDDK_PRINT_PASSWORD=1
> +
> +export out
> +expect -f - <<'EOF'
> +  spawn sh -c "
> +    nbdkit -fv -U - \
> +           vddk \
> +           libdir=.libs \
> +           server=noserver.example.com thumbprint=ab \
> +           vm=novm /nofile \
> +           user=root password=- \
> +           --run 'qemu-img info \$nbd' \
> +           2>$env(out)"
> +  expect "ssword:"
> +  send "abc\r"
> +  wait
> +EOF
> +cat $out
I'm not seeing an obvious race, I'll have to see if I can encounter it 
in person.  In the meantime, from what I know about using expect (which 
isn't much), this looks like a reasonable way to get nbdkit to see a tty 
and perform an interactive password request.

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