Re: [Libguestfs] [PATCH libnbd v2 1/2] lib: Don't use perror after fork in nbd_connect_callback.
On 9/30/19 11:32 AM, Richard W.M. Jones wrote:
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(-)
+++ 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;], [
We probably ought to use AC_CACHE_CHECK for our various configure.ac
things, to let a user have more control at overriding things if we
probed wrong without having to patch configure.ac. But that's a
separate problem, not affecting this one.
+
+/* 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 */
Easy enough to work around. In the parameters:
unsigned long v,
and the condition here should be:
if ((long) v < 0)
+ }
+ if (v == 0)
+ buf[i--] = '0';
+ else {
+ while (v) {
+ buf[i--] = '0' + (v % 10);
+ v /= 10;
+ }
+ }
+ if (neg)
+ buf[i--] = '-';
+
+ i++;
+ return &buf[i];
Do we want to assert that bufsize is large enough? (Or rather, abort()
if it is not, since assert() is borderline in fork-safe context)
+}
+
+/* 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));
Surprisingly, strlen() is not listed in current POSIX' list of
async-signal-safe functions. But I have an open bug to remedy that, and
don't see any problem in using it.
I think it looks nicer to use STDERR_FILENO instead of 2.
+ 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;
+}
Otherwise looks sane.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org