Eric Blake
2021-Aug-11 14:28 UTC
[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. */
--
2.31.1
Richard W.M. Jones
2021-Aug-11 14:39 UTC
[Libguestfs] [nbdkit PATCH] maint: Default to probing valgrind at runtime
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