----- Original Message ----- > From: "Stefan Seefeld" <[email protected]> > To: "Mathieu Desnoyers" <[email protected]> > Cc: [email protected] > Sent: Saturday, February 8, 2014 11:53:44 AM > Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > > On 02/08/2014 11:06 AM, Mathieu Desnoyers wrote: > > > > > > ----- Original Message ----- > >> From: "Stefan Seefeld" <[email protected]> > >> To: [email protected] > >> Sent: Friday, February 7, 2014 4:50:09 PM > >> Subject: [lttng-dev] RFC: Fix crash in dlerror() > >> > >> I have been looking into an issue with the malloc wrapper, where an > >> unsuccessful call to dlopen(), followed by a call to dlerror(), would > >> result in a segmentation fault when the malloc wrapper is being used. > >> > >> The problem is the following: > >> > >> The functions dlopen() and dlsym() make use of a global (though > >> thread-local) "result" structure to hold the error state, to allow a > >> subsequent call to dlerror() to report it. > >> > >> As it turns out, dlerror() itself may implicitly call realloc(), which, > >> if it hasn't been used before, triggers our wrapper to call dlsym(). So, > >> while dlerror() inspects said result structure, dlsym() re-initializes > >> it, causing the crash... > >> > >> This is arguably a bug in the dlfcn functions. The attached patch > >> attempts to fix this by moving the initialization of the realloc() > >> wrapper (i.e., the loading of the symbol) into the constructor. This > >> fixes the crash that I'm observing, but since none of these dependencies > >> are specified or documented, this change may cause other issues elsewhere. > >> > >> Are there any objections to this approach ? If not, I'll submit a formal > >> patch for this. > > > > The issue I see with the approach you propose is if dlsym() is used > > within another constructor (from anoter lib for instance). Given that > > the order of constructor execution should be assumed to be random, we can > > run into the same error scenario you describe here. > > Yes, I was worried about the same. In general there really isn't > anything we can do short of submitting a patch to glibc, as the only way > to address this correctly is to record whether we are inside a dlerror() > call. So, anything I can think of is merely heuristic. > > Here is a slightly different approach: We know that the calloc symbol is > looked up very early (see the trick we use with the temporary static > calloc function to avoid the infinite recursion), and we may safely > assume that the calloc lookup won't fail (if it does, the user has more > important things to worry about). Thus, right after the calloc lookup > completed seems a good time to force the realloc lookup. > > That's what this follow-up patch does (which still works for my test). > > How does that look ?
Interesting approach. Then I wonder if we couldn't simply lookup every symbol we're interested in whenever any of the overridden function is called and we notice a NULL pointer, and provide a simplistic "static" allocator for every function overridden. Please let me know if something like the attached patch works for you. It seems more solid to cover all the cases, so that if the dlsym() implementation ever chooses to use other functions from the memory allocator, it will still work. Thoughts ? Thanks, Mathieu > > Stefan > > -- > Stefan Seefeld > CodeSourcery / Mentor Graphics > http://www.mentor.com/embedded-software/ > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
commit c933316e0c5f45d7c9a49cc75926be3f5c419f10 Author: Mathieu Desnoyers <[email protected]> Date: Sat Feb 8 17:12:16 2014 -0500 Fix: malloc libc instrumentation wrapper calloc and realloc wrt dlsym and dlerror can trigger segmentation faults. Ensure that we fully populate the allocator symbols all at once, and also ensure that we use the static allocator while doing the dlsym lookups. Signed-off-by: Mathieu Desnoyers <[email protected]> diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c index 33ed18b..4db9bf0 100644 --- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c +++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c @@ -21,8 +21,11 @@ #include <dlfcn.h> #include <sys/types.h> #include <stdio.h> +#include <assert.h> #include <urcu/system.h> #include <urcu/uatomic.h> +#include <urcu/compiler.h> +#include <lttng/align.h> #define TRACEPOINT_DEFINE #define TRACEPOINT_CREATE_PROBES @@ -32,10 +35,30 @@ static char static_calloc_buf[STATIC_CALLOC_LEN]; static unsigned long static_calloc_buf_offset; -static void *static_calloc(size_t nmemb, size_t size) +struct alloc_functions { + void *(*calloc)(size_t nmemb, size_t size); + void *(*malloc)(size_t size); + void (*free)(void *ptr); + void *(*realloc)(void *ptr, size_t size); + void *(*memalign)(size_t alignment, size_t size); + int (*posix_memalign)(void **memptr, size_t alignment, size_t size); +}; + +static +struct alloc_functions cur_alloc; + +/* + * Static allocator to use when initially executing dlsym(). + */ +static +void *static_calloc_aligned(size_t nmemb, size_t size, size_t alignment) { unsigned long prev_offset, new_offset, res_offset; + if (nmemb * size == 0) { + return NULL; + } + /* * Protect static_calloc_buf_offset from concurrent updates * using a cmpxchg loop rather than a mutex to remove a @@ -45,125 +68,205 @@ static void *static_calloc(size_t nmemb, size_t size) res_offset = CMM_LOAD_SHARED(static_calloc_buf_offset); do { prev_offset = res_offset; - if (nmemb * size > sizeof(static_calloc_buf) - prev_offset) { + new_offset = ALIGN(prev_offset, alignment) + nmemb * size; + if (new_offset > sizeof(static_calloc_buf)) { return NULL; } - new_offset = prev_offset + nmemb * size; } while ((res_offset = uatomic_cmpxchg(&static_calloc_buf_offset, prev_offset, new_offset)) != prev_offset); return &static_calloc_buf[prev_offset]; } +static +void *static_calloc(size_t nmemb, size_t size) +{ + return static_calloc_aligned(nmemb, size, 1); +} + +static +void *static_malloc(size_t size) +{ + return static_calloc_aligned(1, size, 1); +} + +static +void static_free(void *ptr) +{ + /* no-op. */ +} + +static +void *static_realloc(void *ptr, size_t size) +{ + /* Don't free previous memory location */ + return static_calloc_aligned(1, size, 1); +} + +static +void *static_memalign(size_t alignment, size_t size) +{ + return static_calloc_aligned(1, size, alignment); +} + +static +int static_posix_memalign(void **memptr, size_t alignment, size_t size) +{ + void *ptr; + + /* Check for power of 2 */ + if (alignment & (alignment - 1)) { + return EINVAL; + } + if (alignment < sizeof(void *)) { + return EINVAL; + } + ptr = static_calloc_aligned(1, size, alignment); + if (size && !ptr) { + return ENOMEM; + } + *memptr = ptr; + return 0; +} + +static +void setup_static_allocator(void) +{ + assert(cur_alloc.calloc == NULL); + cur_alloc.calloc = static_calloc; + assert(cur_alloc.malloc == NULL); + cur_alloc.malloc = static_malloc; + assert(cur_alloc.free == NULL); + cur_alloc.free = static_free; + assert(cur_alloc.realloc == NULL); + cur_alloc.realloc = static_realloc; + assert(cur_alloc.memalign == NULL); + cur_alloc.memalign = static_memalign; + assert(cur_alloc.posix_memalign == NULL); + cur_alloc.posix_memalign = static_posix_memalign; +} + +static +void lookup_all_symbols(void) +{ + struct alloc_functions af; + + /* + * Temporarily redirect allocation functions to + * static_calloc_aligned, and free function to static_free + * (no-op), until the dlsym lookup has completed. + */ + setup_static_allocator(); + + /* Perform the actual lookups */ + af.calloc = dlsym(RTLD_NEXT, "calloc"); + af.malloc = dlsym(RTLD_NEXT, "malloc"); + af.free = dlsym(RTLD_NEXT, "free"); + af.realloc = dlsym(RTLD_NEXT, "realloc"); + af.memalign = dlsym(RTLD_NEXT, "memalign"); + af.posix_memalign = dlsym(RTLD_NEXT, "posix_memalign"); + + /* Populate the new allocator functions */ + memcpy(&cur_alloc, &af, sizeof(cur_alloc)); +} + void *malloc(size_t size) { - static void *(*plibc_malloc)(size_t size); void *retval; - if (plibc_malloc == NULL) { - plibc_malloc = dlsym(RTLD_NEXT, "malloc"); - if (plibc_malloc == NULL) { + if (cur_alloc.malloc == NULL) { + lookup_all_symbols(); + if (cur_alloc.malloc == NULL) { fprintf(stderr, "mallocwrap: unable to find malloc\n"); - return NULL; + abort(); } } - retval = plibc_malloc(size); + retval = cur_alloc.malloc(size); tracepoint(ust_libc, malloc, size, retval); return retval; } void free(void *ptr) { - static void (*plibc_free)(void *ptr); - /* Check whether the memory was allocated with - * static_calloc, in which case there is nothing + * static_calloc_align, in which case there is nothing * to free. */ - if ((char *)ptr >= static_calloc_buf && - (char *)ptr < static_calloc_buf + STATIC_CALLOC_LEN) { + if (caa_unlikely((char *)ptr >= static_calloc_buf && + (char *)ptr < static_calloc_buf + STATIC_CALLOC_LEN)) { return; } - if (plibc_free == NULL) { - plibc_free = dlsym(RTLD_NEXT, "free"); - if (plibc_free == NULL) { + if (cur_alloc.free == NULL) { + lookup_all_symbols(); + if (cur_alloc.free == NULL) { fprintf(stderr, "mallocwrap: unable to find free\n"); - return; + abort(); } } tracepoint(ust_libc, free, ptr); - plibc_free(ptr); + cur_alloc.free(ptr); } void *calloc(size_t nmemb, size_t size) { - static void *(*volatile plibc_calloc)(size_t nmemb, size_t size); void *retval; - if (plibc_calloc == NULL) { - /* - * Temporarily redirect to static_calloc, - * until the dlsym lookup has completed. - */ - plibc_calloc = static_calloc; - plibc_calloc = dlsym(RTLD_NEXT, "calloc"); - if (plibc_calloc == NULL) { + if (cur_alloc.calloc == NULL) { + lookup_all_symbols(); + if (cur_alloc.calloc == NULL) { fprintf(stderr, "callocwrap: unable to find calloc\n"); - return NULL; + abort(); } } - retval = plibc_calloc(nmemb, size); + retval = cur_alloc.calloc(nmemb, size); tracepoint(ust_libc, calloc, nmemb, size, retval); return retval; } void *realloc(void *ptr, size_t size) { - static void *(*plibc_realloc)(void *ptr, size_t size); void *retval; - if (plibc_realloc == NULL) { - plibc_realloc = dlsym(RTLD_NEXT, "realloc"); - if (plibc_realloc == NULL) { + if (cur_alloc.realloc == NULL) { + lookup_all_symbols(); + if (cur_alloc.realloc == NULL) { fprintf(stderr, "reallocwrap: unable to find realloc\n"); - return NULL; + abort(); } } - retval = plibc_realloc(ptr, size); + retval = cur_alloc.realloc(ptr, size); tracepoint(ust_libc, realloc, ptr, size, retval); return retval; } void *memalign(size_t alignment, size_t size) { - static void *(*plibc_memalign)(size_t alignment, size_t size); void *retval; - if (plibc_memalign == NULL) { - plibc_memalign = dlsym(RTLD_NEXT, "memalign"); - if (plibc_memalign == NULL) { + if (cur_alloc.memalign == NULL) { + lookup_all_symbols(); + if (cur_alloc.memalign == NULL) { fprintf(stderr, "memalignwrap: unable to find memalign\n"); - return NULL; + abort(); } } - retval = plibc_memalign(alignment, size); + retval = cur_alloc.memalign(alignment, size); tracepoint(ust_libc, memalign, alignment, size, retval); return retval; } int posix_memalign(void **memptr, size_t alignment, size_t size) { - static int(*plibc_posix_memalign)(void **memptr, size_t alignment, size_t size); int retval; - if (plibc_posix_memalign == NULL) { - plibc_posix_memalign = dlsym(RTLD_NEXT, "posix_memalign"); - if (plibc_posix_memalign == NULL) { + if (cur_alloc.posix_memalign == NULL) { + lookup_all_symbols(); + if (cur_alloc.posix_memalign == NULL) { fprintf(stderr, "posix_memalignwrap: unable to find posix_memalign\n"); - return ENOMEM; + abort(); } } - retval = plibc_posix_memalign(memptr, alignment, size); + retval = cur_alloc.posix_memalign(memptr, alignment, size); tracepoint(ust_libc, posix_memalign, *memptr, alignment, size, retval); return retval; }
_______________________________________________ lttng-dev mailing list [email protected] http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
