Libguestfs - Mar 2023 - [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

On Thu, Mar 16, 2023 at 10:50:06AM +0100, Laszlo Ersek
wrote:
> 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>.
Bingo - you caught my poorly stated conclusion.
> 
> 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.
I'm not sure if it remains a core dump, or if it becomes dumpabale to
ABRT (or whatever else was consuming the pipeline) which in turn leads
to unnecessary load on the ABRT servers and bugzilla (etc) for dealing
with an expected crash, but yeah, that appears to be the worst effect
of a false negative.
> 
> (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.
Yep, that's how autoconf checks whether an external symbol is provided
by the current set of linked libraries.  Where it can go wrong is when
a public header has a macro that redefines the normal symbol name into
the actual library linkage name (think about the public stat()
vs. internal __stat64() mess when big files were first introduced, or
more recently, figuring out how to support 64-bit time on 32-bit
systems).
> 
> ... 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.
Autoconf also has a way to write one-off function checks with
AC_CHECK_FUNCTION where you can supply your own #include's specific to
the normal usage of that function (the most robust configure tests are
the ones that mirror actual usage as closely as possible); but
AC_CHECK_FUNCTIONS (with its shell loop) is so much more compact, that
I didn't think it is worth worrying about it.
> 
> 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.
And your conclusion matched mine - this is such a niche function that
even though we are checking only one of the two aspects that both have
to be in play to use the function (the header, and the external
linkage name), we can rely on both or neither aspects being present on
a given system, effectively letting us pick just one configure probe
as the witness for both aspects.
> 
> So indeed I'll stick with the AC_CHECK_FUNCS approach.
Yep, I think the function check is stronger than the header check, and
that's why I gave R-b:
> 
> >
> >> +++ 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!
And while you spent some time learning things, I'm glad we are in
agreement that as ugly as this niche case is, we don't have to do even
more churn to "improve" the situation.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org