Libguestfs - Mar 2023 - [libnbd PATCH v4 0/3] lib/utils: add async-signal-safe assert()

This is version 4 of the following sub-series:

[libnbd PATCH v3 06/29] lib/utils: introduce xwrite() as a more robust write()
[libnbd PATCH v3 07/29] lib/utils: add async-signal-safe assert()
[libnbd PATCH v3 08/29] lib/utils: add unit test for async-signal-safe assert()

http://mid.mail-archive.com/20230215141158.2426855-7-lersek at redhat.com
http://mid.mail-archive.com/20230215141158.2426855-8-lersek at redhat.com
http://mid.mail-archive.com/20230215141158.2426855-9-lersek at redhat.com

The Notes section on each patch records the updates for that patch.

Thanks for reviewing,
Laszlo

Laszlo Ersek (3):
  lib/utils: introduce xwritel() as a more robust and convenient write()
  lib/utils: add async-signal-safe assert()
  lib/utils: add unit test for async-signal-safe assert()

 .gitignore                   |   2 +
 configure.ac                 |   5 +
 lib/Makefile.am              |  38 +++++++-
 lib/internal.h               |  13 +++
 lib/test-fork-safe-assert.c  |  66 +++++++++++++
 lib/test-fork-safe-assert.sh |  32 ++++++
 lib/utils.c                  | 103 +++++++++++++++++++-
 7 files changed, 251 insertions(+), 8 deletions(-)
 create mode 100644 lib/test-fork-safe-assert.c
 create mode 100755 lib/test-fork-safe-assert.sh


base-commit: bbf47ffd4ac48706cbdac080ad36d1250aa1c57b

While nbd_internal_fork_safe_perror() must indeed call write(), and
arguably justifiedly ignores the return value of write(), we can still
make the write operations slightly more robust and convenient. Let's do
that by introducing xwritel():

- Let the caller pass a list of NUL-terminated strings, via stdarg /
  ellipsis notation in the parameter list.

- Handle partial writes.

- Cope with EINTR and EAGAIN errors. (A busy loop around EAGAIN on a
  non-blocking file is not great in the general case, but it's good enough
  for writing out diagnostics before giving up.)

- In the common case, handle an nbd_internal_fork_safe_perror() call with
  a single xwritel() -> writev() call chain, rather than with four
  separate write() calls. In practice, this tends to make the error
  message written to a regular file contiguous, even if other threads are
  writing to the same file. Multiple separate write() calls tend to
  interleave chunks of data from different threads.

As a side bonus, remove the path in nbd_internal_fork_safe_perror() where
at least one of the first two write() syscalls fails, and overwrites
"errno", before we get to formatting the error string from
"errno".

Thanks to Eric Blake for helping me understand the scope of Austin Group
bug reports.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    v4:
    
    - Rework with <stdarg.h> and writev().
    
    - Don't split the output into chunks of SSIZE_MAX bytes. In v3, the goal
      of that chunking was to avoid implementation-defined behavior.
      However, POSIX requires writev() to fail cleanly when more than
      SSIZE_MAX bytes would be transfered in a single call. Hence the
      original goal (avoiding implementation-defined behavior) is ensured
      simply by switching to writev(). The SSIZE_MAX limit is not expected
      to be hit in practice (_POSIX_SSIZE_MAX is 32767).
    
    - As a "bonus", remove the pre-patch possibility to trample
"errno"
      before formatting the error string.
    
    - Refresh the commit message.
    
    - The "contiguous output" from a single xwritel() -> writev()
call (as
      opposed to the "interleaved output" from multiple xwrite() ->
write()
      calls in v3) is easily testable in practice (on my end) with the
      following small patch, even though this "contiguity" is of
course not
      guaranteed:
    
    > diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
    > index e4b3b565ae2e..c66c638d331f 100644
    > --- a/generator/states-connect-socket-activation.c
    > +++ b/generator/states-connect-socket-activation.c
    > @@ -179,6 +179,8 @@  CONNECT_SA.START:
    >         * socket).
    >         */
    >        int flags = fcntl (s, F_GETFD, 0);
    > +      flags = -1;
    > +      errno = EBADF;
    >        if (flags == -1) {
    >          nbd_internal_fork_safe_perror ("fcntl: F_GETFD");
    >          _exit (126);
    
      It results in the following snippet in
      "tests/connect-systemd-socket-activation.log":
    
    > libnbd: debug: nbd1: nbd_connect_systemd_socket_activation: transition:
CONNECT.CONNECTING -> MAGIC.START
    > fcntl: F_GETFD: Bad file descriptor
    > libnbd: debug: nbd1: nbd_connect_systemd_socket_activation: transition:
MAGIC.START -> MAGIC.RECV_MAGIC
    
      Note that the child process's output is on an isolated line.
    
    - Do not pick up R-b tags from Eric and Rich due to significant changes
      in v4.
    
    context:-U5

 lib/utils.c | 87 +++++++++++++++++++-
 1 file changed, 83 insertions(+), 4 deletions(-)

diff --git a/lib/utils.c b/lib/utils.c
index 6df4f14ce9f4..62b4bfdda5c3 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -23,11 +23,14 @@
 #include <string.h>
 #include <unistd.h>
 #include <ctype.h>
 #include <errno.h>
 #include <fcntl.h>
+#include <stdarg.h>
+#include <sys/uio.h>
 
+#include "array-size.h"
 #include "minmax.h"
 
 #include "internal.h"
 
 void
@@ -179,33 +182,109 @@ nbd_internal_fork_safe_itoa (long v, char *buf, size_t
bufsize)
 #if defined (__GNUC__)
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wunused-result"
 #endif
 
+/* "Best effort" function for writing out a list of NUL-terminated
strings to a
+ * file descriptor (without the NUL-terminators). The list is terminated with
+ * (char *)NULL. Partial writes, and EINTR and EAGAIN failures are handled
+ * internally. No value is returned; only call this function for writing
+ * diagnostic data on error paths, when giving up on a higher-level action
+ * anyway.
+ *
+ * No more than 16 strings, excluding the NULL terminator, will be written. (As
+ * of POSIX Issue 7 + TC2, _XOPEN_IOV_MAX is 16.)
+ *
+ * The function is supposed to remain async-signal-safe.
+ *
+ * (The va_*() macros, while not marked async-signal-safe in Issue 7 + TC2, are
+ * considered such, per
<https://www.austingroupbugs.net/view.php?id=711>, which
+ * is binding for Issue 7 implementations via the Interpretations Track.
+ *
+ * Furthermore, writev(), while also not marked async-signal-safe in Issue 7 +
+ * TC2, is considered such, per
+ * <https://www.austingroupbugs.net/view.php?id=1455>, which is slated
for
+ * inclusion in Issue 7 TC3 (if there's going to be a TC3), and in Issue
8.)
+ */
+static void
+xwritel (int fildes, ...)
+{
+  /* Open-code the current value of _XOPEN_IOV_MAX, in order to contain stack
+   * footprint, should _XOPEN_IOV_MAX grow in the future.
+   */
+  struct iovec iovec[16], *filled, *end, *pos;
+  va_list ap;
+  char *arg;
+
+  /* Translate the variable argument list to IO vectors. Note that we cast away
+   * const-ness intentionally.
+   */
+  filled = iovec;
+  end = iovec + ARRAY_SIZE (iovec);
+  va_start (ap, fildes);
+  while (filled < end && (arg = va_arg (ap, char *)) != NULL)
+    *filled++ = (struct iovec){ .iov_base = arg, .iov_len = strlen (arg) };
+  va_end (ap);
+
+  /* Write out the IO vectors. */
+  pos = iovec;
+  while (pos < filled) {
+    ssize_t written;
+
+    /* Skip any empty vectors at the front. */
+    if (pos->iov_len == 0) {
+      ++pos;
+      continue;
+    }
+
+    /* Write out the vectors. */
+    do
+      written = writev (fildes, pos, filled - pos);
+    while (written == -1 && (errno == EINTR || errno == EAGAIN));
+
+    if (written == -1)
+      return;
+
+    /* Consume the vectors that have been written out (fully, or in part). Note
+     * that "written" is positive here.
+     */
+    do {
+      size_t advance;
+
+      advance = MIN (written, pos->iov_len);
+      /* Note that "advance" is positive here iff
"pos->iov_len" is positive. */
+      pos->iov_base = (char *)pos->iov_base + advance;
+      pos->iov_len -= advance;
+      written -= advance;
+
+      /* At least one of "written" and "pos->iov_len" is
zero here. */
+      if (pos->iov_len == 0)
+        ++pos;
+    } while (written > 0);
+  }
+}
+
 /* 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;
   const char *m = NULL;
   char buf[32];
 
-  write (2, s, strlen (s));
-  write (2, ": ", 2);
 #ifdef HAVE_STRERRORDESC_NP
   m = strerrordesc_np (errno);
 #else
 #if HAVE_SYS_ERRLIST /* NB Don't use #ifdef */
   m = errno >= 0 && errno < sys_nerr ? sys_errlist[errno] : NULL;
 #endif
 #endif
   if (!m)
     m = nbd_internal_fork_safe_itoa ((long) errno, buf, sizeof buf);
-  write (2, m, strlen (m));
-  write (2, "\n", 1);
+  xwritel (STDERR_FILENO, s, ": ", m, "\n", (char *)NULL);
 
   /* Restore original errno in case it was disturbed by the system
    * calls above.
    */
   errno = err;

Add an assert() variant that we may call between fork() and exec*().

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    v4:
    
    - rework with xwritel()
    
    - do not pick up R-b tags due to the above
    
    context:-U13

 lib/internal.h | 13 +++++++++++++
 lib/utils.c    | 16 ++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/lib/internal.h b/lib/internal.h
index 73d243a13743..4e75f97d2a8a 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -517,14 +517,27 @@ extern char *nbd_internal_printable_string (const char
*str)
 extern char *nbd_internal_printable_string_list (char **list)
   LIBNBD_ATTRIBUTE_ALLOC_DEALLOC (free);
 
 /* These are wrappers around socket(2) and socketpair(2).  They
  * always set SOCK_CLOEXEC.  nbd_internal_socket can set SOCK_NONBLOCK
  * according to the nonblock parameter.
  */
 extern int nbd_internal_socket (int domain, int type, int protocol,
                                 bool nonblock);
 extern int nbd_internal_socketpair (int domain, int type, int protocol,
                                     int *fds)
   LIBNBD_ATTRIBUTE_NONNULL (4);
 
+extern void nbd_internal_fork_safe_assert (int result, const char *file,
+                                           long line, const char *func,
+                                           const char *assertion)
+  LIBNBD_ATTRIBUTE_NONNULL (2, 4, 5);
+
+#ifdef NDEBUG
+#define NBD_INTERNAL_FORK_SAFE_ASSERT(expression) ((void)0)
+#else
+#define NBD_INTERNAL_FORK_SAFE_ASSERT(expression)                        \
+  (nbd_internal_fork_safe_assert ((expression) != 0, __FILE__, __LINE__, \
+                                  __func__, #expression))
+#endif
+
 #endif /* LIBNBD_INTERNAL_H */
diff --git a/lib/utils.c b/lib/utils.c
index 62b4bfdda5c3..255f428dd8b6 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -479,13 +479,29 @@ nbd_internal_socketpair (int domain, int type, int
protocol, int *fds)
   if (ret == 0) {
     for (i = 0; i < 2; i++) {
       if (fcntl (fds[i], F_SETFD, FD_CLOEXEC) == -1) {
         close (fds[0]);
         close (fds[1]);
         return -1;
       }
     }
   }
 #endif
 
   return ret;
 }
+
+void
+nbd_internal_fork_safe_assert (int result, const char *file, long line,
+                               const char *func, const char *assertion)
+{
+  const char *line_out;
+  char line_buf[32];
+
+  if (result)
+    return;
+
+  line_out = nbd_internal_fork_safe_itoa (line, line_buf, sizeof line_buf);
+  xwritel (STDERR_FILENO, file, ":", line_out, ": ", func,
": Assertion `",
+           assertion, "' failed.\n", (char *)NULL);
+  abort ();
+}

Don't try to test async-signal-safety, only that
NBD_INTERNAL_FORK_SAFE_ASSERT() works similarly to assert():

- it prints diagnostics to stderr,

- it calls abort().

Some unfortunate gymnastics are necessary to avoid littering the system
with unwanted core dumps.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    v4:
    
    - update Red Hat Copyright Notices in the new files, in accordance with
      commit bbf47ffd4ac4 ("Update Red Hat Copyright Notices",
2023-03-04)
    
    - explain the TRUE and FALSE macro definitions in code comments [Eric]
    
    - test that the pattern "`TRUE'" is not present in the output
file; that
      is, the passing assertion does not generate unexpected output [Eric]
    
    - do not pick up R-b tags due to the logic update in the last bullet
      above

 configure.ac                 |  5 ++
 lib/test-fork-safe-assert.c  | 66 ++++++++++++++++++++
 lib/Makefile.am              | 38 +++++++++--
 lib/test-fork-safe-assert.sh | 32 ++++++++++
 .gitignore                   |  2 +
 5 files changed, 139 insertions(+), 4 deletions(-)

diff --git a/configure.ac b/configure.ac
index b6d60c3df6a1..62fe470b6cd5 100644
--- a/configure.ac
+++ b/configure.ac
@@ -132,11 +132,16 @@ dnl Check for various libc functions, all optional.
 dnl
 dnl posix_fadvise helps to optimise linear reads and writes.
 dnl
+dnl When /proc/sys/kernel/core_pattern starts with a pipe (|) symbol, Linux
+dnl ignores "ulimit -c" and (equivalent) setrlimit(RLIMIT_CORE)
actions, for
+dnl disabling core dumping. Only prctl() can be used then, for that purpose.
+dnl
 dnl strerrordesc_np (glibc only) is preferred over sys_errlist:
 dnl https://lists.fedoraproject.org/archives/list/glibc at
lists.fedoraproject.org/thread/WJHGG2OO7ABNAYICGA5WQZ2Q34Q2FEHU/
 AC_CHECK_FUNCS([\
         posix_fadvise \
         posix_memalign \
+        prctl \
         strerrordesc_np \
         valloc])
 
diff --git a/lib/test-fork-safe-assert.c b/lib/test-fork-safe-assert.c
new file mode 100644
index 000000000000..4a4f6e88ce65
--- /dev/null
+++ b/lib/test-fork-safe-assert.c
@@ -0,0 +1,66 @@
+/* nbd client library in userspace
+ * Copyright Red Hat
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#ifdef HAVE_PRCTL
+#include <sys/prctl.h>
+#endif
+#include <sys/resource.h>
+
+#undef NDEBUG
+
+#include "internal.h"
+
+/* Define these to verify that NBD_INTERNAL_FORK_SAFE_ASSERT() properly
+ * stringifies the expression passed to it.
+ */
+#define TRUE  1
+#define FALSE 0
+
+int
+main (void)
+{
+  struct rlimit rlimit;
+
+  /* The standard approach for disabling core dumping. Has no effect on Linux
if
+   * /proc/sys/kernel/core_pattern starts with a pipe (|) symbol.
+   */
+  if (getrlimit (RLIMIT_CORE, &rlimit) == -1) {
+    perror ("getrlimit");
+    return EXIT_FAILURE;
+  }
+  rlimit.rlim_cur = 0;
+  if (setrlimit (RLIMIT_CORE, &rlimit) == -1) {
+    perror ("setrlimit");
+    return EXIT_FAILURE;
+  }
+
+#ifdef HAVE_PRCTL
+  if (prctl (PR_SET_DUMPABLE, 0, 0, 0, 0) == -1) {
+    perror ("prctl");
+    return EXIT_FAILURE;
+  }
+#endif
+
+  NBD_INTERNAL_FORK_SAFE_ASSERT (TRUE);
+  NBD_INTERNAL_FORK_SAFE_ASSERT (FALSE);
+  return 0;
+}
diff --git a/lib/Makefile.am b/lib/Makefile.am
index 52b525819bde..326b550f90bd 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -25,15 +25,30 @@ generator_built = \
 	unlocked.h \
 	$(NULL)
 
+CLEANFILES += \
+	test-fork-safe-assert.err \
+	$(NULL)
+
 EXTRA_DIST = \
 	$(generator_built) \
 	libnbd.syms \
+	test-fork-safe-assert.sh \
 	$(NULL)
 
 lib_LTLIBRARIES = libnbd.la
 
 BUILT_SOURCES = $(generator_built)
 
+AM_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	-I$(top_srcdir)/common/include \
+	-I$(top_srcdir)/common/utils \
+	$(NULL)
+
+AM_CFLAGS = \
+	$(WARNINGS_CFLAGS) \
+	$(NULL)
+
 libnbd_la_SOURCES = \
 	aio.c \
 	api.c \
@@ -60,13 +75,11 @@ libnbd_la_SOURCES = \
 	utils.c \
 	$(NULL)
 libnbd_la_CPPFLAGS = \
-	-I$(top_srcdir)/include \
-	-I$(top_srcdir)/common/include \
-	-I$(top_srcdir)/common/utils \
+	$(AM_CPPFLAGS) \
 	-Dsysconfdir=\"$(sysconfdir)\" \
 	$(NULL)
 libnbd_la_CFLAGS = \
-	$(WARNINGS_CFLAGS) \
+	$(AM_CFLAGS) \
 	$(PTHREAD_CFLAGS) \
 	$(GNUTLS_CFLAGS) \
 	$(LIBXML2_CFLAGS) \
@@ -86,3 +99,20 @@ libnbd_la_LDFLAGS = \
 
 pkgconfigdir = $(libdir)/pkgconfig
 pkgconfig_DATA = libnbd.pc
+
+# Unit tests.
+
+TESTS = \
+	test-fork-safe-assert.sh \
+	$(NULL)
+
+check_PROGRAMS = \
+	test-fork-safe-assert \
+	$(NULL)
+
+test_fork_safe_assert_SOURCES = \
+	$(top_srcdir)/common/utils/vector.c \
+	errors.c \
+	test-fork-safe-assert.c \
+	utils.c \
+	$(NULL)
diff --git a/lib/test-fork-safe-assert.sh b/lib/test-fork-safe-assert.sh
new file mode 100755
index 000000000000..2cf0a29a4c6d
--- /dev/null
+++ b/lib/test-fork-safe-assert.sh
@@ -0,0 +1,32 @@
+#!/usr/bin/env bash
+# nbd client library in userspace
+# Copyright Red Hat
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+set +e
+
+./test-fork-safe-assert 2>test-fork-safe-assert.err
+exit_status=$?
+
+set -e
+
+test $exit_status -gt 128
+signal_name=$(kill -l $exit_status)
+test "x$signal_name" = xABRT || test "x$signal_name" =
xSIGABRT
+
+ptrn="^test-fork-safe-assert\\.c:[0-9]+: main: Assertion \`FALSE'
failed\\.\$"
+grep -E -q -- "$ptrn" test-fork-safe-assert.err
+grep -v -q "\`TRUE'" test-fork-safe-assert.err
diff --git a/.gitignore b/.gitignore
index 4ebd96519c29..54bc20173f95 100644
--- a/.gitignore
+++ b/.gitignore
@@ -128,6 +128,8 @@ Makefile.in
 /lib/states-run.c
 /lib/states.c
 /lib/states.h
+/lib/test-fork-safe-assert
+/lib/test-fork-safe-assert.err
 /lib/unlocked.h
 /libtool
 /ltmain.sh

On 3/15/23 12:01, Laszlo Ersek wrote:
> This is version 4 of the following sub-series:
> 
> [libnbd PATCH v3 06/29] lib/utils: introduce xwrite() as a more robust
write()
> [libnbd PATCH v3 07/29] lib/utils: add async-signal-safe assert()
> [libnbd PATCH v3 08/29] lib/utils: add unit test for async-signal-safe
assert()
> 
> http://mid.mail-archive.com/20230215141158.2426855-7-lersek at redhat.com
> http://mid.mail-archive.com/20230215141158.2426855-8-lersek at redhat.com
> http://mid.mail-archive.com/20230215141158.2426855-9-lersek at redhat.com
> 
> The Notes section on each patch records the updates for that patch.
> 
> Thanks for reviewing,
> Laszlo
> 
> Laszlo Ersek (3):
>   lib/utils: introduce xwritel() as a more robust and convenient write()
>   lib/utils: add async-signal-safe assert()
>   lib/utils: add unit test for async-signal-safe assert()
> 
>  .gitignore                   |   2 +
>  configure.ac                 |   5 +
>  lib/Makefile.am              |  38 +++++++-
>  lib/internal.h               |  13 +++
>  lib/test-fork-safe-assert.c  |  66 +++++++++++++
>  lib/test-fork-safe-assert.sh |  32 ++++++
>  lib/utils.c                  | 103 +++++++++++++++++++-
>  7 files changed, 251 insertions(+), 8 deletions(-)
>  create mode 100644 lib/test-fork-safe-assert.c
>  create mode 100755 lib/test-fork-safe-assert.sh
> 
> 
> base-commit: bbf47ffd4ac48706cbdac080ad36d1250aa1c57b
Series merged as commit range 48eca6a25468..c7d02b4b08ee.

Thanks!
Laszlo