* Re: RFC: Fix crash in dlerror() [not found] <52F55511.2080309@mentor.com> @ 2014-02-08 16:06 ` Mathieu Desnoyers [not found] ` <1380151240.21051.1391875592403.JavaMail.zimbra@efficios.com> [not found] ` <52FCAA28.6020708@mentor.com> 2 siblings, 0 replies; 28+ messages in thread From: Mathieu Desnoyers @ 2014-02-08 16:06 UTC (permalink / raw) To: Stefan Seefeld; +Cc: lttng-dev ----- Original Message ----- > From: "Stefan Seefeld" <stefan_seefeld@mentor.com> > To: lttng-dev@lists.lttng.org > 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. Thoughts ? Thanks, Mathieu > > Thanks, > Stefan > > -- > Stefan Seefeld > CodeSourcery / Mentor Graphics > http://www.mentor.com/embedded-software/ > > > _______________________________________________ > lttng-dev mailing list > lttng-dev@lists.lttng.org > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <1380151240.21051.1391875592403.JavaMail.zimbra@efficios.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <1380151240.21051.1391875592403.JavaMail.zimbra@efficios.com> @ 2014-02-08 16:53 ` Stefan Seefeld [not found] ` <52F66118.5010003@mentor.com> 1 sibling, 0 replies; 28+ messages in thread From: Stefan Seefeld @ 2014-02-08 16:53 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: lttng-dev [-- Attachment #1: Type: text/plain, Size: 2583 bytes --] On 02/08/2014 11:06 AM, Mathieu Desnoyers wrote: > > > ----- Original Message ----- >> From: "Stefan Seefeld" <stefan_seefeld@mentor.com> >> To: lttng-dev@lists.lttng.org >> 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 ? Stefan -- Stefan Seefeld CodeSourcery / Mentor Graphics http://www.mentor.com/embedded-software/ [-- Attachment #2: patch --] [-- Type: text/plain, Size: 1275 bytes --] diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c index 33ed18b..8413f31 100644 --- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c +++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c @@ -54,6 +54,8 @@ static void *static_calloc(size_t nmemb, size_t size) return &static_calloc_buf[prev_offset]; } +static void *(*plibc_realloc)(void *ptr, size_t size); + void *malloc(size_t size) { static void *(*plibc_malloc)(size_t size); @@ -111,6 +113,16 @@ void *calloc(size_t nmemb, size_t size) fprintf(stderr, "callocwrap: unable to find calloc\n"); return NULL; } + /* + * Now seems a good time to also initialize the realloc symbol. + * (Doing that just-in-time won't work as it would corrupt some + * dlfcn-internal state.) + */ + plibc_realloc = dlsym(RTLD_NEXT, "realloc"); + if (plibc_calloc == NULL) { + fprintf(stderr, "callocwrap: unable to find realloc\n"); + return NULL; + } } retval = plibc_calloc(nmemb, size); tracepoint(ust_libc, calloc, nmemb, size, retval); @@ -119,7 +131,6 @@ void *calloc(size_t nmemb, size_t size) void *realloc(void *ptr, size_t size) { - static void *(*plibc_realloc)(void *ptr, size_t size); void *retval; if (plibc_realloc == NULL) { [-- Attachment #3: Type: text/plain, Size: 155 bytes --] _______________________________________________ lttng-dev mailing list lttng-dev@lists.lttng.org http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev ^ permalink raw reply related [flat|nested] 28+ messages in thread
[parent not found: <52F66118.5010003@mentor.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <52F66118.5010003@mentor.com> @ 2014-02-08 22:22 ` Mathieu Desnoyers [not found] ` <905376946.21151.1391898148330.JavaMail.zimbra@efficios.com> 1 sibling, 0 replies; 28+ messages in thread From: Mathieu Desnoyers @ 2014-02-08 22:22 UTC (permalink / raw) To: Stefan Seefeld; +Cc: lttng-dev [-- Attachment #1: Type: text/plain, Size: 3568 bytes --] ----- Original Message ----- > From: "Stefan Seefeld" <stefan_seefeld@mentor.com> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: lttng-dev@lists.lttng.org > 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" <stefan_seefeld@mentor.com> > >> To: lttng-dev@lists.lttng.org > >> 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 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: fix-ust-malloc.patch --] [-- Type: text/x-patch; name=fix-ust-malloc.patch, Size: 8642 bytes --] commit c933316e0c5f45d7c9a49cc75926be3f5c419f10 Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> 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 <mathieu.desnoyers@efficios.com> 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; } [-- Attachment #3: Type: text/plain, Size: 155 bytes --] _______________________________________________ lttng-dev mailing list lttng-dev@lists.lttng.org http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev ^ permalink raw reply related [flat|nested] 28+ messages in thread
[parent not found: <905376946.21151.1391898148330.JavaMail.zimbra@efficios.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <905376946.21151.1391898148330.JavaMail.zimbra@efficios.com> @ 2014-02-09 16:28 ` Stefan Seefeld [not found] ` <52F7ACB6.4060907@mentor.com> ` (2 subsequent siblings) 3 siblings, 0 replies; 28+ messages in thread From: Stefan Seefeld @ 2014-02-09 16:28 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: lttng-dev On 02/08/2014 05:22 PM, Mathieu Desnoyers wrote: > Interesting approach. I assume here you are referring to the temporary static allocator we use during calloc initialization, not my initializing realloc at the same time ? > 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. While I agree that a consistent technique to solve the initialization problem has a lot of appeal, I'm actually hesitant in this particular case: One important limitation of the static allocator is that it requires an upper bound for the buffer. This works fine if we know the circumstance where it is used (I believe dlsym() itself calls calloc() to allocate a global structure that requires 32 bytes). The case I discovered on Friday, however, uses realloc() from within vasprintf(), which needs to grow a buffer to hold an error message, and I don't think the size of that is bounded. Therefore, using a static allocator in that situation seems dangerous. An entirely different argument is that you are suggesting to rewrite an entire library (albeit a small one), when we are trying to get a bugfix into a release even after code freeze. But who am I to tell you that. ;-) Regards, Stefan -- Stefan Seefeld CodeSourcery / Mentor Graphics http://www.mentor.com/embedded-software/ ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <52F7ACB6.4060907@mentor.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <52F7ACB6.4060907@mentor.com> @ 2014-02-11 0:31 ` Mathieu Desnoyers 0 siblings, 0 replies; 28+ messages in thread From: Mathieu Desnoyers @ 2014-02-11 0:31 UTC (permalink / raw) To: Stefan Seefeld; +Cc: lttng-dev ----- Original Message ----- > From: "Stefan Seefeld" <stefan_seefeld@mentor.com> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: lttng-dev@lists.lttng.org > Sent: Sunday, February 9, 2014 11:28:38 AM > Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > > On 02/08/2014 05:22 PM, Mathieu Desnoyers wrote: > > > Interesting approach. > > I assume here you are referring to the temporary static allocator we use > during calloc initialization, not my initializing realloc at the same time ? Actually, I'm referring to your idea of initializing more than just the function we need whenever the allocator is first used. > > > 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. > > While I agree that a consistent technique to solve the initialization > problem has a lot of appeal, I'm actually hesitant in this particular > case: One important limitation of the static allocator is that it > requires an upper bound for the buffer. This works fine if we know the > circumstance where it is used (I believe dlsym() itself calls calloc() > to allocate a global structure that requires 32 bytes). > > The case I discovered on Friday, however, uses realloc() from within > vasprintf(), which needs to grow a buffer to hold an error message, and > I don't think the size of that is bounded. Therefore, using a static > allocator in that situation seems dangerous. By inspecting the source, this error message is made of: - objname (object/file name), - errstring (error detail) I understand your concern about unbounded size for those strings. Since the statically allocated array is 4kB, one thing we could do is rely on a call to mmap() to handle allocations that require more space than available. > An entirely different argument is that you are suggesting to rewrite an > entire library (albeit a small one), when we are trying to get a bugfix > into a release even after code freeze. But who am I to tell you that. ;-) The last thing I want is to start playing hide-and-seek with bugs resulting from interactions between the lttng-ust malloc wrapper and libc. From my point of view, anything we can do to minimize the risk of interaction issues is a huge gain in maintainability, so I'd be OK with fixing things up all throughout this library, rather than going case-by-case for each bug encountered. Thoughts ? Thanks, Mathieu > > Regards, > Stefan > > -- > Stefan Seefeld > CodeSourcery / Mentor Graphics > http://www.mentor.com/embedded-software/ > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: RFC: Fix crash in dlerror() [not found] ` <905376946.21151.1391898148330.JavaMail.zimbra@efficios.com> 2014-02-09 16:28 ` Stefan Seefeld [not found] ` <52F7ACB6.4060907@mentor.com> @ 2014-02-11 0:53 ` Stefan Seefeld [not found] ` <52F9746F.5070302@mentor.com> 3 siblings, 0 replies; 28+ messages in thread From: Stefan Seefeld @ 2014-02-11 0:53 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: lttng-dev Mathieu, after our follow-up discussion I agree to the proposed patch. A few details below... On 02/08/2014 05:22 PM, Mathieu Desnoyers wrote: > 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. OK, agreed. > +static > +void *static_realloc(void *ptr, size_t size) > +{ > + /* Don't free previous memory location */ > + return static_calloc_aligned(1, size, 1); > +} The above of course also needs a memcpy operation, to preserve the original data. > + if (cur_alloc.malloc == NULL) { > fprintf(stderr, "mallocwrap: unable to find malloc\n"); > - return NULL; > + abort(); > } I'm actually not quite sure what behaviour I would prefer as a user. My library-author feeling tell me to not abort the process, but return an error condition (i.e., NULL) and let the caller take care of how to handle it. On the other hand, not finding malloc certainly is a critical error. And, this isn't a library in the proper sense, but a preloaded wrapper, so users can always re-run his apps without it. Hmm... Otherwise the patch looks good. Thanks, Stefan -- Stefan Seefeld CodeSourcery / Mentor Graphics http://www.mentor.com/embedded-software/ ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <52F9746F.5070302@mentor.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <52F9746F.5070302@mentor.com> @ 2014-02-11 20:51 ` Mathieu Desnoyers [not found] ` <1494953964.23233.1392151900002.JavaMail.zimbra@efficios.com> 1 sibling, 0 replies; 28+ messages in thread From: Mathieu Desnoyers @ 2014-02-11 20:51 UTC (permalink / raw) To: Stefan Seefeld; +Cc: lttng-dev [-- Attachment #1: Type: text/plain, Size: 2761 bytes --] ----- Original Message ----- > From: "Stefan Seefeld" <stefan_seefeld@mentor.com> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: lttng-dev@lists.lttng.org > Sent: Monday, February 10, 2014 7:53:03 PM > Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > > Mathieu, > > after our follow-up discussion I agree to the proposed patch. A few > details below... Great! > > On 02/08/2014 05:22 PM, Mathieu Desnoyers wrote: > > > 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. > > OK, agreed. > > > > +static > > +void *static_realloc(void *ptr, size_t size) > > +{ > > + /* Don't free previous memory location */ > > + return static_calloc_aligned(1, size, 1); > > +} > > The above of course also needs a memcpy operation, to preserve the > original data. OK, added. > > > + if (cur_alloc.malloc == NULL) { > > fprintf(stderr, "mallocwrap: unable to find malloc\n"); > > - return NULL; > > + abort(); > > } > > I'm actually not quite sure what behaviour I would prefer as a user. My > library-author feeling tell me to not abort the process, but return an > error condition (i.e., NULL) and let the caller take care of how to > handle it. > On the other hand, not finding malloc certainly is a critical error. > And, this isn't a library in the proper sense, but a preloaded wrapper, > so users can always re-run his apps without it. > Hmm... I'm tempted to go the shortcut route (abort()) since this is really just a wrapper. I'm attaching an updated version, which fixes a couple of incorrect handling of the realloc case for the static allocator. I did not add yet the mmap() stuff, since it adds a bit of complexity to the patch, and I would be tempted to keep things simple in -rc. I really doubt that real-life use-cases will have error messages going beyond 4kB. And if it ever happens, we'll hit a abort() within the wrapper, and it will be easy to fix. I'm tempted to leave this without the mmap stuff for 2.4, so we don't add regressions late in the cycle, is that OK with you ? Thanks, Mathieu > > Otherwise the patch looks good. > > Thanks, > Stefan > > > -- > Stefan Seefeld > CodeSourcery / Mentor Graphics > http://www.mentor.com/embedded-software/ > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: fix-lttng-ust-malloc.patch --] [-- Type: text/x-patch; name=fix-lttng-ust-malloc.patch, Size: 9309 bytes --] diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c index 33ed18b..31be428 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,9 +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(). It keeps a + * size_t value of each object size prior to the object. + */ +static +void *static_calloc_aligned(size_t nmemb, size_t size, size_t alignment) { - unsigned long prev_offset, new_offset, res_offset; + size_t prev_offset, new_offset, res_offset, aligned_offset; + + if (nmemb * size == 0) { + return NULL; + } /* * Protect static_calloc_buf_offset from concurrent updates @@ -45,125 +69,248 @@ 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) { - return NULL; + aligned_offset = ALIGN(prev_offset + sizeof(size_t), alignment); + new_offset = aligned_offset + nmemb * size; + if (new_offset > sizeof(static_calloc_buf)) { + abort(); } - 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]; + *(size_t *) &static_calloc_buf[aligned_offset - sizeof(size_t)] = size; + return &static_calloc_buf[aligned_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) +{ + size_t *old_size = NULL; + void *ret; + + if (size == 0) + return NULL; + + if (ptr) { + old_size = (size_t *) ptr - 1; + if (size <= *old_size) { + /* We can re-use the old entry. */ + *old_size = size; + return ptr; + } + } + /* We need to expand. Don't free previous memory location. */ + ret = static_calloc_aligned(1, size, 1); + if (!ret) + return NULL; + if (ptr) + memcpy(ret, ptr, *old_size); + return ret; +} + +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) { + /* Check whether the memory was allocated with + * static_calloc_align, in which case there is nothing + * to free, and we need to copy the old data. + */ + if (caa_unlikely((char *)ptr >= static_calloc_buf && + (char *)ptr < static_calloc_buf + STATIC_CALLOC_LEN)) { + size_t *old_size; + + old_size = (size_t *) ptr - 1; + retval = cur_alloc.calloc(1, size); + if (retval) { + memcpy(retval, ptr, *old_size); + } + /* + * Note: tweak the "ptr" value so the static allocator + * is transparent to tracepoints. + */ + ptr = NULL; + goto end; + } + + 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); +end: 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; } [-- Attachment #3: Type: text/plain, Size: 155 bytes --] _______________________________________________ lttng-dev mailing list lttng-dev@lists.lttng.org http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev ^ permalink raw reply related [flat|nested] 28+ messages in thread
[parent not found: <1494953964.23233.1392151900002.JavaMail.zimbra@efficios.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <1494953964.23233.1392151900002.JavaMail.zimbra@efficios.com> @ 2014-02-11 20:55 ` Stefan Seefeld [not found] ` <52FA8E5C.40203@mentor.com> 1 sibling, 0 replies; 28+ messages in thread From: Stefan Seefeld @ 2014-02-11 20:55 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: lttng-dev On 02/11/2014 03:51 PM, Mathieu Desnoyers wrote: > ----- Original Message ----- >> From: "Stefan Seefeld" <stefan_seefeld@mentor.com> >> I'm actually not quite sure what behaviour I would prefer as a user. My >> library-author feeling tell me to not abort the process, but return an >> error condition (i.e., NULL) and let the caller take care of how to >> handle it. >> On the other hand, not finding malloc certainly is a critical error. >> And, this isn't a library in the proper sense, but a preloaded wrapper, >> so users can always re-run his apps without it. >> Hmm... > > I'm tempted to go the shortcut route (abort()) since this is really just a > wrapper. OK, fair enough. > I'm attaching an updated version, which fixes a couple of incorrect > handling of the realloc case for the static allocator. I did not add yet the > mmap() stuff, since it adds a bit of complexity to the patch, and I would be > tempted to keep things simple in -rc. I really doubt that real-life use-cases > will have error messages going beyond 4kB. And if it ever happens, we'll hit a > abort() within the wrapper, and it will be easy to fix. I'm tempted to leave > this without the mmap stuff for 2.4, so we don't add regressions late in the > cycle, is that OK with you ? Sounds good. Thanks, Stefan -- Stefan Seefeld CodeSourcery / Mentor Graphics http://www.mentor.com/embedded-software/ ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <52FA8E5C.40203@mentor.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <52FA8E5C.40203@mentor.com> @ 2014-02-12 3:39 ` Mathieu Desnoyers [not found] ` <975853026.23546.1392176395828.JavaMail.zimbra@efficios.com> 1 sibling, 0 replies; 28+ messages in thread From: Mathieu Desnoyers @ 2014-02-12 3:39 UTC (permalink / raw) To: Stefan Seefeld; +Cc: lttng-dev [-- Attachment #1: Type: text/plain, Size: 156 bytes --] Updated patch, now with tracepoints in the static allocator. Review welcome, Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: fix-lttng-ust-malloc.patch --] [-- Type: text/x-patch; name=fix-lttng-ust-malloc.patch, Size: 9813 bytes --] diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c index 33ed18b..b8892e7 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,9 +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(). It keeps a + * size_t value of each object size prior to the object. + */ +static +void *static_calloc_aligned(size_t nmemb, size_t size, size_t alignment) { - unsigned long prev_offset, new_offset, res_offset; + size_t prev_offset, new_offset, res_offset, aligned_offset; + + if (nmemb * size == 0) { + return NULL; + } /* * Protect static_calloc_buf_offset from concurrent updates @@ -45,125 +69,263 @@ 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) { - return NULL; + aligned_offset = ALIGN(prev_offset + sizeof(size_t), alignment); + new_offset = aligned_offset + nmemb * size; + if (new_offset > sizeof(static_calloc_buf)) { + abort(); } - 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]; + *(size_t *) &static_calloc_buf[aligned_offset - sizeof(size_t)] = size; + return &static_calloc_buf[aligned_offset]; +} + +static +void *static_calloc(size_t nmemb, size_t size) +{ + void *retval; + + retval = static_calloc_aligned(nmemb, size, 1); + tracepoint(ust_libc, calloc, nmemb, size, retval); + return retval; +} + +static +void *static_malloc(size_t size) +{ + void *retval; + + retval = static_calloc_aligned(1, size, 1); + tracepoint(ust_libc, malloc, size, retval); + return retval; +} + +static +void static_free(void *ptr) +{ + /* no-op. */ + tracepoint(ust_libc, free, ptr); +} + +static +void *static_realloc(void *ptr, size_t size) +{ + size_t *old_size = NULL; + void *retval; + + if (size == 0) { + retval = NULL; + goto end; + } + + if (ptr) { + old_size = (size_t *) ptr - 1; + if (size <= *old_size) { + /* We can re-use the old entry. */ + *old_size = size; + retval = ptr; + goto end; + } + } + /* We need to expand. Don't free previous memory location. */ + retval = static_calloc_aligned(1, size, 1); + assert(retval); + if (ptr) + memcpy(retval, ptr, *old_size); +end: + tracepoint(ust_libc, realloc, ptr, size, retval); + return retval; +} + +static +void *static_memalign(size_t alignment, size_t size) +{ + void *retval; + + retval = static_calloc_aligned(1, size, alignment); + tracepoint(ust_libc, memalign, alignment, size, retval); + return retval; +} + +static +int static_posix_memalign(void **memptr, size_t alignment, size_t size) +{ + int retval = 0; + void *ptr; + + /* Check for power of 2, larger than void *. */ + if (alignment & (alignment - 1) + || alignment < sizeof(void *) + || alignment == 0) { + retval = EINVAL; + goto end; + } + ptr = static_calloc_aligned(1, size, alignment); + *memptr = ptr; + if (size && !ptr) + retval = ENOMEM; +end: + tracepoint(ust_libc, posix_memalign, *memptr, alignment, size, retval); + 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); + tracepoint(ust_libc, free, ptr); - /* Check whether the memory was allocated with - * static_calloc, in which case there is nothing - * to free. + /* + * Check whether the memory was allocated with + * 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) { + /* Check whether the memory was allocated with + * static_calloc_align, in which case there is nothing + * to free, and we need to copy the old data. + */ + if (caa_unlikely((char *)ptr >= static_calloc_buf && + (char *)ptr < static_calloc_buf + STATIC_CALLOC_LEN)) { + size_t *old_size; + + old_size = (size_t *) ptr - 1; + retval = cur_alloc.calloc(1, size); + if (retval) { + memcpy(retval, ptr, *old_size); + } + goto end; + } + + 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); +end: 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; } [-- Attachment #3: Type: text/plain, Size: 155 bytes --] _______________________________________________ lttng-dev mailing list lttng-dev@lists.lttng.org http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev ^ permalink raw reply related [flat|nested] 28+ messages in thread
[parent not found: <975853026.23546.1392176395828.JavaMail.zimbra@efficios.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <975853026.23546.1392176395828.JavaMail.zimbra@efficios.com> @ 2014-02-12 14:35 ` Mathieu Desnoyers [not found] ` <2141704629.23782.1392215703555.JavaMail.zimbra@efficios.com> 1 sibling, 0 replies; 28+ messages in thread From: Mathieu Desnoyers @ 2014-02-12 14:35 UTC (permalink / raw) To: Stefan Seefeld; +Cc: lttng-dev [-- Attachment #1: Type: text/plain, Size: 1058 bytes --] Updated patch, now ensuring that the wrapper is initialized before UST starts other threads. It now has a constructor function that will take care of initializing the wrapper before other program threads are started anyhow. I also added a missing lookup check (calloc pointer) within realloc (branch handling realloc of a statically allocated pointer). Thanks, Mathieu ----- Original Message ----- > From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > To: "Stefan Seefeld" <stefan_seefeld@mentor.com> > Cc: lttng-dev@lists.lttng.org > Sent: Tuesday, February 11, 2014 10:39:55 PM > Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > > Updated patch, now with tracepoints in the static allocator. > > Review welcome, > > Thanks, > > Mathieu > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com > > _______________________________________________ > lttng-dev mailing list > lttng-dev@lists.lttng.org > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: fix-lttng-ust-malloc.patch --] [-- Type: text/x-patch; name=fix-lttng-ust-malloc.patch, Size: 11146 bytes --] diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c index 33ed18b..8296ae2 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,9 +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(). It keeps a + * size_t value of each object size prior to the object. + */ +static +void *static_calloc_aligned(size_t nmemb, size_t size, size_t alignment) { - unsigned long prev_offset, new_offset, res_offset; + size_t prev_offset, new_offset, res_offset, aligned_offset; + + if (nmemb * size == 0) { + return NULL; + } /* * Protect static_calloc_buf_offset from concurrent updates @@ -45,125 +69,284 @@ 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) { - return NULL; + aligned_offset = ALIGN(prev_offset + sizeof(size_t), alignment); + new_offset = aligned_offset + nmemb * size; + if (new_offset > sizeof(static_calloc_buf)) { + abort(); } - 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]; + *(size_t *) &static_calloc_buf[aligned_offset - sizeof(size_t)] = size; + return &static_calloc_buf[aligned_offset]; +} + +static +void *static_calloc(size_t nmemb, size_t size) +{ + void *retval; + + retval = static_calloc_aligned(nmemb, size, 1); + tracepoint(ust_libc, calloc, nmemb, size, retval); + return retval; +} + +static +void *static_malloc(size_t size) +{ + void *retval; + + retval = static_calloc_aligned(1, size, 1); + tracepoint(ust_libc, malloc, size, retval); + return retval; +} + +static +void static_free(void *ptr) +{ + /* no-op. */ + tracepoint(ust_libc, free, ptr); +} + +static +void *static_realloc(void *ptr, size_t size) +{ + size_t *old_size = NULL; + void *retval; + + if (size == 0) { + retval = NULL; + goto end; + } + + if (ptr) { + old_size = (size_t *) ptr - 1; + if (size <= *old_size) { + /* We can re-use the old entry. */ + *old_size = size; + retval = ptr; + goto end; + } + } + /* We need to expand. Don't free previous memory location. */ + retval = static_calloc_aligned(1, size, 1); + assert(retval); + if (ptr) + memcpy(retval, ptr, *old_size); +end: + tracepoint(ust_libc, realloc, ptr, size, retval); + return retval; +} + +static +void *static_memalign(size_t alignment, size_t size) +{ + void *retval; + + retval = static_calloc_aligned(1, size, alignment); + tracepoint(ust_libc, memalign, alignment, size, retval); + return retval; +} + +static +int static_posix_memalign(void **memptr, size_t alignment, size_t size) +{ + int retval = 0; + void *ptr; + + /* Check for power of 2, larger than void *. */ + if (alignment & (alignment - 1) + || alignment < sizeof(void *) + || alignment == 0) { + retval = EINVAL; + goto end; + } + ptr = static_calloc_aligned(1, size, alignment); + *memptr = ptr; + if (size && !ptr) + retval = ENOMEM; +end: + tracepoint(ust_libc, posix_memalign, *memptr, alignment, size, retval); + 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); + tracepoint(ust_libc, free, ptr); - /* Check whether the memory was allocated with - * static_calloc, in which case there is nothing - * to free. + /* + * Check whether the memory was allocated with + * 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) { + /* Check whether the memory was allocated with + * static_calloc_align, in which case there is nothing + * to free, and we need to copy the old data. + */ + if (caa_unlikely((char *)ptr >= static_calloc_buf && + (char *)ptr < static_calloc_buf + STATIC_CALLOC_LEN)) { + size_t *old_size; + + old_size = (size_t *) ptr - 1; + if (cur_alloc.calloc == NULL) { + lookup_all_symbols(); + if (cur_alloc.calloc == NULL) { + fprintf(stderr, "reallocwrap: unable to find calloc\n"); + abort(); + } + } + retval = cur_alloc.calloc(1, size); + if (retval) { + memcpy(retval, ptr, *old_size); + } + goto end; + } + + 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); +end: 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; } + +__attribute__((constructor)) +void lttng_ust_malloc_wrapper_init(void) +{ + /* Initialization already done */ + if (cur_alloc.calloc) { + return; + } + /* + * Ensure the allocator is in place before the process becomes + * multithreaded. + */ + lookup_all_symbols(); +} diff --git a/liblttng-ust/lttng-ust-comm.c b/liblttng-ust/lttng-ust-comm.c index 0c96f01..2778694 100644 --- a/liblttng-ust/lttng-ust-comm.c +++ b/liblttng-ust/lttng-ust-comm.c @@ -1311,6 +1311,14 @@ quit: } /* + * Weak symbol to call when the ust malloc wrapper is not loaded. + */ +__attribute__((weak)) +void lttng_ust_malloc_wrapper_init(void) +{ +} + +/* * sessiond monitoring thread: monitor presence of global and per-user * sessiond by polling the application common named pipe. */ @@ -1350,6 +1358,10 @@ void __attribute__((constructor)) lttng_ust_init(void) lttng_ring_buffer_client_discard_init(); lttng_ring_buffer_client_discard_rt_init(); lttng_context_init(); + /* + * Invoke ust malloc wrapper init before starting other threads. + */ + lttng_ust_malloc_wrapper_init(); timeout_mode = get_constructor_timeout(&constructor_timeout); [-- Attachment #3: Type: text/plain, Size: 155 bytes --] _______________________________________________ lttng-dev mailing list lttng-dev@lists.lttng.org http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev ^ permalink raw reply related [flat|nested] 28+ messages in thread
[parent not found: <2141704629.23782.1392215703555.JavaMail.zimbra@efficios.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <2141704629.23782.1392215703555.JavaMail.zimbra@efficios.com> @ 2014-02-12 21:59 ` Mathieu Desnoyers 0 siblings, 0 replies; 28+ messages in thread From: Mathieu Desnoyers @ 2014-02-12 21:59 UTC (permalink / raw) To: Stefan Seefeld; +Cc: lttng-dev Patch merged into master as commit: commit 2594a5b4f2b60642973b4941ee506be41786c4d3 Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> 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 <mathieu.desnoyers@efficios.com> Thanks! Mathieu ----- Original Message ----- > From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > To: "Stefan Seefeld" <stefan_seefeld@mentor.com> > Cc: lttng-dev@lists.lttng.org > Sent: Wednesday, February 12, 2014 9:35:03 AM > Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > > Updated patch, now ensuring that the wrapper is initialized before > UST starts other threads. It now has a constructor function that will > take care of initializing the wrapper before other program threads are > started anyhow. > > I also added a missing lookup check (calloc pointer) within realloc > (branch handling realloc of a statically allocated pointer). > > Thanks, > > Mathieu > > ----- Original Message ----- > > From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > > To: "Stefan Seefeld" <stefan_seefeld@mentor.com> > > Cc: lttng-dev@lists.lttng.org > > Sent: Tuesday, February 11, 2014 10:39:55 PM > > Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > > > > Updated patch, now with tracepoints in the static allocator. > > > > Review welcome, > > > > Thanks, > > > > Mathieu > > -- > > Mathieu Desnoyers > > EfficiOS Inc. > > http://www.efficios.com > > > > _______________________________________________ > > lttng-dev mailing list > > lttng-dev@lists.lttng.org > > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev > > > > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com > > _______________________________________________ > lttng-dev mailing list > lttng-dev@lists.lttng.org > http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <52FCAA28.6020708@mentor.com>]
[parent not found: <1026072798.24303.1392297452496.JavaMail.zimbra@efficios.com>]
[parent not found: <52FCCCD2.9050302@mentor.com>]
[parent not found: <610029715.24333.1392300717731.JavaMail.zimbra@efficios.com>]
[parent not found: <1692042945.24342.1392301795996.JavaMail.zimbra@efficios.com>]
[parent not found: <1119459836.24348.1392302831554.JavaMail.zimbra@efficios.com>]
[parent not found: <52FCE732.9090508@mentor.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <52FCE732.9090508@mentor.com> @ 2014-02-13 16:40 ` Mathieu Desnoyers [not found] ` <581364674.24417.1392309613306.JavaMail.zimbra@efficios.com> 1 sibling, 0 replies; 28+ messages in thread From: Mathieu Desnoyers @ 2014-02-13 16:40 UTC (permalink / raw) To: Paul Woegerer; +Cc: Stefan Seefeld, lttng-dev, Paul E. McKenney (adding back lttng-dev, and CC Paul E. McKenney. He may have some interesting insight in this compiler reordering of store vs function call.) ----- Original Message ----- > From: "Paul Woegerer" <Paul_Woegerer@mentor.com> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > Sent: Thursday, February 13, 2014 10:39:30 AM > Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > > On 02/13/2014 03:47 PM, Mathieu Desnoyers wrote: > > ----- Original Message ----- > >> From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >> To: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >> Sent: Thursday, February 13, 2014 9:29:56 AM > >> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >> > >> ----- Original Message ----- > >>> From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >>> To: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >>> Sent: Thursday, February 13, 2014 9:11:57 AM > >>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >>> > >>> ----- Original Message ----- > >>>> From: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >>>> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >>>> Sent: Thursday, February 13, 2014 8:46:58 AM > >>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >>>> > >>>> On 02/13/2014 02:17 PM, Mathieu Desnoyers wrote: [...] > >>> > >>> I've been able to reproduce with > >>> > >>> gcc version 4.8.2 (Debian 4.8.2-14) > >>> > >>> and indeed adding the volatile fixes the issue. But it seems wrong that > >>> the > >>> compiler thinks it is within its rights to assume it can move the store > >>> to that structure after the call to dlsym(). > >> [...] > > > > Adding a cmm_barrier() between the call to setup_static_allocator() and > > the first dlsym() call fixes the issue too. > > > > Thoughts ? > > Intuitively I would have thought that the def-use analysis of the compiler > recognizes the first def to the fields as redundant. > > e.g. > > def cur_alloc.calloc (1) > def cur_alloc.calloc (2) > use cur_alloc.calloc > > so from compiler perspective (1) is redundant. Well actually, it's more: def cur_alloc.calloc (1) calls to dlsym() (should act as a compiler memory barrier) def cur_alloc.calloc (2) (where the use of cur_alloc.calloc is within the dlsym() call) What seems to happen here is that gcc somehow assumes that dlsym() will never be interested in the value of the static variable cur_alloc.calloc. But I doubt this assumption is valid, because dlsym() could very well have access to a function pointer leading to a function within the object containing cur_alloc, and thus have to access cur_alloc. How can this store be optimized away without breaking sequential consistency ? > > It cannot know about the indirect recursion via dlsym. To express this to the > compiler you have to make the fields of your struct volatile. AFAIK, the compiler should assume that there may be a call to a function using cur_alloc within dlsym(). > > BTW, for copying cur_alloc, let the compiler create the memcpy for you. Good point! I don't see how volatile would be required here. Maybe it fixes the issue (so does adding a compiler barrier), but I feel uneasy just fixing it up within lttng-ust if there is a deeper issue in gcc 4.8. Thoughts ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <581364674.24417.1392309613306.JavaMail.zimbra@efficios.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <581364674.24417.1392309613306.JavaMail.zimbra@efficios.com> @ 2014-02-13 16:51 ` Woegerer, Paul [not found] ` <52FCF7F5.9070908@mentor.com> 1 sibling, 0 replies; 28+ messages in thread From: Woegerer, Paul @ 2014-02-13 16:51 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: Stefan Seefeld, lttng-dev, Paul E. McKenney On 02/13/2014 05:40 PM, Mathieu Desnoyers wrote: > (adding back lttng-dev, and CC Paul E. McKenney. He may have > some interesting insight in this compiler reordering of store > vs function call.) > > ----- Original Message ----- >> From: "Paul Woegerer" <Paul_Woegerer@mentor.com> >> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> >> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> >> Sent: Thursday, February 13, 2014 10:39:30 AM >> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() >> >> On 02/13/2014 03:47 PM, Mathieu Desnoyers wrote: >>> ----- Original Message ----- >>>> From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> >>>> To: "Paul Woegerer" <Paul_Woegerer@mentor.com> >>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> >>>> Sent: Thursday, February 13, 2014 9:29:56 AM >>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() >>>> >>>> ----- Original Message ----- >>>>> From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> >>>>> To: "Paul Woegerer" <Paul_Woegerer@mentor.com> >>>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> >>>>> Sent: Thursday, February 13, 2014 9:11:57 AM >>>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() >>>>> >>>>> ----- Original Message ----- >>>>>> From: "Paul Woegerer" <Paul_Woegerer@mentor.com> >>>>>> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> >>>>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> >>>>>> Sent: Thursday, February 13, 2014 8:46:58 AM >>>>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() >>>>>> >>>>>> On 02/13/2014 02:17 PM, Mathieu Desnoyers wrote: > [...] >>>>> I've been able to reproduce with >>>>> >>>>> gcc version 4.8.2 (Debian 4.8.2-14) >>>>> >>>>> and indeed adding the volatile fixes the issue. But it seems wrong that >>>>> the >>>>> compiler thinks it is within its rights to assume it can move the store >>>>> to that structure after the call to dlsym(). > [...] > >>> Adding a cmm_barrier() between the call to setup_static_allocator() and >>> the first dlsym() call fixes the issue too. >>> >>> Thoughts ? >> Intuitively I would have thought that the def-use analysis of the compiler >> recognizes the first def to the fields as redundant. >> >> e.g. >> >> def cur_alloc.calloc (1) >> def cur_alloc.calloc (2) >> use cur_alloc.calloc >> >> so from compiler perspective (1) is redundant. > Well actually, it's more: > > def cur_alloc.calloc (1) > > calls to dlsym() (should act as a compiler memory barrier) > > def cur_alloc.calloc (2) > > (where the use of cur_alloc.calloc is within the dlsym() call) > > What seems to happen here is that gcc somehow assumes that dlsym() will never > be interested in the value of the static variable cur_alloc.calloc. But I doubt > this assumption is valid, because dlsym() could very well have access to a > function pointer leading to a function within the object containing cur_alloc, > and thus have to access cur_alloc. How can this store be optimized away without To assume that dlsym() has "access" to the local static variable cur_alloc is not something anyone should expect from the compiler. The fact that dlsym() does have "access" is only due to preloading and the circumstance that library functions that dlsym() itself uses are redirected to the ones defined in the wrapper. -- Best, Paul > breaking sequential consistency ? > >> It cannot know about the indirect recursion via dlsym. To express this to the >> compiler you have to make the fields of your struct volatile. > AFAIK, the compiler should assume that there may be a call to a function using > cur_alloc within dlsym(). > >> BTW, for copying cur_alloc, let the compiler create the memcpy for you. > Good point! > > I don't see how volatile would be required here. Maybe it fixes the issue > (so does adding a compiler barrier), but I feel uneasy just fixing it up > within lttng-ust if there is a deeper issue in gcc 4.8. > > Thoughts ? > > Thanks, > > Mathieu > > -- Paul Woegerer, SW Development Engineer Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer> Mentor Graphics, Embedded Software Division ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <52FCF7F5.9070908@mentor.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <52FCF7F5.9070908@mentor.com> @ 2014-02-13 18:52 ` Mathieu Desnoyers [not found] ` <934762932.24558.1392317539645.JavaMail.zimbra@efficios.com> 1 sibling, 0 replies; 28+ messages in thread From: Mathieu Desnoyers @ 2014-02-13 18:52 UTC (permalink / raw) To: Paul Woegerer; +Cc: Stefan Seefeld, lttng-dev, Paul E. McKenney ----- Original Message ----- > From: "Paul Woegerer" <Paul_Woegerer@mentor.com> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com>, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>, "lttng-dev" > <lttng-dev@lists.lttng.org> > Sent: Thursday, February 13, 2014 11:51:01 AM > Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > > On 02/13/2014 05:40 PM, Mathieu Desnoyers wrote: > > (adding back lttng-dev, and CC Paul E. McKenney. He may have > > some interesting insight in this compiler reordering of store > > vs function call.) > > > > ----- Original Message ----- > >> From: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >> Sent: Thursday, February 13, 2014 10:39:30 AM > >> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >> > >> On 02/13/2014 03:47 PM, Mathieu Desnoyers wrote: > >>> ----- Original Message ----- > >>>> From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >>>> To: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >>>> Sent: Thursday, February 13, 2014 9:29:56 AM > >>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >>>> > >>>> ----- Original Message ----- > >>>>> From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >>>>> To: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >>>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >>>>> Sent: Thursday, February 13, 2014 9:11:57 AM > >>>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >>>>> > >>>>> ----- Original Message ----- > >>>>>> From: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >>>>>> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >>>>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >>>>>> Sent: Thursday, February 13, 2014 8:46:58 AM > >>>>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >>>>>> > >>>>>> On 02/13/2014 02:17 PM, Mathieu Desnoyers wrote: > > [...] > >>>>> I've been able to reproduce with > >>>>> > >>>>> gcc version 4.8.2 (Debian 4.8.2-14) > >>>>> > >>>>> and indeed adding the volatile fixes the issue. But it seems wrong that > >>>>> the > >>>>> compiler thinks it is within its rights to assume it can move the store > >>>>> to that structure after the call to dlsym(). > > [...] > > > >>> Adding a cmm_barrier() between the call to setup_static_allocator() and > >>> the first dlsym() call fixes the issue too. > >>> > >>> Thoughts ? > >> Intuitively I would have thought that the def-use analysis of the compiler > >> recognizes the first def to the fields as redundant. > >> > >> e.g. > >> > >> def cur_alloc.calloc (1) > >> def cur_alloc.calloc (2) > >> use cur_alloc.calloc > >> > >> so from compiler perspective (1) is redundant. > > Well actually, it's more: > > > > def cur_alloc.calloc (1) > > > > calls to dlsym() (should act as a compiler memory barrier) > > > > def cur_alloc.calloc (2) > > > > (where the use of cur_alloc.calloc is within the dlsym() call) > > > > What seems to happen here is that gcc somehow assumes that dlsym() will > > never > > be interested in the value of the static variable cur_alloc.calloc. But I > > doubt > > this assumption is valid, because dlsym() could very well have access to a > > function pointer leading to a function within the object containing > > cur_alloc, > > and thus have to access cur_alloc. How can this store be optimized away > > without > > To assume that dlsym() has "access" to the local static variable > cur_alloc is not something anyone should expect from the compiler. The > fact that dlsym() does have "access" is only due to preloading and the > circumstance that library functions that dlsym() itself uses are > redirected to the ones defined in the wrapper. Can you provide quotes of the relevant parts of the C99 standard supporting this ? My understanding is that there is a sequence point before the function call: ISO/IEC 9899:TC2 5.1.2.3 Program execution "2. Accessing a volatile object, modifying an object, modifying a file, or calling a function that does any of those operations are all side effects,11) which are changes in the state of the execution environment. Evaluation of an expression may produce side effects. At certain specified points in the execution sequence called sequence points, all side effects of previous evaluations shall be complete and no side effects of subsequent evaluations shall have taken place. (A summary of the sequence points is given in annex C.)" 6.5.2.2 Function calls "10. The order of evaluation of the function designator, the actual arguments, and subexpressions within the actual arguments is unspecified, but there is a sequence point before the actual call." And this sequence point should ensure that the side-effects of stores to cur_alloc should be complete before the call takes place. Thoughts ? Thanks, Mathieu > > -- > Best, > Paul > > > breaking sequential consistency ? > > > >> It cannot know about the indirect recursion via dlsym. To express this to > >> the > >> compiler you have to make the fields of your struct volatile. > > AFAIK, the compiler should assume that there may be a call to a function > > using > > cur_alloc within dlsym(). > > > >> BTW, for copying cur_alloc, let the compiler create the memcpy for you. > > Good point! > > > > I don't see how volatile would be required here. Maybe it fixes the issue > > (so does adding a compiler barrier), but I feel uneasy just fixing it up > > within lttng-ust if there is a deeper issue in gcc 4.8. > > > > Thoughts ? > > > > Thanks, > > > > Mathieu > > > > > > > -- > Paul Woegerer, SW Development Engineer > Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer> > Mentor Graphics, Embedded Software Division > > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <934762932.24558.1392317539645.JavaMail.zimbra@efficios.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <934762932.24558.1392317539645.JavaMail.zimbra@efficios.com> @ 2014-02-13 19:44 ` Woegerer, Paul [not found] ` <A30AF42E15BD64459E202697468DFFA77E387F33@EU-MBX-04.mgc.mentorg.com> 1 sibling, 0 replies; 28+ messages in thread From: Woegerer, Paul @ 2014-02-13 19:44 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: Seefeld, Stefan, lttng-dev, Paul E. McKenney Sorry for the webmail reply ... A sequence point is not a memory barrier. (I will try to come back with more precise arguments tomorrow morning) -- Thanks, Paul ________________________________________ From: Mathieu Desnoyers [mathieu.desnoyers@efficios.com] Sent: Thursday, February 13, 2014 19:52 To: Woegerer, Paul Cc: Seefeld, Stefan; Paul E. McKenney; lttng-dev Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() ----- Original Message ----- > From: "Paul Woegerer" <Paul_Woegerer@mentor.com> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com>, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>, "lttng-dev" > <lttng-dev@lists.lttng.org> > Sent: Thursday, February 13, 2014 11:51:01 AM > Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > > On 02/13/2014 05:40 PM, Mathieu Desnoyers wrote: > > (adding back lttng-dev, and CC Paul E. McKenney. He may have > > some interesting insight in this compiler reordering of store > > vs function call.) > > > > ----- Original Message ----- > >> From: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >> Sent: Thursday, February 13, 2014 10:39:30 AM > >> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >> > >> On 02/13/2014 03:47 PM, Mathieu Desnoyers wrote: > >>> ----- Original Message ----- > >>>> From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >>>> To: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >>>> Sent: Thursday, February 13, 2014 9:29:56 AM > >>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >>>> > >>>> ----- Original Message ----- > >>>>> From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >>>>> To: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >>>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >>>>> Sent: Thursday, February 13, 2014 9:11:57 AM > >>>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >>>>> > >>>>> ----- Original Message ----- > >>>>>> From: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >>>>>> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >>>>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >>>>>> Sent: Thursday, February 13, 2014 8:46:58 AM > >>>>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >>>>>> > >>>>>> On 02/13/2014 02:17 PM, Mathieu Desnoyers wrote: > > [...] > >>>>> I've been able to reproduce with > >>>>> > >>>>> gcc version 4.8.2 (Debian 4.8.2-14) > >>>>> > >>>>> and indeed adding the volatile fixes the issue. But it seems wrong that > >>>>> the > >>>>> compiler thinks it is within its rights to assume it can move the store > >>>>> to that structure after the call to dlsym(). > > [...] > > > >>> Adding a cmm_barrier() between the call to setup_static_allocator() and > >>> the first dlsym() call fixes the issue too. > >>> > >>> Thoughts ? > >> Intuitively I would have thought that the def-use analysis of the compiler > >> recognizes the first def to the fields as redundant. > >> > >> e.g. > >> > >> def cur_alloc.calloc (1) > >> def cur_alloc.calloc (2) > >> use cur_alloc.calloc > >> > >> so from compiler perspective (1) is redundant. > > Well actually, it's more: > > > > def cur_alloc.calloc (1) > > > > calls to dlsym() (should act as a compiler memory barrier) > > > > def cur_alloc.calloc (2) > > > > (where the use of cur_alloc.calloc is within the dlsym() call) > > > > What seems to happen here is that gcc somehow assumes that dlsym() will > > never > > be interested in the value of the static variable cur_alloc.calloc. But I > > doubt > > this assumption is valid, because dlsym() could very well have access to a > > function pointer leading to a function within the object containing > > cur_alloc, > > and thus have to access cur_alloc. How can this store be optimized away > > without > > To assume that dlsym() has "access" to the local static variable > cur_alloc is not something anyone should expect from the compiler. The > fact that dlsym() does have "access" is only due to preloading and the > circumstance that library functions that dlsym() itself uses are > redirected to the ones defined in the wrapper. Can you provide quotes of the relevant parts of the C99 standard supporting this ? My understanding is that there is a sequence point before the function call: ISO/IEC 9899:TC2 5.1.2.3 Program execution "2. Accessing a volatile object, modifying an object, modifying a file, or calling a function that does any of those operations are all side effects,11) which are changes in the state of the execution environment. Evaluation of an expression may produce side effects. At certain specified points in the execution sequence called sequence points, all side effects of previous evaluations shall be complete and no side effects of subsequent evaluations shall have taken place. (A summary of the sequence points is given in annex C.)" 6.5.2.2 Function calls "10. The order of evaluation of the function designator, the actual arguments, and subexpressions within the actual arguments is unspecified, but there is a sequence point before the actual call." And this sequence point should ensure that the side-effects of stores to cur_alloc should be complete before the call takes place. Thoughts ? Thanks, Mathieu > > -- > Best, > Paul > > > breaking sequential consistency ? > > > >> It cannot know about the indirect recursion via dlsym. To express this to > >> the > >> compiler you have to make the fields of your struct volatile. > > AFAIK, the compiler should assume that there may be a call to a function > > using > > cur_alloc within dlsym(). > > > >> BTW, for copying cur_alloc, let the compiler create the memcpy for you. > > Good point! > > > > I don't see how volatile would be required here. Maybe it fixes the issue > > (so does adding a compiler barrier), but I feel uneasy just fixing it up > > within lttng-ust if there is a deeper issue in gcc 4.8. > > > > Thoughts ? > > > > Thanks, > > > > Mathieu > > > > > > > -- > Paul Woegerer, SW Development Engineer > Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer> > Mentor Graphics, Embedded Software Division > > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <A30AF42E15BD64459E202697468DFFA77E387F33@EU-MBX-04.mgc.mentorg.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <A30AF42E15BD64459E202697468DFFA77E387F33@EU-MBX-04.mgc.mentorg.com> @ 2014-02-13 22:06 ` Woegerer, Paul [not found] ` <A30AF42E15BD64459E202697468DFFA77E389F61@EU-MBX-04.mgc.mentorg.com> 1 sibling, 0 replies; 28+ messages in thread From: Woegerer, Paul @ 2014-02-13 22:06 UTC (permalink / raw) To: Mathieu Desnoyers; +Cc: Seefeld, Stefan, lttng-dev, Paul E. McKenney Let me put it this way ... If (hypothetically, just for the sake of the argument) we would have dlsym with the following signature: void *dlsym(void *handle, const char *symbol, void *dummy); instead of: void *dlsym(void *handle, const char *symbol); and we would call it with: af.calloc = dlsym(RTLD_NEXT, "calloc", &cur_alloc); then (because of the aliasing of cur_alloc (caused by &cur_alloc) the compiler would be forced to store the effects done on cur_alloc into memory prior to calling dlsym. -- Thanks, Paul ________________________________________ From: Woegerer, Paul Sent: Thursday, February 13, 2014 20:44 To: Mathieu Desnoyers Cc: Seefeld, Stefan; lttng-dev; Paul E. McKenney Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() Sorry for the webmail reply ... A sequence point is not a memory barrier. (I will try to come back with more precise arguments tomorrow morning) -- Thanks, Paul ________________________________________ From: Mathieu Desnoyers [mathieu.desnoyers@efficios.com] Sent: Thursday, February 13, 2014 19:52 To: Woegerer, Paul Cc: Seefeld, Stefan; Paul E. McKenney; lttng-dev Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() ----- Original Message ----- > From: "Paul Woegerer" <Paul_Woegerer@mentor.com> > To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com>, "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>, "lttng-dev" > <lttng-dev@lists.lttng.org> > Sent: Thursday, February 13, 2014 11:51:01 AM > Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > > On 02/13/2014 05:40 PM, Mathieu Desnoyers wrote: > > (adding back lttng-dev, and CC Paul E. McKenney. He may have > > some interesting insight in this compiler reordering of store > > vs function call.) > > > > ----- Original Message ----- > >> From: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >> Sent: Thursday, February 13, 2014 10:39:30 AM > >> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >> > >> On 02/13/2014 03:47 PM, Mathieu Desnoyers wrote: > >>> ----- Original Message ----- > >>>> From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >>>> To: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >>>> Sent: Thursday, February 13, 2014 9:29:56 AM > >>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >>>> > >>>> ----- Original Message ----- > >>>>> From: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >>>>> To: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >>>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >>>>> Sent: Thursday, February 13, 2014 9:11:57 AM > >>>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >>>>> > >>>>> ----- Original Message ----- > >>>>>> From: "Paul Woegerer" <Paul_Woegerer@mentor.com> > >>>>>> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com> > >>>>>> Cc: "Stefan Seefeld" <Stefan_Seefeld@mentor.com> > >>>>>> Sent: Thursday, February 13, 2014 8:46:58 AM > >>>>>> Subject: Re: [lttng-dev] RFC: Fix crash in dlerror() > >>>>>> > >>>>>> On 02/13/2014 02:17 PM, Mathieu Desnoyers wrote: > > [...] > >>>>> I've been able to reproduce with > >>>>> > >>>>> gcc version 4.8.2 (Debian 4.8.2-14) > >>>>> > >>>>> and indeed adding the volatile fixes the issue. But it seems wrong that > >>>>> the > >>>>> compiler thinks it is within its rights to assume it can move the store > >>>>> to that structure after the call to dlsym(). > > [...] > > > >>> Adding a cmm_barrier() between the call to setup_static_allocator() and > >>> the first dlsym() call fixes the issue too. > >>> > >>> Thoughts ? > >> Intuitively I would have thought that the def-use analysis of the compiler > >> recognizes the first def to the fields as redundant. > >> > >> e.g. > >> > >> def cur_alloc.calloc (1) > >> def cur_alloc.calloc (2) > >> use cur_alloc.calloc > >> > >> so from compiler perspective (1) is redundant. > > Well actually, it's more: > > > > def cur_alloc.calloc (1) > > > > calls to dlsym() (should act as a compiler memory barrier) > > > > def cur_alloc.calloc (2) > > > > (where the use of cur_alloc.calloc is within the dlsym() call) > > > > What seems to happen here is that gcc somehow assumes that dlsym() will > > never > > be interested in the value of the static variable cur_alloc.calloc. But I > > doubt > > this assumption is valid, because dlsym() could very well have access to a > > function pointer leading to a function within the object containing > > cur_alloc, > > and thus have to access cur_alloc. How can this store be optimized away > > without > > To assume that dlsym() has "access" to the local static variable > cur_alloc is not something anyone should expect from the compiler. The > fact that dlsym() does have "access" is only due to preloading and the > circumstance that library functions that dlsym() itself uses are > redirected to the ones defined in the wrapper. Can you provide quotes of the relevant parts of the C99 standard supporting this ? My understanding is that there is a sequence point before the function call: ISO/IEC 9899:TC2 5.1.2.3 Program execution "2. Accessing a volatile object, modifying an object, modifying a file, or calling a function that does any of those operations are all side effects,11) which are changes in the state of the execution environment. Evaluation of an expression may produce side effects. At certain specified points in the execution sequence called sequence points, all side effects of previous evaluations shall be complete and no side effects of subsequent evaluations shall have taken place. (A summary of the sequence points is given in annex C.)" 6.5.2.2 Function calls "10. The order of evaluation of the function designator, the actual arguments, and subexpressions within the actual arguments is unspecified, but there is a sequence point before the actual call." And this sequence point should ensure that the side-effects of stores to cur_alloc should be complete before the call takes place. Thoughts ? Thanks, Mathieu > > -- > Best, > Paul > > > breaking sequential consistency ? > > > >> It cannot know about the indirect recursion via dlsym. To express this to > >> the > >> compiler you have to make the fields of your struct volatile. > > AFAIK, the compiler should assume that there may be a call to a function > > using > > cur_alloc within dlsym(). > > > >> BTW, for copying cur_alloc, let the compiler create the memcpy for you. > > Good point! > > > > I don't see how volatile would be required here. Maybe it fixes the issue > > (so does adding a compiler barrier), but I feel uneasy just fixing it up > > within lttng-ust if there is a deeper issue in gcc 4.8. > > > > Thoughts ? > > > > Thanks, > > > > Mathieu > > > > > > > -- > Paul Woegerer, SW Development Engineer > Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer> > Mentor Graphics, Embedded Software Division > > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ lttng-dev mailing list lttng-dev@lists.lttng.org http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <A30AF42E15BD64459E202697468DFFA77E389F61@EU-MBX-04.mgc.mentorg.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <A30AF42E15BD64459E202697468DFFA77E389F61@EU-MBX-04.mgc.mentorg.com> @ 2014-02-13 22:44 ` Stefan Seefeld [not found] ` <52FD4AB0.4060203@mentor.com> 1 sibling, 0 replies; 28+ messages in thread From: Stefan Seefeld @ 2014-02-13 22:44 UTC (permalink / raw) To: Woegerer, Paul, Mathieu Desnoyers Cc: Seefeld, Stefan, lttng-dev, Paul E. McKenney On 02/13/2014 05:06 PM, Woegerer, Paul wrote: > Let me put it this way ... > > If (hypothetically, just for the sake of the argument) we would have dlsym with the following signature: > > void *dlsym(void *handle, const char *symbol, void *dummy); > > instead of: > > void *dlsym(void *handle, const char *symbol); > > and we would call it with: > > af.calloc = dlsym(RTLD_NEXT, "calloc", &cur_alloc); > > then (because of the aliasing of cur_alloc (caused by &cur_alloc) the compiler would be forced to store the effects done on cur_alloc into memory prior to calling dlsym. Paul, I'm not convinced. Our compilation unit defines a bunch of functions with external linkage, which access cur_alloc. And since gcc has no way to rule out that the call to dlsym() will not cause any of these functions to be called, it mustn't make any assumptions about whether or not the first initialization of cur_alloc is redundant or not, and thus shouldn't elide it. (The above is in fact quite a frequent idiom in C/C++ framework libraries. Just imagine dlsym() being a call into a GUI (such as an event loop), and the functions in this CU unit as callbacks.) Stefan -- Stefan Seefeld CodeSourcery / Mentor Graphics http://www.mentor.com/embedded-software/ ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <52FD4AB0.4060203@mentor.com>]
* Re: RFC: Fix crash in dlerror() [not found] ` <52FD4AB0.4060203@mentor.com> @ 2014-02-14 6:59 ` Woegerer, Paul 2014-02-14 10:30 ` Alexander Monakov [not found] ` <alpine.LNX.2.00.1402141420420.24828@monopod.intra.ispras.ru> 2 siblings, 0 replies; 28+ messages in thread From: Woegerer, Paul @ 2014-02-14 6:59 UTC (permalink / raw) To: Stefan Seefeld, Mathieu Desnoyers; +Cc: lttng-dev, Paul E. McKenney Good Morning Stefan, On 02/13/2014 11:44 PM, Stefan Seefeld wrote: > On 02/13/2014 05:06 PM, Woegerer, Paul wrote: >> Let me put it this way ... >> >> If (hypothetically, just for the sake of the argument) we would have dlsym with the following signature: >> >> void *dlsym(void *handle, const char *symbol, void *dummy); >> >> instead of: >> >> void *dlsym(void *handle, const char *symbol); >> >> and we would call it with: >> >> af.calloc = dlsym(RTLD_NEXT, "calloc", &cur_alloc); >> >> then (because of the aliasing of cur_alloc (caused by &cur_alloc) the compiler would be forced to store the effects done on cur_alloc into memory prior to calling dlsym. > > Paul, I'm not convinced. > > Our compilation unit defines a bunch of functions with external linkage, > which access cur_alloc. And since gcc has no way to rule out that the > call to dlsym() will not cause any of these functions to be called, it > mustn't make any assumptions about whether or not the first > initialization of cur_alloc is redundant or not, and thus shouldn't > elide it. Thinking about it again (with 7hrs of sleep in-between), I'm still not convinced that this is a compiler bug. See: https://www.kernel.org/doc/Documentation/memory-barriers.txt and search for "The compiler is within its rights to reorder memory accesses" and "the compiler is within its rights to omit a store entirely" Also related: http://stackoverflow.com/questions/5693274/is-function-call-a-memory-barrier http://stackoverflow.com/questions/10698253/is-function-call-an-effective-memory-barrier-for-modern-platforms Nonetheless, I suggest to replace the volatile fields (see my first patch) with ACCESS_ONCE() barriers as described in: https://www.kernel.org/doc/Documentation/memory-barriers.txt diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c index 8296ae2..44ce933 100644 --- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c +++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c @@ -173,17 +173,17 @@ static void setup_static_allocator(void) { assert(cur_alloc.calloc == NULL); - cur_alloc.calloc = static_calloc; + CMM_ACCESS_ONCE(cur_alloc.calloc) = static_calloc; assert(cur_alloc.malloc == NULL); - cur_alloc.malloc = static_malloc; + CMM_ACCESS_ONCE(cur_alloc.malloc) = static_malloc; assert(cur_alloc.free == NULL); - cur_alloc.free = static_free; + CMM_ACCESS_ONCE(cur_alloc.free) = static_free; assert(cur_alloc.realloc == NULL); - cur_alloc.realloc = static_realloc; + CMM_ACCESS_ONCE(cur_alloc.realloc) = static_realloc; assert(cur_alloc.memalign == NULL); - cur_alloc.memalign = static_memalign; + CMM_ACCESS_ONCE(cur_alloc.memalign) = static_memalign; assert(cur_alloc.posix_memalign == NULL); - cur_alloc.posix_memalign = static_posix_memalign; + CMM_ACCESS_ONCE(cur_alloc.posix_memalign) = static_posix_memalign; } static @@ -207,7 +207,7 @@ void lookup_all_symbols(void) af.posix_memalign = dlsym(RTLD_NEXT, "posix_memalign"); /* Populate the new allocator functions */ - memcpy(&cur_alloc, &af, sizeof(cur_alloc)); + cur_alloc = af; } void *malloc(size_t size) -- Many Thanks, Paul > > (The above is in fact quite a frequent idiom in C/C++ framework > libraries. Just imagine dlsym() being a call into a GUI (such as an > event loop), and the functions in this CU unit as callbacks.) > > Stefan > -- Paul Woegerer, SW Development Engineer Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer> Mentor Graphics, Embedded Software Division ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: RFC: Fix crash in dlerror() [not found] ` <52FD4AB0.4060203@mentor.com> 2014-02-14 6:59 ` Woegerer, Paul @ 2014-02-14 10:30 ` Alexander Monakov [not found] ` <alpine.LNX.2.00.1402141420420.24828@monopod.intra.ispras.ru> 2 siblings, 0 replies; 28+ messages in thread From: Alexander Monakov @ 2014-02-14 10:30 UTC (permalink / raw) To: Stefan Seefeld; +Cc: lttng-dev, Paul E. McKenney On Thu, 13 Feb 2014, Stefan Seefeld wrote: > Our compilation unit defines a bunch of functions with external linkage, > which access cur_alloc. And since gcc has no way to rule out that the > call to dlsym() will not cause any of these functions to be called, it > mustn't make any assumptions about whether or not the first > initialization of cur_alloc is redundant or not, and thus shouldn't > elide it. Stefan, LTTng developers, The problem here is that glibc declares dlsym() with __attribute__((leaf)) (see the definition of __THROW in /usr/include/sys/cdefs.h and the difference from __THROWNL). Presence of the attribute allows the compiler to assume that no functions from the current compilation unit will be called from dlsym, and thus there's no need to write back potentially escaping data. glibc used to have __THROW annotations on dlopen() as well, and they changed it to __THROWNL (removing the "leaf" attribute) after it was pointed out that dlopen will call constructors: https://sourceware.org/ml/libc-alpha/2013-08/msg00465.html Unfortunately when I pointed out that dlsym is not really "leaf" as well, my argument was dismissed: https://sourceware.org/ml/libc-alpha/2013-09/msg00012.html Please consider filing a glibc bug or otherwise reopening that discussion. Hope that helps, Alexander ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <alpine.LNX.2.00.1402141420420.24828@monopod.intra.ispras.ru>]
* Re: RFC: Fix crash in dlerror() [not found] ` <alpine.LNX.2.00.1402141420420.24828@monopod.intra.ispras.ru> @ 2014-02-14 11:35 ` Woegerer, Paul 2014-02-14 11:54 ` Woegerer, Paul ` (2 subsequent siblings) 3 siblings, 0 replies; 28+ messages in thread From: Woegerer, Paul @ 2014-02-14 11:35 UTC (permalink / raw) To: Alexander Monakov, Stefan Seefeld; +Cc: lttng-dev, Paul E. McKenney Thank you Alexander ! So Stefans reasoning was entirely correct but the definition of dlsym() lies about its effect on others. ...which makes GCC believe that there is no need to store the changes to the cur_alloc fields prior to calling dlsym(). So I agree with you that this is a glibc bug then. -- Best, Paul On 02/14/2014 11:30 AM, Alexander Monakov wrote: > On Thu, 13 Feb 2014, Stefan Seefeld wrote: >> Our compilation unit defines a bunch of functions with external linkage, >> which access cur_alloc. And since gcc has no way to rule out that the >> call to dlsym() will not cause any of these functions to be called, it >> mustn't make any assumptions about whether or not the first >> initialization of cur_alloc is redundant or not, and thus shouldn't >> elide it. > Stefan, LTTng developers, > > The problem here is that glibc declares dlsym() with __attribute__((leaf)) > (see the definition of __THROW in /usr/include/sys/cdefs.h and the difference > from __THROWNL). Presence of the attribute allows the compiler to assume that > no functions from the current compilation unit will be called from dlsym, and > thus there's no need to write back potentially escaping data. > > glibc used to have __THROW annotations on dlopen() as well, and they changed > it to __THROWNL (removing the "leaf" attribute) after it was pointed out that > dlopen will call constructors: > > https://sourceware.org/ml/libc-alpha/2013-08/msg00465.html > > Unfortunately when I pointed out that dlsym is not really "leaf" as well, my > argument was dismissed: > > https://sourceware.org/ml/libc-alpha/2013-09/msg00012.html > > Please consider filing a glibc bug or otherwise reopening that discussion. > > Hope that helps, > Alexander -- Paul Woegerer, SW Development Engineer Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer> Mentor Graphics, Embedded Software Division ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: RFC: Fix crash in dlerror() [not found] ` <alpine.LNX.2.00.1402141420420.24828@monopod.intra.ispras.ru> 2014-02-14 11:35 ` Woegerer, Paul @ 2014-02-14 11:54 ` Woegerer, Paul 2014-02-14 13:45 ` [PATCH] Force static_alloc setup to be written into memory Paul Woegerer [not found] ` <1392385505-13405-1-git-send-email-paul_woegerer@mentor.com> 3 siblings, 0 replies; 28+ messages in thread From: Woegerer, Paul @ 2014-02-14 11:54 UTC (permalink / raw) To: Alexander Monakov, Stefan Seefeld; +Cc: lttng-dev, Paul E. McKenney On 02/14/2014 11:30 AM, Alexander Monakov wrote: > On Thu, 13 Feb 2014, Stefan Seefeld wrote: >> Our compilation unit defines a bunch of functions with external linkage, >> which access cur_alloc. And since gcc has no way to rule out that the >> call to dlsym() will not cause any of these functions to be called, it >> mustn't make any assumptions about whether or not the first >> initialization of cur_alloc is redundant or not, and thus shouldn't >> elide it. > Stefan, LTTng developers, > > The problem here is that glibc declares dlsym() with __attribute__((leaf)) > (see the definition of __THROW in /usr/include/sys/cdefs.h and the difference > from __THROWNL). Presence of the attribute allows the compiler to assume that > no functions from the current compilation unit will be called from dlsym, and > thus there's no need to write back potentially escaping data. So the CMM_ACCESS_ONCE's I suggested would only be required if dlsym() would call back into the wrapper from another thread, right ? Thanks, Paul > > glibc used to have __THROW annotations on dlopen() as well, and they changed > it to __THROWNL (removing the "leaf" attribute) after it was pointed out that > dlopen will call constructors: > > https://sourceware.org/ml/libc-alpha/2013-08/msg00465.html > > Unfortunately when I pointed out that dlsym is not really "leaf" as well, my > argument was dismissed: > > https://sourceware.org/ml/libc-alpha/2013-09/msg00012.html > > Please consider filing a glibc bug or otherwise reopening that discussion. > > Hope that helps, > Alexander -- Paul Woegerer, SW Development Engineer Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer> Mentor Graphics, Embedded Software Division ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] Force static_alloc setup to be written into memory [not found] ` <alpine.LNX.2.00.1402141420420.24828@monopod.intra.ispras.ru> 2014-02-14 11:35 ` Woegerer, Paul 2014-02-14 11:54 ` Woegerer, Paul @ 2014-02-14 13:45 ` Paul Woegerer [not found] ` <1392385505-13405-1-git-send-email-paul_woegerer@mentor.com> 3 siblings, 0 replies; 28+ messages in thread From: Paul Woegerer @ 2014-02-14 13:45 UTC (permalink / raw) To: lttng-dev, mathieu.desnoyers, stefan_seefeld; +Cc: paulmck As explained by Alexander Monakov, dlsym() is defined to be pure, thus the compiler is allowed to assume that there is no need to write the changes performed by setup_static_allocator() into memory prior to calling dlsym(). The added cmm_barrier() forces the compiler to write the changes into memory. For more details refer to: http://lists.lttng.org/pipermail/lttng-dev/2014-February/022389.html Signed-off-by: Paul Woegerer <paul_woegerer@mentor.com> --- liblttng-ust-libc-wrapper/lttng-ust-malloc.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c index 8296ae2..7dd647f 100644 --- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c +++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c @@ -198,6 +198,9 @@ void lookup_all_symbols(void) */ setup_static_allocator(); + /* Dlsym is defined pure -> force static alloc into memory */ + cmm_barrier(); + /* Perform the actual lookups */ af.calloc = dlsym(RTLD_NEXT, "calloc"); af.malloc = dlsym(RTLD_NEXT, "malloc"); @@ -207,7 +210,7 @@ void lookup_all_symbols(void) af.posix_memalign = dlsym(RTLD_NEXT, "posix_memalign"); /* Populate the new allocator functions */ - memcpy(&cur_alloc, &af, sizeof(cur_alloc)); + cur_alloc = af; } void *malloc(size_t size) -- 1.8.5.2 ^ permalink raw reply related [flat|nested] 28+ messages in thread
[parent not found: <1392385505-13405-1-git-send-email-paul_woegerer@mentor.com>]
* Re: [PATCH] Force static_alloc setup to be written into memory [not found] ` <1392385505-13405-1-git-send-email-paul_woegerer@mentor.com> @ 2014-02-14 14:08 ` Mathieu Desnoyers 2014-02-14 14:23 ` Alexander Monakov [not found] ` <alpine.LNX.2.00.1402141816441.24828@monopod.intra.ispras.ru> 2 siblings, 0 replies; 28+ messages in thread From: Mathieu Desnoyers @ 2014-02-14 14:08 UTC (permalink / raw) To: Paul Woegerer; +Cc: stefan seefeld, lttng-dev, paulmck ----- Original Message ----- > From: "Paul Woegerer" <paul_woegerer@mentor.com> > To: lttng-dev@lists.lttng.org, "mathieu desnoyers" <mathieu.desnoyers@efficios.com>, "stefan seefeld" > <stefan_seefeld@mentor.com> > Cc: paulmck@linux.vnet.ibm.com > Sent: Friday, February 14, 2014 8:45:05 AM > Subject: [PATCH] Force static_alloc setup to be written into memory > > As explained by Alexander Monakov, dlsym() is defined to be pure, thus the > compiler is allowed to assume that there is no need to write the changes > performed by setup_static_allocator() into memory prior to calling dlsym(). > The added cmm_barrier() forces the compiler to write the changes into memory. > > For more details refer to: > http://lists.lttng.org/pipermail/lttng-dev/2014-February/022389.html > > Signed-off-by: Paul Woegerer <paul_woegerer@mentor.com> > --- > liblttng-ust-libc-wrapper/lttng-ust-malloc.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c > b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c > index 8296ae2..7dd647f 100644 > --- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c > +++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c > @@ -198,6 +198,9 @@ void lookup_all_symbols(void) > */ > setup_static_allocator(); > > + /* Dlsym is defined pure -> force static alloc into memory */ > + cmm_barrier(); > + > /* Perform the actual lookups */ > af.calloc = dlsym(RTLD_NEXT, "calloc"); > af.malloc = dlsym(RTLD_NEXT, "malloc"); > @@ -207,7 +210,7 @@ void lookup_all_symbols(void) > af.posix_memalign = dlsym(RTLD_NEXT, "posix_memalign"); Shouldn't we also add a cmm_barrier() between the last call to dlsym() and the stores to cur_alloc then, to ensure that the allocator is swapped only after all calls to dlsym ? Thanks, Mathieu > > /* Populate the new allocator functions */ > - memcpy(&cur_alloc, &af, sizeof(cur_alloc)); > + cur_alloc = af; > } > > void *malloc(size_t size) > -- > 1.8.5.2 > > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Force static_alloc setup to be written into memory [not found] ` <1392385505-13405-1-git-send-email-paul_woegerer@mentor.com> 2014-02-14 14:08 ` Mathieu Desnoyers @ 2014-02-14 14:23 ` Alexander Monakov [not found] ` <alpine.LNX.2.00.1402141816441.24828@monopod.intra.ispras.ru> 2 siblings, 0 replies; 28+ messages in thread From: Alexander Monakov @ 2014-02-14 14:23 UTC (permalink / raw) To: Paul Woegerer; +Cc: stefan_seefeld, lttng-dev, paulmck On Fri, 14 Feb 2014, Paul Woegerer wrote: > As explained by Alexander Monakov, dlsym() is defined to be pure, thus the > compiler is allowed to assume that there is no need to write the changes > performed by setup_static_allocator() into memory prior to calling dlsym(). > The added cmm_barrier() forces the compiler to write the changes into memory. > > For more details refer to: > http://lists.lttng.org/pipermail/lttng-dev/2014-February/022389.html If everyone here agrees that this is a workaround for a glibc bug, please add a note to that effect in the patch and please notify glibc upstream (again). FWIW, when toying with a similar code I implemented a different workaround along the lines of #define dlsym glibc_dlsym_proto_lies_about_leafness #include <dlfcn.h> #undef dlsym extern void *dlsym(void *, const char *); Thus avoiding the need to sprinkle unneeded compiler memory barriers in code. HTH Alexander ^ permalink raw reply [flat|nested] 28+ messages in thread
[parent not found: <alpine.LNX.2.00.1402141816441.24828@monopod.intra.ispras.ru>]
* Re: [PATCH] Force static_alloc setup to be written into memory [not found] ` <alpine.LNX.2.00.1402141816441.24828@monopod.intra.ispras.ru> @ 2014-02-14 14:39 ` Woegerer, Paul 2014-02-14 14:46 ` Stefan Seefeld 2014-02-14 15:12 ` Mathieu Desnoyers 2 siblings, 0 replies; 28+ messages in thread From: Woegerer, Paul @ 2014-02-14 14:39 UTC (permalink / raw) To: Alexander Monakov; +Cc: stefan_seefeld, lttng-dev, paulmck That's excellent ! Thanks, Paul On 02/14/2014 03:23 PM, Alexander Monakov wrote: > > On Fri, 14 Feb 2014, Paul Woegerer wrote: > >> As explained by Alexander Monakov, dlsym() is defined to be pure, thus the >> compiler is allowed to assume that there is no need to write the changes >> performed by setup_static_allocator() into memory prior to calling dlsym(). >> The added cmm_barrier() forces the compiler to write the changes into memory. >> >> For more details refer to: >> http://lists.lttng.org/pipermail/lttng-dev/2014-February/022389.html > If everyone here agrees that this is a workaround for a glibc bug, please add > a note to that effect in the patch and please notify glibc upstream (again). > > FWIW, when toying with a similar code I implemented a different workaround > along the lines of > > #define dlsym glibc_dlsym_proto_lies_about_leafness > #include <dlfcn.h> > #undef dlsym > > extern void *dlsym(void *, const char *); > > > Thus avoiding the need to sprinkle unneeded compiler memory barriers in code. > > HTH > Alexander -- Paul Woegerer, SW Development Engineer Sourcery Analyzer <http://go.mentor.com/sourceryanalyzer> Mentor Graphics, Embedded Software Division ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Force static_alloc setup to be written into memory [not found] ` <alpine.LNX.2.00.1402141816441.24828@monopod.intra.ispras.ru> 2014-02-14 14:39 ` Woegerer, Paul @ 2014-02-14 14:46 ` Stefan Seefeld 2014-02-14 15:12 ` Mathieu Desnoyers 2 siblings, 0 replies; 28+ messages in thread From: Stefan Seefeld @ 2014-02-14 14:46 UTC (permalink / raw) To: lttng-dev On 02/14/2014 09:23 AM, Alexander Monakov wrote: > > On Fri, 14 Feb 2014, Paul Woegerer wrote: > >> As explained by Alexander Monakov, dlsym() is defined to be pure, thus the >> compiler is allowed to assume that there is no need to write the changes >> performed by setup_static_allocator() into memory prior to calling dlsym(). >> The added cmm_barrier() forces the compiler to write the changes into memory. >> >> For more details refer to: >> http://lists.lttng.org/pipermail/lttng-dev/2014-February/022389.html > If everyone here agrees that this is a workaround for a glibc bug, please add > a note to that effect in the patch and please notify glibc upstream (again). I'v just submitted https://sourceware.org/bugzilla/show_bug.cgi?id=16585 > FWIW, when toying with a similar code I implemented a different workaround > along the lines of > > #define dlsym glibc_dlsym_proto_lies_about_leafness > #include <dlfcn.h> > #undef dlsym > > extern void *dlsym(void *, const char *); > > > Thus avoiding the need to sprinkle unneeded compiler memory barriers in code. Right, this looks very clean indeed. Stefan -- Stefan Seefeld CodeSourcery / Mentor Graphics http://www.mentor.com/embedded-software/ ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] Force static_alloc setup to be written into memory [not found] ` <alpine.LNX.2.00.1402141816441.24828@monopod.intra.ispras.ru> 2014-02-14 14:39 ` Woegerer, Paul 2014-02-14 14:46 ` Stefan Seefeld @ 2014-02-14 15:12 ` Mathieu Desnoyers 2 siblings, 0 replies; 28+ messages in thread From: Mathieu Desnoyers @ 2014-02-14 15:12 UTC (permalink / raw) To: Alexander Monakov; +Cc: stefan seefeld, lttng-dev, paulmck ----- Original Message ----- > From: "Alexander Monakov" <amonakov@ispras.ru> > To: "Paul Woegerer" <paul_woegerer@mentor.com> > Cc: lttng-dev@lists.lttng.org, "mathieu desnoyers" <mathieu.desnoyers@efficios.com>, "stefan seefeld" > <stefan_seefeld@mentor.com>, paulmck@linux.vnet.ibm.com > Sent: Friday, February 14, 2014 9:23:19 AM > Subject: Re: [lttng-dev] [PATCH] Force static_alloc setup to be written into memory > > > > On Fri, 14 Feb 2014, Paul Woegerer wrote: > > > As explained by Alexander Monakov, dlsym() is defined to be pure, thus the > > compiler is allowed to assume that there is no need to write the changes > > performed by setup_static_allocator() into memory prior to calling dlsym(). > > The added cmm_barrier() forces the compiler to write the changes into > > memory. > > > > For more details refer to: > > http://lists.lttng.org/pipermail/lttng-dev/2014-February/022389.html > > If everyone here agrees that this is a workaround for a glibc bug, please add > a note to that effect in the patch and please notify glibc upstream (again). > > FWIW, when toying with a similar code I implemented a different workaround > along the lines of > > #define dlsym glibc_dlsym_proto_lies_about_leafness > #include <dlfcn.h> > #undef dlsym > > extern void *dlsym(void *, const char *); > > > Thus avoiding the need to sprinkle unneeded compiler memory barriers in code. Good idea, this is what I did in the final fix. It's pushed as: commit f02baefb3ba4d5493816d63f65625ba4269224d2 Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Date: Fri Feb 14 10:02:51 2014 -0500 Fix: work-around glibc lying about dlsym()/dlerror() leafness Especially in the LTTng-UST malloc instrumentation, we run into the following situation: 1) Our calloc wrapper is called, 2) we setup the static allocator, 3) we call dlsym() to lookup the symbol of the real allocator, 4) dlsym() calls into calloc(), which is overridden by our own wrapper. Our calloc does not see that the static allocator has been set, because the stores setting up the static allocator have been optimized away by gcc-4.8 (in O2), because the dlsym() prototype declares it with the "leaf" attribute, and thus we end up doing an infinite recursion, and eventually a segmentation fault. Thanks to Alexander Monakov for pointing out the culprit of this glibc bug. Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Thanks for your input !! Mathieu > > HTH > Alexander > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 28+ messages in thread
* RFC: Fix crash in dlerror()
@ 2014-02-07 21:50 Stefan Seefeld
0 siblings, 0 replies; 28+ messages in thread
From: Stefan Seefeld @ 2014-02-07 21:50 UTC (permalink / raw)
To: lttng-dev
[-- Attachment #1: Type: text/plain, Size: 1216 bytes --]
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.
Thanks,
Stefan
--
Stefan Seefeld
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 805 bytes --]
diff --git a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
index 33ed18b..3f391db 100644
--- a/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
+++ b/liblttng-ust-libc-wrapper/lttng-ust-malloc.c
@@ -54,6 +54,14 @@ static void *static_calloc(size_t nmemb, size_t size)
return &static_calloc_buf[prev_offset];
}
+static void *(*plibc_realloc)(void *ptr, size_t size);
+
+__attribute__((constructor))
+static void init()
+{
+ plibc_realloc = dlsym(RTLD_NEXT, "realloc");
+}
+
void *malloc(size_t size)
{
static void *(*plibc_malloc)(size_t size);
@@ -119,7 +127,6 @@ void *calloc(size_t nmemb, size_t size)
void *realloc(void *ptr, size_t size)
{
- static void *(*plibc_realloc)(void *ptr, size_t size);
void *retval;
if (plibc_realloc == NULL) {
[-- Attachment #3: Type: text/plain, Size: 155 bytes --]
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
^ permalink raw reply related [flat|nested] 28+ messages in threadend of thread, other threads:[~2014-02-14 15:12 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <52F55511.2080309@mentor.com>
2014-02-08 16:06 ` RFC: Fix crash in dlerror() Mathieu Desnoyers
[not found] ` <1380151240.21051.1391875592403.JavaMail.zimbra@efficios.com>
2014-02-08 16:53 ` Stefan Seefeld
[not found] ` <52F66118.5010003@mentor.com>
2014-02-08 22:22 ` Mathieu Desnoyers
[not found] ` <905376946.21151.1391898148330.JavaMail.zimbra@efficios.com>
2014-02-09 16:28 ` Stefan Seefeld
[not found] ` <52F7ACB6.4060907@mentor.com>
2014-02-11 0:31 ` Mathieu Desnoyers
2014-02-11 0:53 ` Stefan Seefeld
[not found] ` <52F9746F.5070302@mentor.com>
2014-02-11 20:51 ` Mathieu Desnoyers
[not found] ` <1494953964.23233.1392151900002.JavaMail.zimbra@efficios.com>
2014-02-11 20:55 ` Stefan Seefeld
[not found] ` <52FA8E5C.40203@mentor.com>
2014-02-12 3:39 ` Mathieu Desnoyers
[not found] ` <975853026.23546.1392176395828.JavaMail.zimbra@efficios.com>
2014-02-12 14:35 ` Mathieu Desnoyers
[not found] ` <2141704629.23782.1392215703555.JavaMail.zimbra@efficios.com>
2014-02-12 21:59 ` Mathieu Desnoyers
[not found] ` <52FCAA28.6020708@mentor.com>
[not found] ` <1026072798.24303.1392297452496.JavaMail.zimbra@efficios.com>
[not found] ` <52FCCCD2.9050302@mentor.com>
[not found] ` <610029715.24333.1392300717731.JavaMail.zimbra@efficios.com>
[not found] ` <1692042945.24342.1392301795996.JavaMail.zimbra@efficios.com>
[not found] ` <1119459836.24348.1392302831554.JavaMail.zimbra@efficios.com>
[not found] ` <52FCE732.9090508@mentor.com>
2014-02-13 16:40 ` Mathieu Desnoyers
[not found] ` <581364674.24417.1392309613306.JavaMail.zimbra@efficios.com>
2014-02-13 16:51 ` Woegerer, Paul
[not found] ` <52FCF7F5.9070908@mentor.com>
2014-02-13 18:52 ` Mathieu Desnoyers
[not found] ` <934762932.24558.1392317539645.JavaMail.zimbra@efficios.com>
2014-02-13 19:44 ` Woegerer, Paul
[not found] ` <A30AF42E15BD64459E202697468DFFA77E387F33@EU-MBX-04.mgc.mentorg.com>
2014-02-13 22:06 ` Woegerer, Paul
[not found] ` <A30AF42E15BD64459E202697468DFFA77E389F61@EU-MBX-04.mgc.mentorg.com>
2014-02-13 22:44 ` Stefan Seefeld
[not found] ` <52FD4AB0.4060203@mentor.com>
2014-02-14 6:59 ` Woegerer, Paul
2014-02-14 10:30 ` Alexander Monakov
[not found] ` <alpine.LNX.2.00.1402141420420.24828@monopod.intra.ispras.ru>
2014-02-14 11:35 ` Woegerer, Paul
2014-02-14 11:54 ` Woegerer, Paul
2014-02-14 13:45 ` [PATCH] Force static_alloc setup to be written into memory Paul Woegerer
[not found] ` <1392385505-13405-1-git-send-email-paul_woegerer@mentor.com>
2014-02-14 14:08 ` Mathieu Desnoyers
2014-02-14 14:23 ` Alexander Monakov
[not found] ` <alpine.LNX.2.00.1402141816441.24828@monopod.intra.ispras.ru>
2014-02-14 14:39 ` Woegerer, Paul
2014-02-14 14:46 ` Stefan Seefeld
2014-02-14 15:12 ` Mathieu Desnoyers
2014-02-07 21:50 RFC: Fix crash in dlerror() Stefan Seefeld
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.