Libguestfs - May 2023 - [PATCH nbdkit v2 0/4] Add -D nbdkit.environ=1, Windows fixes and quoting issues

This grew a bit from last time ...

The basic change to allow dumping the environment is the same, except
I enabled this for Windows (and checked that it does in fact work --
Windows has environment variables, who knew?)

The second patch fixes a problem I noticed while testing the above on
Windows.

The third & fourth patches escape debug strings similarly to C
strings, so for example if a debug string contains '\n' then it will
be printed with ...\n... and not (as currently) break the debug
message across multiple lines.  Other non-printable characters are
handled in the obvious way too.

Rich.

This is not secure so should not be used routinely.  Also we do not
attempt to filter environment variables, so even ones containing
multiple lines or special characters are all sent to nbdkit_debug.

The reason for adding this is to allow for debugging the new
nbd_set_socket_activation_name(3) API added in libnbd 1.16.
---
 docs/nbdkit.pod |  8 ++++++++
 server/main.c   | 22 ++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod
index 83cf6a11c..634c97e3a 100644
--- a/docs/nbdkit.pod
+++ b/docs/nbdkit.pod
@@ -619,6 +619,14 @@ are numerous and not usually very interesting.
 S<I<-D nbdkit.backend.controlpath=0>> suppresses the non-datapath
 commands (config, open, close, can_write, etc.)
 
+=item B<-D nbdkit.environ=1>
+
+Print nbdkit's environment variables in the debug output at start up.
+This is insecure because environment variables may contain both
+sensitive and user-controlled information, so it should not be used
+routinely.  But it is useful for tracking down problems related to
+environment variables.
+
 =item B<-D nbdkit.tls.log=>N
 
 Enable TLS logging.  C<N> can be in the range 0 (no logging) to 99.
diff --git a/server/main.c b/server/main.c
index 1df5d69ac..528a2dfea 100644
--- a/server/main.c
+++ b/server/main.c
@@ -210,6 +210,22 @@ dump_config (void)
 #endif
 }
 
+/* -D nbdkit.environ=1 to dump the environment at start up. */
+NBDKIT_DLL_PUBLIC int nbdkit_debug_environ;
+
+#ifndef HAVE_ENVIRON_DECL
+extern char **environ;
+#endif
+
+static void
+dump_environment (void)
+{
+  size_t i;
+
+  for (i = 0; environ[i]; ++i)
+    nbdkit_debug ("%s", environ[i]);
+}
+
 int
 main (int argc, char *argv[])
 {
@@ -662,6 +678,12 @@ main (int argc, char *argv[])
   /* Check all debug flags were used, and free them. */
   free_debug_flags ();
 
+  /* Dump the environment if asked.  This is the earliest we can do it
+   * because it uses a debug flag.
+   */
+  if (nbdkit_debug_environ && verbose)
+    dump_environment ();
+
   if (help) {
     struct backend *b;
 
-- 
2.39.2

At least under Wine they are not recognized and you just see literal
"[0m" etc appearing in the output.

Updates: commit 5a011e02375912f2ad88de1ebc6d609e29d38f71
---
 server/debug.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/server/debug.c b/server/debug.c
index 1b5ddaafe..2231d40a0 100644
--- a/server/debug.c
+++ b/server/debug.c
@@ -70,7 +70,9 @@ static void
 debug_common (bool in_server, const char *fs, va_list args)
 {
   int err = errno;
+#ifndef WIN32
   int tty;
+#endif
   CLEANUP_FREE char *str = NULL;
   size_t len = 0;
   FILE *fp;
@@ -88,15 +90,19 @@ debug_common (bool in_server, const char *fs, va_list args)
     return;
   }
 
+#ifndef WIN32
   tty = isatty (fileno (stderr));
   if (!in_server && tty) ansi_force_colour (ANSI_FG_BOLD_BLACK, fp);
+#endif
 
   prologue (fp);
 
   errno = err;
   vfprintf (fp, fs, args);
 
+#ifndef WIN32
   if (!in_server && tty) ansi_force_restore (fp);
+#endif
   fprintf (fp, "\n");
   close_memstream (fp);
 
-- 
2.39.2

eg. { '\r', '\n' } -> { '\\', 'n',
'\\', 'r' }
---
 common/utils/utils.h |  1 +
 common/utils/quote.c | 58 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/common/utils/utils.h b/common/utils/utils.h
index 42288a5cd..1e6d8972b 100644
--- a/common/utils/utils.h
+++ b/common/utils/utils.h
@@ -35,6 +35,7 @@
 
 extern void shell_quote (const char *str, FILE *fp);
 extern void uri_quote (const char *str, FILE *fp);
+extern void c_string_quote (const char *str, FILE *fp);
 extern int exit_status_to_nbd_error (int status, const char *cmd);
 extern int set_cloexec (int fd);
 extern int set_nonblock (int fd);
diff --git a/common/utils/quote.c b/common/utils/quote.c
index 863e08a8e..877035387 100644
--- a/common/utils/quote.c
+++ b/common/utils/quote.c
@@ -37,6 +37,9 @@
 #include <string.h>
 #include <unistd.h>
 
+#include "ascii-ctype.h"
+#include "hexdigit.h"
+
 /* Print str to fp, shell quoting if necessary.  This comes from
  * libguestfs, but was written by me so I'm relicensing it to a BSD
  * license for nbdkit.
@@ -98,3 +101,58 @@ uri_quote (const char *str, FILE *fp)
       fprintf (fp, "%%%02X", str[i] & 0xff);
   }
 }
+
+/* Print str to fp, quoting in a way similar to C strings, for example
+ * '\n' -> "\n".
+ *
+ * Note that we do not emit quotes around the string, and double
+ * quotes within the string are not escaped.
+ */
+void
+c_string_quote (const char *str, FILE *fp)
+{
+  size_t i;
+  char c;
+
+  for (i = 0; c = str[i], c != '\0'; ++i) {
+    if (ascii_isprint (c))
+      fputc (c, fp);
+    else {
+      switch (c) {
+      case '\a':
+        fputc ('\\', fp);
+        fputc ('a', fp);
+        break;
+      case '\b':
+        fputc ('\\', fp);
+        fputc ('b', fp);
+        break;
+      case '\f':
+        fputc ('\\', fp);
+        fputc ('f', fp);
+        break;
+      case '\n':
+        fputc ('\\', fp);
+        fputc ('n', fp);
+        break;
+      case '\r':
+        fputc ('\\', fp);
+        fputc ('r', fp);
+        break;
+      case '\t':
+        fputc ('\\', fp);
+        fputc ('t', fp);
+        break;
+      case '\v':
+        fputc ('\\', fp);
+        fputc ('v', fp);
+        break;
+      default:
+        fputc ('\\', fp);
+        fputc ('x', fp);
+        fputc (hexchar (c >> 4), fp);
+        fputc (hexchar (c), fp);
+      }
+    }
+  }
+}
-- 
2.39.2

Debug strings contain all kinds of information including some under
user control.  Previously we simply sent everything to stderr, but
this is potentially insecure, as well as not dealing well with
non-printable characters.  Escape these strings when printing.
---
 server/debug.c | 52 ++++++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 21 deletions(-)

diff --git a/server/debug.c b/server/debug.c
index 2231d40a0..1fc56ec50 100644
--- a/server/debug.c
+++ b/server/debug.c
@@ -42,6 +42,7 @@
 
 #include "ansi-colours.h"
 #include "open_memstream.h"
+#include "utils.h"
 
 #include "internal.h"
 
@@ -69,19 +70,22 @@ prologue (FILE *fp)
 static void
 debug_common (bool in_server, const char *fs, va_list args)
 {
-  int err = errno;
-#ifndef WIN32
-  int tty;
-#endif
-  CLEANUP_FREE char *str = NULL;
-  size_t len = 0;
-  FILE *fp;
-
   if (!verbose)
     return;
 
-  fp = open_memstream (&str, &len);
-  if (fp == NULL) {
+  int err = errno;
+
+#ifndef WIN32
+  int tty;
+#endif
+  CLEANUP_FREE char *str_inner = NULL;
+  CLEANUP_FREE char *str_outer = NULL;
+  FILE *fp_inner, *fp_outer;
+  size_t len_inner = 0, len_outer = 0;
+
+  /* The "inner" string is the debug string before escaping. */
+  fp_inner = open_memstream (&str_inner, &len_inner);
+  if (fp_inner == NULL) {
   fail:
     /* Try to emit what we can. */
     errno = err;
@@ -89,28 +93,34 @@ debug_common (bool in_server, const char *fs, va_list args)
     fprintf (stderr, "\n");
     return;
   }
+  errno = err;
+  vfprintf (fp_inner, fs, args);
+  close_memstream (fp_inner);
+
+  /* The "outer" string contains the prologue, escaped debug string
and \n. */
+  fp_outer = open_memstream (&str_outer, &len_outer);
+  if (fp_outer == NULL) goto fail;
 
 #ifndef WIN32
   tty = isatty (fileno (stderr));
-  if (!in_server && tty) ansi_force_colour (ANSI_FG_BOLD_BLACK, fp);
+  if (!in_server && tty) ansi_force_colour (ANSI_FG_BOLD_BLACK,
fp_outer);
 #endif
 
-  prologue (fp);
-
-  errno = err;
-  vfprintf (fp, fs, args);
+  prologue (fp_outer);
+  c_string_quote (str_inner, fp_outer);
 
 #ifndef WIN32
-  if (!in_server && tty) ansi_force_restore (fp);
+  if (!in_server && tty) ansi_force_restore (fp_outer);
 #endif
-  fprintf (fp, "\n");
-  close_memstream (fp);
+  fprintf (fp_outer, "\n");
+  close_memstream (fp_outer);
 
-  if (str)
-    fputs (str, stderr);
-  else
+  if (!str_outer)
     goto fail;
 
+  /* Send it to stderr as atomically as possible. */
+  fputs (str_outer, stderr);
+
   errno = err;
 }
 
-- 
2.39.2