[Libguestfs] [PATCH nbdkit] tests: Invoke MALLOC_CHECK_ correctly and only for glibc

[Eric Blake]

On Sat, Aug 21, 2021 at 02:39:37PM +0100, Richard W.M. Jones wrote:
> MALLOC_CHECK_ and MALLOC_PRELOAD_ environment variables have been
> replaced in glibc 2.34 by GLIBC_TUNABLES settings[1][2].  In addition
> the new glibc requires that you preload a new library called
> libc_malloc_debug.so.0 to get these features.
> 
> These features never worked in non-glibc (but the environment
> variables are ignored).
> 
> Only define MALLOC_* environment variables when we detect glibc < 2.34,
> using the ordinary glibc macros[3].
> 
> For glibc >= 2.34, use the new library and GLIBC_TUNABLES.  Note this
> relies on LD_PRELOAD warning but otherwise not breaking if a preload
> library does not exist.
> 
> [1] https://www.gnu.org/software/libc/manual/html_node/Memory-Allocation-Tunables.html
> [2] https://sourceware.org/pipermail/libc-alpha/2021-August/129718.html
> [3] https://sourceforge.net/p/predef/wiki/Libraries/
> 
> Thanks: Eric Blake
> ---
>  configure.ac      | 24 ++++++++++++++++++++++++
>  tests/Makefile.am | 23 +++++++++++++++++------
>  2 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 9eb9b174..bdb56b37 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -569,6 +569,30 @@ AS_IF([test "x$is_windows" = "xyes"],[
>  
>  AC_SEARCH_LIBS([getaddrinfo], [network socket])
>  
> +dnl Does this libc have MALLOC_CHECK_ and MALLOC_PERTURB_ (both glibc)
> +dnl and does the feature require libc_malloc_debug.so (glibc >= 2.34)?
> +AC_MSG_CHECKING([if this is glibc])
> +AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
> +#include <limits.h>
> +#ifndef __GLIBC__
> +#error "not glibc"
> +#endif
> +        ]])], [is_glibc=yes], [is_glibc=no]
> +)
> +AC_MSG_RESULT([$is_glibc])
> +AM_CONDITIONAL([HAVE_GLIBC], [test "x$is_glibc" = "xyes"])

The justification for this check is a bit weak; after all...

> +
> +AC_MSG_CHECKING([if this is glibc >= 2.34])
> +AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[
> +#include <limits.h>
> +#if !defined(__GLIBC__) || __GLIBC__ < 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ < 34)
> +#error "not glibc 2.34"
> +#endif
> +        ]])], [is_glibc_234=yes], [is_glibc_234=no]
> +)
> +AC_MSG_RESULT([$is_glibc_234])
> +AM_CONDITIONAL([HAVE_GLIBC_234], [test "x$is_glibc_234" = "xyes"])

...this check alone determines if you are glibc 2.34 or greater, and
anything else is either not glibc or is older glibc, without having to
depend on $is_glibc.

> +
>  dnl Check for SELinux socket labelling (optional).
>  PKG_CHECK_MODULES([LIBSELINUX], [libselinux], [
>      AC_SUBST([LIBSELINUX_CFLAGS])
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 02b6d44a..90363f62 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -61,21 +61,32 @@ EXTRA_DIST = README.tests
>  EXTRA_PROGRAMS =
>  
>  # Use the 'direct' backend, and ensure maximum libguestfs debugging.
> -#
>  # Enable libnbd debugging.
> -#
> -# Enable malloc-check as a cheap way to find some use-after-free and
> -# uninitialized read problems when using glibc, and doesn't affect
> -# normal operation or other libc.
>  TESTS_ENVIRONMENT = \
>  	SRCDIR=$(srcdir) \
>  	LIBGUESTFS_ATTACH_METHOD=appliance \
>  	LIBGUESTFS_DEBUG=1 \
>  	LIBGUESTFS_TRACE=1 \
>  	LIBNBD_DEBUG=1 \
> +	$(NULL)
> +
> +# Enable malloc-check as a cheap way to find some use-after-free and
> +# uninitialized read problems when using glibc, and doesn't affect
> +# normal operation or other libc.
> +random = $(shell bash -c 'echo $$(( 1 + (RANDOM & 255) ))')
> +if HAVE_GLIBC_234
> +TESTS_ENVIRONMENT += \
> +	LD_PRELOAD="$${LD_PRELOAD:+"$$LD_PRELOAD:"}$(libdir)/libc_malloc_debug.so.0" \
> +	GLIBC_TUNABLES=glibc.malloc.check=1:glibc.malloc.perturb=$(random) \
> +	$(NULL)
> +else
> +if HAVE_GLIBC
> +TESTS_ENVIRONMENT += \
>  	MALLOC_CHECK_=1 \
> -	MALLOC_PERTURB_=$(shell bash -c 'echo $$(( 1 + (RANDOM & 255) ))') \
> +	MALLOC_PERTURB_=$(random) \
>  	$(NULL)
> +endif

And here, the HAVE_GLIBC condition is weak: even if you are not on
glibc, exporting the environment variables doesn't hurt.  I agree that
we want to modify LD_PRELOAD for new-enough glibc, but avoiding the
modification of MALLOC_CHECK_ on non-glibc just to limit it to older
glibc may not be worth the extra conditional.

> +endif
>  
>  if !IS_WINDOWS
>  # Ensure we're testing the local copy by running everything through
> -- 
> 2.32.0
>

But overall the idea makes sense, as otherwise we are regressing in
our ability to do cheap detection of certain memory bugs when
upgrading to newer glibc.

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