Re: Issue with musl and valgrind

[Rich Felker <dalias@libc.org>]

On Mon, Jan 06, 2020 at 12:01:34PM +0100, d.dorau@avm.de wrote:
> Hello and a happy new year!
> 
> I'm looking for some help/advice regarding the use of valgrind with musl.
> In my attempts to use valgrind with musl I ran into a reproducible crash. 
> I found others
> experienced this earlier as well, but with no apparent solution:
> 
> https://forum.archive.openwrt.org/viewtopic.php?id=70169
> 
> ==3915== Invalid read of size 4
> ==3915==    at 0x48C3154: free (in 
> /usr/lib/valgrind/vgpreload_memcheck-mips32-linux.so)
> ==3915==    by 0x4088C04: ??? (in /lib/libc.so)
> ==3915==  Address  is not stack'd, malloc'd or (recently) free'd
> 
> I did some debugging and came to a plausible reason, and with a quick
> hack I could get it to run.
> 
> My findings are that when musl loads vgpreload_memcheck-mips32-linux.so,
> malloc/free are replaced by the implementation provided from valgrind, as 
> expected.
> 
> The free() call that leads to the crash originates from musl's 
> dynlink.c:782 in
> map_library().
> 
> The crash in valgrind's vgpreload_memcheck-mips32-linux.so is located in 
> coregrind/m_replacemalloc/vg_replace_malloc.c:184 as part of the macro 
> DO_INIT
> 
> 
> static int init_done;
> #define DO_INIT if (UNLIKELY(!init_done)) init()
> [...]
> #define FREE(soname, fnname, vg_replacement) \
>    \
>    void VG_REPLACE_FUNCTION_EZU(10050,soname,fnname) (void *p); \
>    void VG_REPLACE_FUNCTION_EZU(10050,soname,fnname) (void *p)  \
>    { \
>       DO_INIT; \
>       MALLOC_TRACE(#fnname "(%p)\n", p ); \
>       if (p == NULL)  \
>          return; \
>       (void)VALGRIND_NON_SIMD_CALL1( info.tl_##vg_replacement, p ); \
>    }
> 
> that is called in the free function provided by valgrind.
> 
> Calculating back the crash address 0x1e0d8 from the example above leads to 
> the
> address of the variable "init_done" of the valgrind code in the ELF file 
> itself!
> 
> My interpretation is that although the relocation of the new free function 
> has
> apprently been done, the variables (and maybe other called functions) used 
> *inside*
> the valgrind library have not been relocated yet, so that their addresses 
> still
> point to the bare address in the ELF file. (I hope the explanation makes 
> sense.)
> 
> I could avoid the crash by patching musl so that dynlink.c always calls 
> musl's
> own malloc/free function (by renaming and an additional wrapper for the 
> exported
> symbols).
> 
> Since I don't have in-depth knowledge about the inner workings of musl's 
> shared
> library loader, I'm now looking for advice on how to solve this issue.
> 
> At first glance the fact that functions inside the loaded library are 
> called before
> all relocations are done looks problematic to me. 
> Assuming this would be solved, wouldn't it be desirable to ensure that 
> malloc/free
> calls inside musl won't be replaced by external libraries so that we don't 
> get
> stale allocations left from the old allocator?

This is really a bug in valgrind, that it's relying on a mix of the
dynamic linker and its own mechanism for interposing malloc. Either
the interposition should not take place until after line 1913 of
__dls3 in dynlink.c (this is what would happen if you did it via
LD_PRELOAD without valgrind), or valgrind should do its own loading
and relocation of vgpreload_memcheck-*.so independent of the tracee's
dynamic linker.

On most archs, the problem does not manifest as things are setup so
that it's safe to call free in an unrelocated context. However
realloc is gratuitously not safe and Adélie Linux has a patch for it
to do it like free:

https://code.foxkit.us/adelie/packages/blob/master/user/valgrind/realloc.patch

But on MIPS, where there are no PC-relative references and everything
goes through the GOT, this blows up.

I'm not sure how practical it is to get valgrind to fix this upstream.
In the either/or above, if the first course of action is taken, the
mechanism should probably be by inserting a breakpoint (or valgrind's
equivalent) at the main program's e_entry address and not doing the
replacement hack until then. Alternatively, it could just rely on
LD_PRELOAD working without trying to do additional tricks, but maybe
they have a good reason they're not doing that.

Rich