Re: [Libguestfs] [libnbd PATCH v4 3/3] lib/utils: add unit test for async-signal-safe assert()

On 3/15/23 18:25, Eric Blake wrote: > On Wed, Mar 15, 2023 at 12:01:57PM +0100, Laszlo Ersek wrote: >> 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(a)redhat.com&gt; >> --- > >> 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@lists.fedoraproject.o... >> AC_CHECK_FUNCS([\ >> posix_fadvise \ >> posix_memalign \ >> + prctl \ >> strerrordesc_np \ >> valloc]) > > AC_CHECK_FUNCS looks for whether the given entry point can be linked > with, which is okay for functions in the common headers (<stdlib.h>, > <unistd.h>, ...) that autoconf includes in its test programs by > default. But... > >> >> 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 @@ > >> + >> +#include <stdio.h> >> +#include <stdlib.h> >> +#ifdef HAVE_PRCTL >> +#include <sys/prctl.h> >> +#endif > > ...the fact that prctl() is in a non-standard header makes me wonder > if we might fail to detect the function merely because we didn't > include the right header, rather than because its symbol was not > exported. > > On the other hand, prctl() is definitely Linux-specific, so I think > you are quite safe in assuming that <sys/prctl.h> exists if and only > if prctl() is a linkable entry point. If it does turn out to break > someone, we can fix it in a followup patch, so no change needed in > your usage at this time. *shudder* Another hideous piece of complexity that's orthogonal to what one wants to do :) So, I investigated a bit. If I understand correctly, your point is that we could get a *false negative* here, because AC_CHECK_FUNCS might not find prctl() due to the autoconf-generated test program not #including <sys/prctl.h>.

Assuming I got that right, I have two comments on it: (1) A false negative in this case would not be a huge problem; we'd miss out on prctl(), i.e. the test program would remain dumpable on Linux. The test would still function as needed, just litter the user's machine with a coredump during "make check". Not ideal, but also not tragic.

(2) I believe I disagree with the idea that AC_CHECK_FUNCS might not find an otherwise existent prctl() *due to* AC_CHECK_FUNCS not generating "#include <sys/prctl.h>" into the test program. On my RHEL-9.1 install (using autoconf-2.69-38.el9.noarch), I've checked the generated "configure" script. First, we have

As far as I can tell, the test program provides its own declaration for prctl() on line 2037, so it does not depend on any system header providing a real prototype.

... I figured autoconf should have a "header check" too, and indeed it does: AC_CHECK_HEADER, AC_CHECK_HEADERS, at <https://www.gnu.org/software/autoconf/manual/autoconf-2.69/html_node/Gene...;. We do use AC_CHECK_HEADERS in libnbd.

The question is now whether checking for <sys/prctl.h> with AC_CHECK_HEADERS is "more robust" than checking for prctl() with AC_CHECK_FUNCS(). I'd say: AC_CHECK_FUNCS() is a tiny bit stronger (we actually want to be able to call the particular prctl() function), but they should be mostly *interchangeable*. I'm saying that because prctl() is "Linux-specific" (per prctl(2) manual), so: (a) <sys/prctl.h> existing (per AC_CHECK_HEADERS), but not exposing the real -- and linkable -- prctl() prototype, (b) a call to prctl() being linkable (via the bogus declaration used by AC_CHECK_HEADERS), but <sys/prctl.h> not exposing a proper prctl() prototype, are *equally* Linux userspace bugs.

So indeed I'll stick with the AC_CHECK_FUNCS approach.

> >> +++ b/lib/test-fork-safe-assert.sh >> @@ -0,0 +1,32 @@ >> +#!/usr/bin/env bash > > Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; > Thank you!