Re: RFC: Fix crash in dlerror()

Re: RFC: Fix crash in dlerror()

[Mathieu Desnoyers]

----- 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

Re: RFC: Fix crash in dlerror()

[Stefan Seefeld]

[-- 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

Re: RFC: Fix crash in dlerror()

[Mathieu Desnoyers]

[-- 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

Re: RFC: Fix crash in dlerror()

[Stefan Seefeld]

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/

Re: RFC: Fix crash in dlerror()

[Mathieu Desnoyers]

----- 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

Re: RFC: Fix crash in dlerror()

[Stefan Seefeld]

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/

Re: RFC: Fix crash in dlerror()

[Mathieu Desnoyers]

[-- 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

Re: RFC: Fix crash in dlerror()

[Stefan Seefeld]

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/

Re: RFC: Fix crash in dlerror()

[Mathieu Desnoyers]

[-- 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

Re: RFC: Fix crash in dlerror()

[Mathieu Desnoyers]

[-- 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

Re: RFC: Fix crash in dlerror()

[Mathieu Desnoyers]

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

Re: RFC: Fix crash in dlerror()

[Mathieu Desnoyers]

(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

Re: RFC: Fix crash in dlerror()

[Woegerer, Paul]

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

Re: RFC: Fix crash in dlerror()

[Mathieu Desnoyers]

----- 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

Re: RFC: Fix crash in dlerror()

[Woegerer, Paul]

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

Re: RFC: Fix crash in dlerror()

[Woegerer, Paul]

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

Re: RFC: Fix crash in dlerror()

[Stefan Seefeld]

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/

Re: RFC: Fix crash in dlerror()

[Woegerer, Paul]

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

Re: RFC: Fix crash in dlerror()

[Alexander Monakov]

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

Re: RFC: Fix crash in dlerror()

[Woegerer, Paul]

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

Re: RFC: Fix crash in dlerror()

[Woegerer, 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.

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

[PATCH] Force static_alloc setup to be written into memory

[Paul Woegerer]

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

Re: [PATCH] Force static_alloc setup to be written into memory

[Mathieu Desnoyers]

----- 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

Re: [PATCH] Force static_alloc setup to be written into memory

[Alexander Monakov]

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

Re: [PATCH] Force static_alloc setup to be written into memory

[Woegerer, Paul]

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

Re: [PATCH] Force static_alloc setup to be written into memory

[Stefan Seefeld]

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/

Re: [PATCH] Force static_alloc setup to be written into memory

[Mathieu Desnoyers]

----- 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

RFC: Fix crash in dlerror()

[Stefan Seefeld]

[-- 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