Created attachment 11750 [details] sample reproducer glibc 2.28 includes [0], which causes __libc_freeres to free some memory from libdl. One of the functions it calls is __dlerror_main_freeres[1], which frees the current thread's dlerror state. This does not work right if init[2] here was never run. In that case, __dlerror_main_freeres ends up freeing whatever is at thread local key zero, which may have been allocated by some other application and may not be cleaned up by free(). I've attached a sample program which demonstrates the problem when run in valgrind. See the comment at the top for instructions and notes. [0] https://sourceware.org/git/?p=glibc.git;a=commit;h=2827ab990aefbb0e53374199b875d98f116d6390 [1] https://sourceware.org/git/?p=glibc.git;a=blob;f=dlfcn/dlerror.c;h=33574faab65628ff85c358677e567837390efe7b;hb=3c03baca37fdcb52c3881e653ca392bba7a99c2b#l226 [2] https://sourceware.org/git/?p=glibc.git;a=blob;f=dlfcn/dlerror.c;h=33574faab65628ff85c358677e567837390efe7b;hb=3c03baca37fdcb52c3881e653ca392bba7a99c2b#l173
I'm looking into this to see what went wrong and how I might fix it. Thanks for the report.
Proposed fix. Only free the memory if __libc_once (once, init) has been called. diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c index 2737658..41a41ee 100644 --- a/dlfcn/dlerror.c +++ b/dlfcn/dlerror.c @@ -230,13 +230,16 @@ free_key_mem (void *mem) void __dlerror_main_freeres (void) { - void *mem; - /* Free the global memory if used. */ - check_free (&last_result); - /* Free the TSD memory if used. */ - mem = __libc_getspecific (key); - if (mem != NULL) - free_key_mem (mem); + if (__libc_once_get (once)) + { + void *mem; + /* Free the global memory if used. */ + check_free (&last_result); + /* Free the TSD memory if used. */ + mem = __libc_getspecific (key); + if (mem != NULL) + free_key_mem (mem); + } } struct dlfcn_hook *_dlfcn_hook __attribute__((nocommon));
(In reply to Mark Wielaard from comment #2) > Proposed fix. Only free the memory if __libc_once (once, init) has been > called. > > diff --git a/dlfcn/dlerror.c b/dlfcn/dlerror.c > index 2737658..41a41ee 100644 > --- a/dlfcn/dlerror.c > +++ b/dlfcn/dlerror.c > @@ -230,13 +230,16 @@ free_key_mem (void *mem) > void > __dlerror_main_freeres (void) > { > - void *mem; > - /* Free the global memory if used. */ > - check_free (&last_result); > - /* Free the TSD memory if used. */ > - mem = __libc_getspecific (key); > - if (mem != NULL) > - free_key_mem (mem); > + if (__libc_once_get (once)) > + { > + void *mem; > + /* Free the global memory if used. */ > + check_free (&last_result); > + /* Free the TSD memory if used. */ > + mem = __libc_getspecific (key); > + if (mem != NULL) > + free_key_mem (mem); > + } > } > > struct dlfcn_hook *_dlfcn_hook __attribute__((nocommon)); I reviewed this on libc-alpha. It isn't quite right, and I noticed a similar failure if the key fails to be created. I have suggested some changes for Mark to look at. I think we're almost done though.
Updated patch: https://sourceware.org/ml/libc-alpha/2019-05/msg00320.html
commit 11b451c8868d8a2b0edc5dfd44fc58d9ee538be0 Author: Mark Wielaard <mark@klomp.org> Date: Wed May 15 17:14:01 2019 +0200 dlfcn: Guard __dlerror_main_freeres with __libc_once_get (once) [BZ# 24476] dlerror.c (__dlerror_main_freeres) will try to free resources which only have been initialized when init () has been called. That function is called when resources are needed using __libc_once (once, init) where once is a __libc_once_define (static, once) in the dlerror.c file. Trying to free those resources if init () hasn't been called will produce errors under valgrind memcheck. So guard the freeing of those resources using __libc_once_get (once) and make sure we have a valid key. Also add a similar guard to __dlerror (). * dlfcn/dlerror.c (__dlerror_main_freeres): Guard using __libc_once_get (once) and static_bug == NULL. (__dlerror): Check we have a valid key, set result to static_buf otherwise. Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Also backported/cherry-picked to release/2.28/master and release/2.29/master.