Re: dynlink.c: bug in reclaim_gaps leading to segfault in __libc_exit_fini

[Szabolcs Nagy <nsz@port70.net>]

* Timo Teras <timo.teras@iki.fi> [2016-02-17 09:03:27 +0200]:
> On Tue, 16 Feb 2016 22:55:50 +0100
> Szabolcs Nagy <nsz@port70.net> wrote:
> 
> > * Hugues Bruant <hugues@aerofs.com> [2016-02-16 16:30:42 -0500]:
> > > Affects both 1.1.12 and 1.1.13
> > > 
> > > Tracked down with valgrind in Alpine Linux 3.3.
> > > 
> > > The dmg tool build from https://github.com/aerofs/libdmg-hfsplus
> > > links to a handful shared libs. The following message is seen
> > > immediately at start:
> > > 
> > > ==59== Invalid free() / delete / delete[] / realloc()
> > > ==59==    at 0x4C92B0E: free (vg_replace_malloc.c:530)
> > > ==59==    by 0x4056F68: reclaim_gaps (dynlink.c:488)
> > > ==59==    by 0x405743D: map_library (dynlink.c:708)
> > > ==59==    by 0x4057EF3: load_library (dynlink.c:1014)
> > > ==59==    by 0x4058CA8: load_preload (dynlink.c:1112)
> > > ==59==    by 0x4058CA8: __dls3 (dynlink.c:1581)
> > > ==59==    by 0x405856A: __dls2 (dynlink.c:1383)
> > > ==59==    by 0x405655E: ??? (in /lib/ld-musl-x86_64.so.1)
> > > ==59==    by 0x3: ???
> > > ==59==    by 0xFFF000E3A: ???
> > > ==59==    by 0xFFF000E3E: ???
> > > ==59==    by 0xFFF000E44: ???
> > > ==59==    by 0xFFF000E86: ???
> > > 
> > > Afterwards, the program proceeds with no issue, until it exists, at
> > > which point a segfault is triggered when cleaning up shared
> > > libraries: 
> > 
> > this is not a bug.
> 
> It is compliance issue. POSIX says about free:
> --
> The free() function shall cause the space pointed to by ptr to be
> deallocated; that is, made available for further allocation. If ptr is
> a null pointer, no action shall occur. Otherwise, if the argument does
> not match a pointer earlier returned by a function in POSIX.1-2008 that
> allocates memory as if by malloc(), or if the space has been
> deallocated by a call to free() or realloc(), the behavior is undefined.
> --
> 
> While overloading allocators are not supported, they'd break at this
> too. And it'll be highly annoying if someone decides to test a new
> memory allocator inside musl and does not know about this one exception.
> 

interception of malloc/free/... is supported, just not
calls inside the libc (so you can intercept allocations
that happen in user code, but libc internal calls will
keep using libc allocators).

libc internal calls are unobservable on the language
level, so i don't think there is a conformance issue.

valgrind does not use elf preloading to intercept free,
but emulates the instructions, one basic block at a time,
and if it sees a call to 'free' according to its symbol
tables, then it redirects the call to its own free
(which happens to sit in a preloaded module so it is in
'valgrind client space' and runs on the simulated cpu).

note that valgrind does the redirection based on the
called address and not using GOT entries, so it discards
symbol visibility rules.

in principle this could arbitrarily break, but in
practice it happens to work.. except reclaim gaps
in case of musl. (there are a large amount of hacks
in valgrind to make it not spew out errors on glibc
ld.so and early startup code, musl only needs this
one hack.)

> > valgrind is not aware of dynamic linker internals,
> > you have to use a musl specific suppression file
> > to hide this message (but i dont know if anybody
> > wrote such thing for valgrind).
> 
> Well - musl really should introduce __donatemem or similar for this
> purpose, and not overload the standard free() function. This would make
> the valgrind warning go away.
> 
> I'd rather not write a suppression for the above, since the internals
> are misusing/overloading a standard api call against posix.
> 

i would not call it 'overloading the standard free',
this is a libc internal call.

using a different symbol only helps if it has different
address as well (an alias would be still redirected).

to please valgrind the options are

1) have an internal free which valgrind does not know
   about, but public free calls it, so all public calls
   of free would go through an extra indirection.

2) have a copy of the internal logic of free under a
   different name, which means maintenance work and
   code size increase.

3) or have a suppression file.

i think 3) is a reasonable solution.

> Technically valgrind is detecting a valid case for misuse of free().
> While in context of standard musl allocator it's ok.
> 
> Thanks,
> Timo