[Libguestfs] [nbdkit PATCH] maint: Default to probing valgrind at runtime

[Richard W.M. Jones]

On Wed, Aug 11, 2021 at 09:28:17AM -0500, Eric Blake wrote:
> When we added --enable-valgrind in commit ac4ad5545, we argued that a
> runtime probe for valgrind might be invasive enough that it should be
> opt-in for developers only.  But it turns out that valgrind.h's
> implementation for RUNNING_ON_VALGRIND is intentionally minimally
> invasive: it does not link in any external library or function calls,
> but merely injects a few benign instructions (a sequence of rol on
> x86_64) that the valgrind JIT recognizes as special, but which are a
> no-op on bare-metal runs.  Furthermore, we are not checking valgrind
> use in a hotspot.  So there is no real penalty to defaulting the
> valgrind probe to on, which makes 'make check-valgrind' more likely to
> work out-of-the-box.  Still, we keep --disable-valgrind for anyone
> that does not want to use 'CFLAGS=-DNVALGRIND' as another alternative
> for avoiding even those few assembly instructions in a production
> binary.
> 
> Also, we misused AC_ARG_ENABLE, such that './configure
> --disable-valgrind' actually enables valgrind.  We then copied that
> same bug for --disable-libfuzzer (commit 9b0300ca).  An audit of the
> other AC_ARG_WITH and AC_ARG_ENABLE did not find other instances of
> this misuse.
> 
> In addition to tweaking the configure default, we need to rearrange
> the logic of internal.h so that libfuzzer and sanitize address still
> unconditionally disable dlclose, even when valgrind headers were
> detected.
> ---
>  configure.ac      | 20 +++++++++++---------
>  server/internal.h | 14 ++++++++------
>  2 files changed, 19 insertions(+), 15 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index a813dfff..ef9c99b4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -638,20 +638,22 @@ dnl Check for valgrind.
>  AC_CHECK_PROG([VALGRIND],[valgrind],[valgrind],[no])
> 
>  dnl If valgrind headers are available (optional).
> -dnl Since this is only useful for developers, you have to enable
> -dnl it explicitly using --enable-valgrind.
> +dnl Although the runtime check adds only a trivial amount of code, you can
> +dnl avoid it with explicit --disable-valgrind.
>  AC_ARG_ENABLE([valgrind],
> -    [AS_HELP_STRING([--enable-valgrind],
> -                    [enable Valgrind extensions (for developers)])],
> -    [enable_valgrind=yes],
> -    [enable_valgrind=no])
> -AS_IF([test "x$enable_valgrind" = "xyes"],[
> +    [AS_HELP_STRING([--disable-valgrind],
> +                    [disable Valgrind probe])],
> +    [],
> +    [enable_valgrind=check])
> +AS_IF([test "x$enable_valgrind" != "xno"],[
>      PKG_CHECK_MODULES([VALGRIND], [valgrind], [
>          AC_SUBST([VALGRIND_CFLAGS])
>          AC_SUBST([VALGRIND_LIBS])
>          AC_DEFINE([HAVE_VALGRIND],[1],[Valgrind headers found at compile time])
>      ],[
> -        AC_MSG_ERROR([--enable-valgrind given, but Valgrind headers are not available])
> +        AS_IF([test "x$enable_valgrind" = "xyes"], [
> +            AC_MSG_ERROR([--enable-valgrind given, but Valgrind headers are not available])
> +        ])
>      ])
>  ])
> 
> @@ -660,7 +662,7 @@ dnl normal builds.  See fuzzing/README.
>  AC_ARG_ENABLE([libfuzzer],
>      [AS_HELP_STRING([--enable-libfuzzer],
>                      [build libFuzzer test binary (developers only)])],
> -    [enable_libfuzzer=yes],
> +    [],
>      [enable_libfuzzer=no])
>  AS_IF([test "x$enable_libfuzzer" = "xyes"],[
>      AC_DEFINE([ENABLE_LIBFUZZER],[1],[Enable special libFuzzer binary])
> diff --git a/server/internal.h b/server/internal.h
> index b1473911..bc81b786 100644
> --- a/server/internal.h
> +++ b/server/internal.h
> @@ -63,11 +63,7 @@
>  #define if_verbose if (verbose)
>  #endif
> 
> -#if HAVE_VALGRIND
> -# include <valgrind.h>
> -/* http://valgrind.org/docs/manual/faq.html#faq.unhelpful */
> -# define DO_DLCLOSE !RUNNING_ON_VALGRIND
> -#elif defined(__SANITIZE_ADDRESS__)
> +#if defined(__SANITIZE_ADDRESS__)
>  # define DO_DLCLOSE 0
>  #elif ENABLE_LIBFUZZER
>  /* XXX This causes dlopen in the server to leak during fuzzing.
> @@ -76,7 +72,13 @@
>   */
>  # define DO_DLCLOSE 0
>  #else
> -# define DO_DLCLOSE 1
> +# if HAVE_VALGRIND
> +#  include <valgrind.h>
> +/* http://valgrind.org/docs/manual/faq.html#faq.unhelpful */
> +#  define DO_DLCLOSE !RUNNING_ON_VALGRIND
> +# else
> +#  define DO_DLCLOSE 1
> +# endif
>  #endif
> 
>  /* Declare program_name. */
> -- 

ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org