Eric Blake
2023-Mar-15 17:25 UTC
[Libguestfs] [libnbd PATCH v4 3/3] lib/utils: add unit test for async-signal-safe assert()
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 at 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.> +++ b/lib/test-fork-safe-assert.sh > @@ -0,0 +1,32 @@ > +#!/usr/bin/env bashReviewed-by: Eric Blake <eblake at redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Laszlo Ersek
2023-Mar-16 09:50 UTC
[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 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 at 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