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

Thu Mar 16 09:50:06 UTC 2023

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 at redhat.com>
>> ---
>
>> 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.org/thread/WJHGG2OO7ABNAYICGA5WQZ2Q34Q2FEHU/
>>  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

  18509 for ac_func in \
  18510         posix_fadvise \
  18511         posix_memalign \
  18512         prctl \
  18513         strerrordesc_np \
  18514         valloc
  18515 do :
  18516   as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
  18517 ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
  18518 if eval test \"x\$"$as_ac_var"\" = x"yes"; then :
  18519   cat >>confdefs.h <<_ACEOF
  18520 #define `$as_echo "HAVE_$ac_func" | $as_tr_cpp` 1
  18521 _ACEOF
  18522
  18523 fi
  18524 done

I.e., we call "ac_fn_c_check_func" with "prctl" passed as second
argument.

Then, that function is defined as follows:

   2001 # ac_fn_c_check_func LINENO FUNC VAR
   2002 # ----------------------------------
   2003 # Tests whether FUNC exists, setting the cache variable VAR accordingly
   2004 ac_fn_c_check_func ()
   2005 {
   2006   as_lineno=${as_lineno-"$1"} as_lineno_stack=as_lineno_stack=$as_lineno_stack
   2007   { $as_echo "$as_me:${as_lineno-$LINENO}: checking for $2" >&5
   2008 $as_echo_n "checking for $2... " >&6; }
   2009 if eval \${$3+:} false; then :
   2010   $as_echo_n "(cached) " >&6
   2011 else
   2012   cat confdefs.h - <<_ACEOF >conftest.$ac_ext
   2013 /* end confdefs.h.  */
   2014 /* Define $2 to an innocuous variant, in case <limits.h> declares $2.
   2015    For example, HP-UX 11i <limits.h> declares gettimeofday.  */
   2016 #define $2 innocuous_$2
   2017
   2018 /* System header to define __stub macros and hopefully few prototypes,
   2019     which can conflict with char $2 (); below.
   2020     Prefer <limits.h> to <assert.h> if __STDC__ is defined, since
   2021     <limits.h> exists even on freestanding compilers.  */
   2022
   2023 #ifdef __STDC__
   2024 # include <limits.h>
   2025 #else
   2026 # include <assert.h>
   2027 #endif
   2028
   2029 #undef $2
   2030
   2031 /* Override any GCC internal prototype to avoid an error.
   2032    Use char because int might match the return type of a GCC
   2033    builtin and then its argument prototype would still apply.  */
   2034 #ifdef __cplusplus
   2035 extern "C"
   2036 #endif
   2037 char $2 ();
   2038 /* The GNU C library defines this for functions which it implements
   2039     to always fail with ENOSYS.  Some functions are actually named
   2040     something starting with __ and the normal name is an alias.  */
   2041 #if defined __stub_$2 || defined __stub___$2
   2042 choke me
   2043 #endif
   2044
   2045 int
   2046 main ()
   2047 {
   2048 return $2 ();
   2049   ;
   2050   return 0;
   2051 }
   2052 _ACEOF
   2053 if ac_fn_c_try_link "$LINENO"; then :
   2054   eval "$3=yes"
   2055 else
   2056   eval "$3=no"
   2057 fi
   2058 rm -f core conftest.err conftest.$ac_objext \
   2059     conftest$ac_exeext conftest.$ac_ext
   2060 fi
   2061 eval ac_res=\$$3
   2062                { $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_res" >&5
   2063 $as_echo "$ac_res" >&6; }
   2064   eval $as_lineno_stack; ${as_lineno_stack:+:} unset as_lineno
   2065
   2066 } # ac_fn_c_check_func

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/Generic-Headers.html>.
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 at redhat.com>
>

Thank you!
Laszlo