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

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. */ --