Libguestfs - May 2023 - [PATCH nbdkit v3 5/6] server: debug: Fix error handling

Add R-b tags.

Split the last patch into 3 parts, fixing the error handling issues
raised by Laszlo in the previous review.

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.

Reviewed-by: Laszlo Ersek <lersek at redhat.com>
---
 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
Reviewed-by: Laszlo Ersek <lersek at redhat.com>
---
 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

In particular return before we have allocated 'str' with
attribute((cleanup)) so that we don't waste time calling free (NULL)
on the hot-ish path out of the function.
---
 server/debug.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/server/debug.c b/server/debug.c
index 2231d40a0..8afa0ae8b 100644
--- a/server/debug.c
+++ b/server/debug.c
@@ -69,6 +69,9 @@ prologue (FILE *fp)
 static void
 debug_common (bool in_server, const char *fs, va_list args)
 {
+  if (!verbose)
+    return;
+
   int err = errno;
 #ifndef WIN32
   int tty;
@@ -77,9 +80,6 @@ debug_common (bool in_server, const char *fs, va_list args)
   size_t len = 0;
   FILE *fp;
 
-  if (!verbose)
-    return;
-
   fp = open_memstream (&str, &len);
   if (fp == NULL) {
   fail:
-- 
2.39.2

Preserve errno even if the function fails.  We need to set
errno = err again on every exit path out of the function.

Check if close_memstream failed and go to the error path if so.  This
is most important on Windows where close_memstream is what actually
allocates the memory.  But even on Unix, close_memstream (which is an
alias for fclose) can return an error.

Move the error-handling code to the end of the function.

I added a comment above the 'fputs' explaining why exactly we're
using
memstream in the first place -- because it is more atomic than using
multiple fprintf.

See: https://listman.redhat.com/archives/libguestfs/2023-May/031456.html
Thanks: Laszlo Ersek
---
 server/debug.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/server/debug.c b/server/debug.c
index 8afa0ae8b..177d9d5da 100644
--- a/server/debug.c
+++ b/server/debug.c
@@ -81,14 +81,8 @@ debug_common (bool in_server, const char *fs, va_list args)
   FILE *fp;
 
   fp = open_memstream (&str, &len);
-  if (fp == NULL) {
-  fail:
-    /* Try to emit what we can. */
-    errno = err;
-    vfprintf (stderr, fs, args);
-    fprintf (stderr, "\n");
-    return;
-  }
+  if (fp == NULL)
+    goto fail;
 
 #ifndef WIN32
   tty = isatty (fileno (stderr));
@@ -104,13 +98,23 @@ debug_common (bool in_server, const char *fs, va_list args)
   if (!in_server && tty) ansi_force_restore (fp);
 #endif
   fprintf (fp, "\n");
-  close_memstream (fp);
+  if (close_memstream (fp) == -1)
+    goto fail;
 
-  if (str)
-    fputs (str, stderr);
-  else
+  if (!str)
     goto fail;
 
+  /* Send it to stderr as atomically as possible. */
+  fputs (str, stderr);
+
+  errno = err;
+  return;
+
+ fail:
+  /* Try to emit what we can. */
+  errno = err;
+  vfprintf (stderr, fs, args);
+  fprintf (stderr, "\n");
   errno = err;
 }
 
-- 
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 | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)

diff --git a/server/debug.c b/server/debug.c
index 177d9d5da..6cf1af316 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"
 
@@ -76,36 +77,43 @@ debug_common (bool in_server, const char *fs, va_list args)
 #ifndef WIN32
   int tty;
 #endif
-  CLEANUP_FREE char *str = NULL;
-  size_t len = 0;
-  FILE *fp;
+  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;
 
-  fp = open_memstream (&str, &len);
-  if (fp == NULL)
+  /* The "inner" string is the debug string before escaping. */
+  fp_inner = open_memstream (&str_inner, &len_inner);
+  if (fp_inner == NULL)
     goto fail;
+  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");
-  if (close_memstream (fp) == -1)
+  fprintf (fp_outer, "\n");
+  if (close_memstream (fp_outer) == -1)
     goto fail;
 
-  if (!str)
+  if (!str_outer)
     goto fail;
 
   /* Send it to stderr as atomically as possible. */
-  fputs (str, stderr);
+  fputs (str_outer, stderr);
 
   errno = err;
   return;
-- 
2.39.2