Date: Fri, 29 Aug 2008 22:10:05 GMT From: "Kip Macy" <kmacy@freebsd.org> To: freebsd-threads@FreeBSD.org Subject: Re: threads/126950: rtld malloc is thread-unsafe Message-ID: <200808292210.m7TMA53f019709@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR threads/126950; it has been noted by GNATS.
From: "Kip Macy" <kmacy@freebsd.org>
To: "Oleg Dolgov" <agile@sunbay.com>
Cc: freebsd-gnats-submit@freebsd.org
Subject: Re: threads/126950: rtld malloc is thread-unsafe
Date: Fri, 29 Aug 2008 14:41:59 -0700
I think the real solution is to serialize calls to dlopen. Please
consider submitting a patch for that.
-Kip
On Fri, Aug 29, 2008 at 7:48 AM, Oleg Dolgov <agile@sunbay.com> wrote:
>
>>Number: 126950
>>Category: threads
>>Synopsis: rtld malloc is thread-unsafe
>>Confidential: no
>>Severity: serious
>>Priority: medium
>>Responsible: freebsd-threads
>>State: open
>>Quarter:
>>Keywords:
>>Date-Required:
>>Class: sw-bug
>>Submitter-Id: current-users
>>Arrival-Date: Fri Aug 29 14:50:08 UTC 2008
>>Closed-Date:
>>Last-Modified:
>>Originator: Oleg Dolgov
>>Release: 7.0-RELEASE
>>Organization:
> Sunbay
>>Environment:
>>Description:
> rtld internal memory allocator are not thread safe. It use global array 'nextf' of free blocks. Dynamic loader can be easily segfaulted with parallel invocations of rtld operations from multiple threads. Test case and fix for problems exposed by the test are attached.
>
> precondition: apply patch kern/95339 (fixes for dlopen mt behavior)
>
>>How-To-Repeat:
> Use the following test by Kazuaki Oda <kaakun at highway dot ne dot jp>
> to see errant behaviour, but first apply patch from kern/95339.
>
> #include <err.h>
> #include <dlfcn.h>
> #include <pthread.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
>
> #define NTHREADS 10
>
> void *func(void *dummy);
>
> int main(void)
> {
> pthread_t tids[NTHREADS];
> int error;
> int i;
>
> for (i = 0; i < NTHREADS; i++) {
> error = pthread_create(&tids[i], NULL, func, NULL);
> if (error)
> errc(1, error, "pthread_create");
> }
>
> for (;;)
> sleep(1);
>
> /* NOTREACHED */
>
> exit(0);
> }
>
> void *func(void *dummy)
> {
> void *h;
> unsigned long c = 0;
>
> for (;;) {
> if ((h = dlopen("/usr/lib/libm.so", RTLD_NOW)) == NULL) {
> fprintf(stderr, "dlopen: %s\n", dlerror());
> continue;
> }
> if (dlclose(h) == -1)
> fprintf(stderr, "dlclose: %s\n", dlerror());
> if (c++ == 10000) {
> printf(".\n");
> c = 0;
> }
> }
>
> /* NOTREACHED */
>
> return (NULL);
> }
>>Fix:
> We need synchronization, but cant use rtld_bind_lock or similar because memory allocation even occurred from _rtld (init_rtld) function (and lock's space also allocated with malloc), in context, where dynamic linker itself has not been relocated yet.
>
>
> Patch attached with submission follows:
>
> --- rtld-elf/malloc.c 2008-08-29 16:18:17.000000000 +0300
> +++ /usr/src/libexec/rtld-elf/malloc.c 2008-08-29 17:42:38.000000000 +0300
> @@ -58,6 +58,8 @@
> #include <unistd.h>
> #include <sys/param.h>
> #include <sys/mman.h>
> +#include <machine/atomic.h>
> +
> #ifndef BSD
> #define MAP_COPY MAP_PRIVATE
> #define MAP_FILE 0
> @@ -68,6 +70,10 @@
> #define NEED_DEV_ZERO 1
> #endif
>
> +static volatile u_int mem_mtx = 0;
> +static void mtx_acquire();
> +static void mtx_release();
> +
> static void morecore();
> static int findbucket();
>
> @@ -152,8 +158,8 @@
> static void xprintf(const char *, ...);
> #define TRACE() xprintf("TRACE %s:%d\n", __FILE__, __LINE__)
>
> -void *
> -malloc(nbytes)
> +static void *
> +__malloc(nbytes)
> size_t nbytes;
> {
> register union overhead *op;
> @@ -299,8 +305,8 @@
> }
> }
>
> -void
> -free(cp)
> +static void
> +__free(cp)
> void *cp;
> {
> register int size;
> @@ -341,8 +347,8 @@
> */
> int realloc_srchlen = 4; /* 4 should be plenty, -1 =>'s whole list */
>
> -void *
> -realloc(cp, nbytes)
> +static void *
> +__realloc(cp, nbytes)
> void *cp;
> size_t nbytes;
> {
> @@ -516,3 +522,49 @@
> (void)write(STDOUT_FILENO, buf, strlen(buf));
> va_end(ap);
> }
> +
> +/*
> + * Thread safe allocator
> + */
> +
> +static void mtx_acquire()
> +{
> + for ( ; ; ) {
> + if (atomic_cmpset_acq_int(&mem_mtx, 0, 1))
> + break;
> + }
> +}
> +
> +static void mtx_release()
> +{
> + atomic_add_rel_int(&mem_mtx, -1);
> +}
> +
> +void *
> +malloc(size_t nbytes)
> +{
> + void *p;
> + mtx_acquire();
> + p = __malloc(nbytes);
> + mtx_release();
> + return p;
> +}
> +
> +void *
> +realloc(void *cp, size_t nbytes)
> +{
> + void *p;
> + mtx_acquire();
> + p = __realloc(cp, nbytes);
> + mtx_release();
> + return p;
> +}
> +
> +void
> +free(void *cp)
> +{
> + mtx_acquire();
> + __free(cp);
> + mtx_release();
> +}
> +
>
>
>>Release-Note:
>>Audit-Trail:
>>Unformatted:
> _______________________________________________
> freebsd-threads@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-threads
> To unsubscribe, send any mail to "freebsd-threads-unsubscribe@freebsd.org"
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200808292210.m7TMA53f019709>