Bug #2305

dmalloc calls malloc before malloc_init constructor called.

Added by dragonflybsd1 almost 3 years ago. Updated over 2 years ago.

Status:ClosedStart date:02/11/2012
Priority:NormalDue date:
Assignee:me1% Done:

0%

Category:-
Target version:-

Description

During the development of the .preinit_array ELF section support, it was discovered that a test passed on i386 but failed on x86_64 regardless if the binary was statically or dynamically linked. The program segfaulted on the first preinit function in the dmalloc function.

i386 uses nmalloc rather than dmalloc and the preinit test case passes just fine on that architecture.

The test case is here: http://leaf.dragonflybsd.org/~marino/preinit.c
The backtrace:
Program received signal SIGSEGV, Segmentation fault.
0x0000000800891269 in slaballoc (chunk_size=<optimized out>, chunking=<optimized out>, zi=<optimized out>)
at /usr/src/lib/libc/../libc/stdlib/dmalloc.c:1069
1069 while ((slab = zinfo->avail_base) != NULL) {
#0 0x0000000800891269 in slaballoc (chunk_size=<optimized out>, chunking=<optimized out>, zi=<optimized out>)
at /usr/src/lib/libc/../libc/stdlib/dmalloc.c:1069
#1 memalloc (size=4096, flags=0) at /usr/src/lib/libc/../libc/stdlib/dmalloc.c:744
#2 0x0000000800891cf1 in malloc (size=1344) at /usr/src/lib/libc/../libc/stdlib/dmalloc.c:593
#3 0x00000008008fe89b in __smakebuf (fp=0x800b2e850) at /usr/src/lib/libc/../libc/stdio/makebuf.c:70
#4 0x00000008008fe74f in __swsetup (fp=0x800b2e850) at /usr/src/lib/libc/../libc/stdio/wsetup.c:79
#5 0x00000008008fde15 in __sfvwrite (fp=0x800b2e850, uio=0x0) at /usr/src/lib/libc/../libc/stdio/fvwrite.c:62
#6 0x00000008008e6473 in puts (s=0x540 <Address 0x540 out of bounds>) at /usr/src/lib/libc/../libc/stdio/puts.c:65
#7 0x0000000000400742 in preinit_0 () at preinit.c:6
#8 0x000000080060ba71 in preinitialize_main_object () at /usr/src/libexec/rtld-elf/rtld.c:2051
#9 _rtld_call_init () at /usr/src/libexec/rtld-elf/rtld.c:703
#10 0x00000000004005ba in _start (ap=<optimized out>, cleanup=0x80060d65b <rtld_exit>)
at /usr/src/lib/csu/x86_64/crt1.c:96

History

#1 Updated by dragonflybsd1 almost 3 years ago

I forgot: To test this, one must have world built at least to commit b28bf640312db2b299faff75052fbb01d67fd821
Sat, 11 Feb 2012 20:04:12 +0000 (21:04 +0100)
rtld: Add support for preinit, init, and fini arrays

#2 Updated by vsrinivas over 2 years ago

dmalloc is unhappy because the preinit code is calling malloc() before the malloc_init constructor has had a chance to run.

malloc_init() for dmalloc is called by a reserved-priority constructor; that call is set to happen after the preinit vector is executed, however. nmalloc is okay because malloc_init does nothing of urgent need there; dmalloc uses it to call its own (misnamed) _nmalloc_thr_init() for the first thread. [nmalloc calls that from the thread library and doesn't need it in the first thread].

Possible fixes:
* Have dmalloc check if it has run malloc_init, and if not, run it. {remember to interlock correctly!}
* Force malloc's initialization to happen before preinit
* something else?

#3 Updated by dragonflybsd1 over 2 years ago

Which fix do you recommend?
Or to put it another way: Who do you want to answer this question? It seems to me that you are more qualified than most...

#4 Updated by vsrinivas over 2 years ago

--- /usr/src/lib/libc/stdlib/dmalloc.c 2012-05-10 17:31:04.234525000 -0700
+++ dmalloc.c 2012-05-18 22:50:07.624198000 -0700
@@ -303,6 +303,7 @@
static int opt_utrace = 0;
static int g_malloc_flags = 0;
static int malloc_panic;
+static int malloc_started = 0;

static const int32_t weirdary[16] = {
WEIRD_ADDR, WEIRD_ADDR, WEIRD_ADDR, WEIRD_ADDR,
@@ -320,11 +321,7 @@
static void *_vmem_alloc(int ri, size_t slab_size);
static void _vmem_free(void *ptr, size_t slab_size);
static void _mpanic(const char *ctl, ...) __printflike(1, 2);
-#ifndef STANDALONE_DEBUG
-static void malloc_init(void) __constructor(0);
-#else
-static void malloc_init(void) __constructor(101);
-#endif
+static void malloc_init(void);

struct nmalloc_utrace {
void *p;
@@ -353,6 +350,18 @@
malloc_init(void)
{
const char *p = NULL;
+ static spinlock_t malloc_init_lock;
+
+ if (malloc_started)
+ return;
+
+ if (__isthreaded) {
+ _SPINLOCK(&malloc_init_lock);
+ if (malloc_started) {
+ _SPINUNLOCK(&malloc_init_lock);
+ return;
+ }
+ }

Regions[0].mask = -1; /* disallow activity in lowest region */

@@ -399,6 +408,10 @@

UTRACE((void *) -1, 0, NULL);
_nmalloc_thr_init();
+ malloc_started = 1;
+
+ if (__isthreaded)
+ _SPINUNLOCK(&malloc_init_lock);
}

/*
@@ -712,6 +725,9 @@
size_t off;
char *obj;

+ if (!malloc_started)
+ malloc_init();
+
/*
* If 0 bytes is requested we have to return a unique pointer, allocate
* at least one byte.

Is a diff that does solve this problem (and is tested). However, it adds a (hopefully) minor cost to malloc() calls, a test of the malloc_started variable. I'd like to apply it after some review, but other opinions welcome.

#5 Updated by vsrinivas over 2 years ago

The patch was premature; it had malloc_init happen before libpthread_init in threaded programs, which is not acceptable...

#6 Updated by vsrinivas over 2 years ago

  • Status changed from New to Resolved

I believe commit e12d3396c777165504d60d2a1408dcd7cb63660d solves this bug. The preinit.c test case now passes, both in threaded and unthreaded environments.

#7 Updated by vsrinivas over 2 years ago

  • Status changed from Resolved to In Progress

Spoke too soon. The commit broke libc's interactions with libpthread programs on x86-64 and was quickly reverted. (pthreaded programs which used preinit functions which allocated memory were okay.)

The problem with the earlier commit was -- if libpthread's init code (_libpthread_init) was called via an attribute constructor before malloc was initialized, it would try to set up a redzone for thread 0 and then call thr_alloc, which would try _nmalloc_thr_init. _nmalloc_thr_init itself would then try to initialize libpthread, which would fail on the second attempt to set up the redzone.

#8 Updated by vsrinivas over 2 years ago

The straightforward fix is to apply the original commit,
(http://gitweb.dragonflybsd.org/dragonfly.git/blobdiff_plain/7a7e10d6267ef12bf6e84d69a9e77fec14d6dccb..e12d3396c777165504d60d2a1408dcd7cb63660d:/lib/libc/stdlib/dmalloc.c), however do not remove the __constructor(0) attribute on malloc_init.

This will force malloc_init to always run before pthread init; initialization works correctly when malloc is set up before pthreads, as malloc will not force pthreads to be set up before it is safe. This is tested in full-system builds, with both threaded and unthreaded programs, with and without preinit arrays.

There is one concern which has been raised -- the addition of a conditional (if (malloc_started == 0) malloc_init(); adds a cost to allocations and frees. I don't think the cost is significant, as this branch is going to predict as 'not taken' roughly all the time (except for once), but it has been brought up. An alternate approach would be to add the notion of 'preconstructors' to RTLD and to execute malloc initialization and pthread initialization from there.

#9 Updated by marino over 2 years ago

  • Status changed from In Progress to Closed

Venkatesh agrees this should be closed now.

Also available in: Atom PDF