[RFC PATCH] Allow annotating calloc for Valgrind

[RFC PATCH] Allow annotating calloc for Valgrind

[Alexander Monakov]

With mal0_clear entered only for larger-than-one-page allocations, a
few additional nops and stack accesses don't matter.

This only needs Valgrind headers to build, Valgrind itself doesn't need to be
built/installed.  However, valgrind.h unconditionally includes stdarg.h.

Untested.
---
 configure           | 11 +++++++++++
 src/malloc/malloc.c |  4 ++++
 2 files changed, 15 insertions(+)

diff --git a/configure b/configure
index c2db298c..7e78094f 100755
--- a/configure
+++ b/configure
@@ -30,6 +30,7 @@ System types:
 Optional features:
   --enable-optimize=...   optimize listed components for speed over size [auto]
   --enable-debug          build with debugging information [disabled]
+  --enable-valgrind       add Valgrind annotations [disabled]
   --enable-warnings       build with recommended warnings flags [disabled]
   --enable-visibility     use global visibility options to optimize PIC [auto]
   --enable-wrapper=...    build given musl toolchain wrapper [auto]
@@ -134,6 +135,7 @@ build=
 target=
 optimize=auto
 debug=no
+valgrind=no
 warnings=no
 visibility=auto
 shared=auto
@@ -161,6 +163,8 @@ case "$arg" in
 --disable-optimize) optimize=no ;;
 --enable-debug|--enable-debug=yes) debug=yes ;;
 --disable-debug|--enable-debug=no) debug=no ;;
+--enable-valgrind|--enable-valgrind=yes) valgrind=yes ;;
+--disable-valgrind|--enable-valgrind=no) valgrind=no ;;
 --enable-warnings|--enable-warnings=yes) warnings=yes ;;
 --disable-warnings|--enable-warnings=no) warnings=no ;;
 --enable-visibility|--enable-visibility=yes) visibility=yes ;;
@@ -390,6 +394,13 @@ tryflag CFLAGS_MEMOPS -fno-tree-loop-distribute-patterns
 #
 test "$debug" = yes && CFLAGS_AUTO=-g
 
+if test x"$valgrind" = xyes  ; then
+CPPFLAGS="$CPPFLAGS -DVG_MEMCHECK_H='<memcheck.h>'"
+else
+CPPFLAGS="$CPPFLAGS -DVG_MEMCHECK_H='</dev/null>'"
+fi
+CPPFLAGS="${CPPFLAGS# }"
+
 #
 # Preprocess asm files to add extra debugging information if debug is
 # enabled, our assembler supports the needed directives, and the
diff --git a/src/malloc/malloc.c b/src/malloc/malloc.c
index b56fdaa2..d4c229bd 100644
--- a/src/malloc/malloc.c
+++ b/src/malloc/malloc.c
@@ -8,6 +8,7 @@
 #include "libc.h"
 #include "atomic.h"
 #include "pthread_impl.h"
+#include VG_MEMCHECK_H
 
 #if defined(__GNUC__) && defined(__PIC__)
 #define inline inline __attribute__((always_inline))
@@ -373,6 +374,9 @@ static size_t mal0_clear(char *p, size_t pagesz, size_t n)
 	typedef unsigned long long T;
 	char *pp = p + n;
 	size_t i = (uintptr_t)pp & (pagesz - 1);
+#ifdef VALGRIND_MAKE_MEM_DEFINED_IF_ADDRESSABLE
+	VALGRIND_MAKE_MEM_DEFINED_IF_ADDRESSABLE(p, n);
+#endif
 	for (;;) {
 		pp = memset(pp - i, 0, i);
 		if (pp - p < pagesz) return pp - p;
-- 
2.11.0

Re: [RFC PATCH] Allow annotating calloc for Valgrind

[Rich Felker]

On Fri, Jun 30, 2017 at 01:56:14AM +0300, Alexander Monakov wrote:
> With mal0_clear entered only for larger-than-one-page allocations, a
> few additional nops and stack accesses don't matter.
> 
> This only needs Valgrind headers to build, Valgrind itself doesn't need to be
> built/installed.  However, valgrind.h unconditionally includes stdarg.h.

Use of valgrind annotation was already rejected a long time ago. The
same can be done with a suppressions file and that's where it belongs.

> Untested.
> ---
>  configure           | 11 +++++++++++
>  src/malloc/malloc.c |  4 ++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/configure b/configure
> index c2db298c..7e78094f 100755
> --- a/configure
> +++ b/configure
> @@ -30,6 +30,7 @@ System types:
>  Optional features:
>    --enable-optimize=...   optimize listed components for speed over size [auto]
>    --enable-debug          build with debugging information [disabled]
> +  --enable-valgrind       add Valgrind annotations [disabled]
>    --enable-warnings       build with recommended warnings flags [disabled]
>    --enable-visibility     use global visibility options to optimize PIC [auto]
>    --enable-wrapper=...    build given musl toolchain wrapper [auto]
> @@ -134,6 +135,7 @@ build=
>  target=
>  optimize=auto
>  debug=no
> +valgrind=no
>  warnings=no
>  visibility=auto
>  shared=auto
> @@ -161,6 +163,8 @@ case "$arg" in
>  --disable-optimize) optimize=no ;;
>  --enable-debug|--enable-debug=yes) debug=yes ;;
>  --disable-debug|--enable-debug=no) debug=no ;;
> +--enable-valgrind|--enable-valgrind=yes) valgrind=yes ;;
> +--disable-valgrind|--enable-valgrind=no) valgrind=no ;;
>  --enable-warnings|--enable-warnings=yes) warnings=yes ;;
>  --disable-warnings|--enable-warnings=no) warnings=no ;;
>  --enable-visibility|--enable-visibility=yes) visibility=yes ;;
> @@ -390,6 +394,13 @@ tryflag CFLAGS_MEMOPS -fno-tree-loop-distribute-patterns
>  #
>  test "$debug" = yes && CFLAGS_AUTO=-g
>  
> +if test x"$valgrind" = xyes  ; then
> +CPPFLAGS="$CPPFLAGS -DVG_MEMCHECK_H='<memcheck.h>'"
> +else
> +CPPFLAGS="$CPPFLAGS -DVG_MEMCHECK_H='</dev/null>'"
> +fi
> +CPPFLAGS="${CPPFLAGS# }"
> +
>  #
>  # Preprocess asm files to add extra debugging information if debug is
>  # enabled, our assembler supports the needed directives, and the
> diff --git a/src/malloc/malloc.c b/src/malloc/malloc.c
> index b56fdaa2..d4c229bd 100644
> --- a/src/malloc/malloc.c
> +++ b/src/malloc/malloc.c
> @@ -8,6 +8,7 @@
>  #include "libc.h"
>  #include "atomic.h"
>  #include "pthread_impl.h"
> +#include VG_MEMCHECK_H

This looks really flaky, and makes compilation dependent on a
POSIX-conforming filesystem. For example it almost certainly breaks
cross-compiling from a mingw-based cross compiler (which
musl-cross-make supports building).

The same could be achieved without hacks just using #if/#ifdef around
the #include. But I don't see how <memcheck.h> could work anyway; musl
obviously does not, and can't, use any host include paths where a
header might already be installed.

Rich

Re: [RFC PATCH] Allow annotating calloc for Valgrind

[Alexander Monakov]

On Thu, 29 Jun 2017, Rich Felker wrote:
> Use of valgrind annotation was already rejected a long time ago.

I don't see any record of that in the archives...

> The same can be done with a suppressions file and that's where it belongs.

What would you write in the suppression file? If you tell Valgrind to ignore
branch-on-uninit in calloc, it will report errors later on anyway when the
application reads from the calloc'ed region. Szabolcs went down that road once.

> The same could be achieved without hacks just using #if/#ifdef around
> the #include. But I don't see how <memcheck.h> could work anyway; musl
> obviously does not, and can't, use any host include paths where a
> header might already be installed.

With an appropriate -I in CPPFLAGS?

Alexander

Re: [RFC PATCH] Allow annotating calloc for Valgrind

[Rich Felker]

On Fri, Jun 30, 2017 at 02:42:53AM +0300, Alexander Monakov wrote:
> On Thu, 29 Jun 2017, Rich Felker wrote:
> > Use of valgrind annotation was already rejected a long time ago.
> 
> I don't see any record of that in the archives...

It reached the point of an faq item on irc; perhaps it was never
discussed on the ml.

> > The same can be done with a suppressions file and that's where it belongs.
> 
> What would you write in the suppression file? If you tell Valgrind to ignore
> branch-on-uninit in calloc, it will report errors later on anyway when the
> application reads from the calloc'ed region. Szabolcs went down that road once.

If that happens, it's just a valgrind bug. It can see the memory was
returned by calloc and therefore the contents are defined. But maybe
before discussing this further we need to clarify what the actual
scenario is. I thought normally valgrind does some trick to hook in
its own malloc implementation. Are there different ways of invoking it
where the system malloc gets used and instrumented? I've never been
able to get authoritative answers on anything related to this, so all
of the arguments have stumbled around whatever the person arguing
_claims_ valgrind does or needs to do...

> > The same could be achieved without hacks just using #if/#ifdef around
> > the #include. But I don't see how <memcheck.h> could work anyway; musl
> > obviously does not, and can't, use any host include paths where a
> > header might already be installed.
> 
> With an appropriate -I in CPPFLAGS?

Yes, that would presumably work, but I can imagine users making a huge
mess by putting some dir full of host headers (not just valgrind ones)
in the -I...

Rich

Re: [RFC PATCH] Allow annotating calloc for Valgrind

[Alexander Monakov]

On Thu, 29 Jun 2017, Rich Felker wrote:
> It reached the point of an faq item on irc; perhaps it was never
> discussed on the ml.

I feel a quick reminder of what was discussed would be very nice.

> If that happens, it's just a valgrind bug. It can see the memory was
> returned by calloc and therefore the contents are defined. But maybe
> before discussing this further we need to clarify what the actual
> scenario is.

Alright. As far as I can tell, everyone hits this (only) with static
linking. Valgrind core *does* have a concept of a symbol table being
distinct from dynamic symbol table (info from symtab is successfully
used for backtracing for example), so in principle Memcheck could use
just the .symtab when running unstripped static executables.

Unfortunately, historically the implementation of Memcheck relies
entirely on dynamic linking to intercept allocation functions. As a
result, Memcheck's functionality on static executables degrades
significantly (it can still find a subset of uninit access errors).

I guess the proper fix - wiring up .symtab-based interception - might
require more time than anyone was prepared to volunteer.

I think at the moment client requests are the only straightforward way
to use Memcheck fully with statically-linked programs.  But for full
functionality, people would need extra requests informing Memcheck
about the effects of static malloc & free. Where would such a patch
belong?

Alexander

Re: [RFC PATCH] Allow annotating calloc for Valgrind

[Rich Felker]

On Sun, Jul 02, 2017 at 04:55:08PM +0300, Alexander Monakov wrote:
> On Thu, 29 Jun 2017, Rich Felker wrote:
> > It reached the point of an faq item on irc; perhaps it was never
> > discussed on the ml.
> 
> I feel a quick reminder of what was discussed would be very nice.

I'm not sure I can do the topic justice without digging through a lot
of old logs. I don't want to misrepresent what anyone said.

> > If that happens, it's just a valgrind bug. It can see the memory was
> > returned by calloc and therefore the contents are defined. But maybe
> > before discussing this further we need to clarify what the actual
> > scenario is.
> 
> Alright. As far as I can tell, everyone hits this (only) with static
> linking. Valgrind core *does* have a concept of a symbol table being
> distinct from dynamic symbol table (info from symtab is successfully
> used for backtracing for example), so in principle Memcheck could use
> just the .symtab when running unstripped static executables.
> 
> Unfortunately, historically the implementation of Memcheck relies
> entirely on dynamic linking to intercept allocation functions. As a
> result, Memcheck's functionality on static executables degrades
> significantly (it can still find a subset of uninit access errors).
> 
> I guess the proper fix - wiring up .symtab-based interception - might
> require more time than anyone was prepared to volunteer.
> 
> I think at the moment client requests are the only straightforward way
> to use Memcheck fully with statically-linked programs.  But for full
> functionality, people would need extra requests informing Memcheck
> about the effects of static malloc & free. Where would such a patch
> belong?

I'm not sure it makes sense to do -- is there a good reason dynamic
linking can't be used when debugging memory errors? Surely some apps
(especially proprietary ones) might be shipped as static binaries, but
these will likely lack debugging symbols anyway.

There are also fundamental limits to the correctness of any approach
that uses static linking, since too much information has already been
lost. It's calling the _name_ malloc, realloc, or free (not the code
at the location; think aliases etc.) that must have the allocation
semantics. Even if nothing weird is happening with aliases at the libc
implementation level, the compiler could do major transformations with
IPA (especially with LTO) that end up resulting in code being shared
in unexpected ways.

Rich

Re: [RFC PATCH] Allow annotating calloc for Valgrind

[Alexander Monakov]

On Sun, 2 Jul 2017, Rich Felker wrote:
> I'm not sure it makes sense to do -- is there a good reason dynamic
> linking can't be used when debugging memory errors? Surely some apps
> (especially proprietary ones) might be shipped as static binaries, but
> these will likely lack debugging symbols anyway.

Perhaps the project is hard to rebuild with shared libc.so, for reasons
like using linker functionality (e.g. --wrap, linker scripts) that does
not have direct equivalents outside of fully-static linking.

But in any case, even if there are doubts as to *why* people do it, we
know for certain that people hit this issue - there were two independent
reports on the mailing list this month. It would be nice to come up with
some kind of "canonical answer" for those situations - is it going to be
"just don't use static linking"?

> There are also fundamental limits to the correctness of any approach
> that uses static linking, since too much information has already been
> lost. It's calling the _name_ malloc, realloc, or free (not the code
> at the location; think aliases etc.) that must have the allocation
> semantics. Even if nothing weird is happening with aliases at the libc
> implementation level, the compiler could do major transformations with
> IPA (especially with LTO) that end up resulting in code being shared
> in unexpected ways.

Are you sure the same theory doesn't apply with shared libc.so? When you
call malloc internally in libc.so (e.g. printf->...->realloc), you're not
calling it via a dynamic relocation.

Alexander

Re: [RFC PATCH] Allow annotating calloc for Valgrind

[Rich Felker]

On Sun, Jul 02, 2017 at 07:16:15PM +0300, Alexander Monakov wrote:
> On Sun, 2 Jul 2017, Rich Felker wrote:
> > I'm not sure it makes sense to do -- is there a good reason dynamic
> > linking can't be used when debugging memory errors? Surely some apps
> > (especially proprietary ones) might be shipped as static binaries, but
> > these will likely lack debugging symbols anyway.
> 
> Perhaps the project is hard to rebuild with shared libc.so, for reasons
> like using linker functionality (e.g. --wrap, linker scripts) that does
> not have direct equivalents outside of fully-static linking.
> 
> But in any case, even if there are doubts as to *why* people do it, we
> know for certain that people hit this issue - there were two independent
> reports on the mailing list this month. It would be nice to come up with
> some kind of "canonical answer" for those situations - is it going to be
> "just don't use static linking"?

I think "debugging tool X has inherent limitations with static
linking, so you should dynamic-link to use it" is a fairly reasonable
position to take. Switching to dynamic-linking everything may be
complex or difficult for some larger projects, but just dynamically
linking libc.so and static linking everything else should not be hard.

> > There are also fundamental limits to the correctness of any approach
> > that uses static linking, since too much information has already been
> > lost. It's calling the _name_ malloc, realloc, or free (not the code
> > at the location; think aliases etc.) that must have the allocation
> > semantics. Even if nothing weird is happening with aliases at the libc
> > implementation level, the compiler could do major transformations with
> > IPA (especially with LTO) that end up resulting in code being shared
> > in unexpected ways.
> 
> Are you sure the same theory doesn't apply with shared libc.so? When you
> call malloc internally in libc.so (e.g. printf->...->realloc), you're not
> calling it via a dynamic relocation.

Well printf doesn't call realloc, but some things will, and yes I
think you're right. This problem might go away if we made an effort to
actually support malloc interposition (with a well-defined model for
what you can and can't do rather than just being sloppy about it)
since libc-internal callers would have to go through the same
interfaces, but there's been some question whether, if we did this,
purely libc-internal (i.e. not "as if by malloc") allocations would be
subject to interposition or would always use an internal malloc. Maybe
it doesn't matter one way or the other if we assume there are not bugs
in the purely libc-internal use of the allocator.

Rich