11:32 a.m.
perror is not fork-safe and so could deadlock. Instead open code a
fork-safe version of perror. While this fixes the current behaviour,
in the long term we'd like to capture the error message into the usual
error mechanism, so this is not the full and final fix for this issue.
Also this fixes the exit code to be 126/127 instead of 1.
Thanks: Eric Blake
---
TODO | 1 +
configure.ac | 10 ++++++
generator/states-connect.c | 7 ++--
lib/internal.h | 2 ++
lib/utils.c | 66 ++++++++++++++++++++++++++++++++++++++
5 files changed, 84 insertions(+), 2 deletions(-)
diff --git a/TODO b/TODO
index 6c03736..2f23a34 100644
--- a/TODO
+++ b/TODO
@@ -57,3 +57,4 @@ Suggested API improvements:
- it should be possible to use nbd_close and never block, so
maybe nbd_shutdown should wait for the subprocess or there
should be another API to do this
+ - capture error message when nbd_connect_command fails
diff --git a/configure.ac b/configure.ac
index 6296ccf..35a4fed 100644
--- a/configure.ac
+++ b/configure.ac
@@ -77,6 +77,16 @@ AC_CHECK_HEADERS([\
stdatomic.h \
sys/endian.h])
+dnl Check for sys_errlist (optional).
+AC_MSG_CHECKING([for sys_errlist])
+AC_TRY_LINK([], [extern int sys_errlist; char *p = &sys_errlist;], [
+ AC_DEFINE([HAVE_SYS_ERRLIST], [1],
+ [Define if libc has a sys_errlist variable.])
+ AC_MSG_RESULT([yes])
+], [
+ AC_MSG_RESULT([no])
+])
+
dnl Check for GnuTLS (optional, for TLS support).
AC_ARG_WITH([gnutls],
[AS_HELP_STRING([--without-gnutls],
diff --git a/generator/states-connect.c b/generator/states-connect.c
index 7a828f4..04e894c 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -259,8 +259,11 @@ STATE_MACHINE {
signal (SIGPIPE, SIG_DFL);
execvp (h->argv[0], h->argv);
- perror (h->argv[0]);
- _exit (EXIT_FAILURE);
+ nbd_internal_fork_safe_perror (h->argv[0]);
+ if (errno == ENOENT)
+ _exit (127);
+ else
+ _exit (126);
}
/* Parent. */
diff --git a/lib/internal.h b/lib/internal.h
index bdb0e83..31bc3d4 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -384,5 +384,7 @@ extern void nbd_internal_hexdump (const void *data, size_t len, FILE
*fp);
extern size_t nbd_internal_string_list_length (char **argv);
extern char **nbd_internal_copy_string_list (char **argv);
extern void nbd_internal_free_string_list (char **argv);
+extern const char *nbd_internal_fork_safe_itoa (long v, char *buf, size_t len);
+extern void nbd_internal_fork_safe_perror (const char *s);
#endif /* LIBNBD_INTERNAL_H */
diff --git a/lib/utils.c b/lib/utils.c
index 2d7fbf3..644781b 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -20,7 +20,10 @@
#include <stdio.h>
#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
#include <ctype.h>
+#include <errno.h>
#include "internal.h"
@@ -96,3 +99,66 @@ nbd_internal_free_string_list (char **argv)
free (argv[i]);
free (argv);
}
+
+/* Like sprintf (s, "%ld", v). The caller must supply a scratch
+ * buffer which is large enough for the result (32 bytes is fine), but
+ * note that the returned string does not point to the start of this
+ * buffer.
+ */
+const char *
+nbd_internal_fork_safe_itoa (long v, char *buf, size_t bufsize)
+{
+ size_t i = bufsize - 1;
+ bool neg = false;
+
+ buf[i--] = '\0';
+ if (v < 0) {
+ neg = true;
+ v = -v; /* XXX fails for INT_MIN */
+ }
+ if (v == 0)
+ buf[i--] = '0';
+ else {
+ while (v) {
+ buf[i--] = '0' + (v % 10);
+ v /= 10;
+ }
+ }
+ if (neg)
+ buf[i--] = '-';
+
+ i++;
+ return &buf[i];
+}
+
+/* Fork-safe version of perror. ONLY use this after fork and before
+ * exec, the rest of the time use set_error().
+ */
+void
+nbd_internal_fork_safe_perror (const char *s)
+{
+ const int err = errno;
+
+#if defined(__GNUC__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wunused-result"
+#endif
+ write (2, s, strlen (s));
+ write (2, ": ", 2);
+#if HAVE_SYS_ERRLIST
+ write (2, sys_errlist[errno], strlen (sys_errlist[errno]));
+#else
+ char buf[32];
+ const char *v = nbd_internal_fork_safe_itoa ((long) errno, buf, sizeof buf);
+ write (2, v, strlen (v));
+#endif
+ write (2, "\n", 1);
+#if defined(__GNUC__)
+#pragma GCC diagnostic pop
+#endif
+
+ /* Restore original errno in case it was disturbed by the system
+ * calls above.
+ */
+ errno = err;
+}
--
2.23.0