Message ID | 20210617103217.2633690-1-siddhesh@sourceware.org |
---|---|
State | New |
Headers | show |
Series | malloc: Ensure that ptmalloc_init runs only once | expand |
* Siddhesh Poyarekar via Libc-alpha: > It is possible that multiple threads simultaneously enter > ptmalloc_init and succeed the < 0 check. Make the comparison and > setting of __malloc_initialized atomic so that only one of them goes > through. Additionally, if a thread sees that another thread is > running the initialization (i.e. __malloc_initialized == 0) then wait > till it is done. No, this cannot happen because pthread_create calls malloc before creating the new thread. Thanks, Florian
On 6/17/21 8:01 PM, Florian Weimer via Libc-alpha wrote: > * Siddhesh Poyarekar via Libc-alpha: > >> It is possible that multiple threads simultaneously enter >> ptmalloc_init and succeed the < 0 check. Make the comparison and >> setting of __malloc_initialized atomic so that only one of them goes >> through. Additionally, if a thread sees that another thread is >> running the initialization (i.e. __malloc_initialized == 0) then wait >> till it is done. > > No, this cannot happen because pthread_create calls malloc before > creating the new thread. Yes but I wonder if we should rely on that. If we decide to rely on this semantic then we implicitly specify thread creation through methods other than pthread_create that happen to not call malloc as unsupported. AFAICT it's not just a QOI issue; it may not be safe to call malloc_init_state twice on the same arena through different threads. Siddhesh
* Siddhesh Poyarekar: > On 6/17/21 8:01 PM, Florian Weimer via Libc-alpha wrote: >> * Siddhesh Poyarekar via Libc-alpha: >> >>> It is possible that multiple threads simultaneously enter >>> ptmalloc_init and succeed the < 0 check. Make the comparison and >>> setting of __malloc_initialized atomic so that only one of them goes >>> through. Additionally, if a thread sees that another thread is >>> running the initialization (i.e. __malloc_initialized == 0) then wait >>> till it is done. >> No, this cannot happen because pthread_create calls malloc before >> creating the new thread. > > Yes but I wonder if we should rely on that. If we decide to rely on > this semantic then we implicitly specify thread creation through > methods other than pthread_create that happen to not call malloc as > unsupported. There is no way to allocate a TCB for such foreign threads, and malloc depends on the TCB, so this is not something that can work for completely different reasons. THanks, Florian
On 6/18/21 11:15 AM, Florian Weimer wrote: > There is no way to allocate a TCB for such foreign threads, and malloc > depends on the TCB, so this is not something that can work for > completely different reasons. That's a fair point, thanks for reviewing this. In that case, doesn't it make sense to simplify __malloc_initialized to a bool and have it set at ptmalloc_init entry? Siddhesh
* Siddhesh Poyarekar: > On 6/18/21 11:15 AM, Florian Weimer wrote: >> There is no way to allocate a TCB for such foreign threads, and malloc >> depends on the TCB, so this is not something that can work for >> completely different reasons. > > That's a fair point, thanks for reviewing this. In that case, doesn't > it make sense to simplify __malloc_initialized to a bool and have it > set at ptmalloc_init entry? I think it's a three-state value to avoid calling __malloc_initialize_hook multiple times. This is all a bit under-documented unfortunately. Thanks, Florian
On 6/18/21 11:25 AM, Florian Weimer wrote: > I think it's a three-state value to avoid calling > __malloc_initialize_hook multiple times. This is all a bit > under-documented unfortunately. How so? It's only called from ptmalloc_init now AFAICT and running ptmalloc_init concurrently won't work anyway. Siddhesh
On 6/18/21 11:58 AM, Siddhesh Poyarekar via Libc-alpha wrote: > On 6/18/21 11:25 AM, Florian Weimer wrote: >> I think it's a three-state value to avoid calling >> __malloc_initialize_hook multiple times. This is all a bit >> under-documented unfortunately. > > How so? It's only called from ptmalloc_init now AFAICT and running > ptmalloc_init concurrently won't work anyway. To be clear what I'm suggesting is setting __malloc_initialized at the entry of ptmalloc_init, which should ensure that the body is executed only once as long as it is not called concurrently. Siddhesh
* Siddhesh Poyarekar: > On 6/18/21 11:25 AM, Florian Weimer wrote: >> I think it's a three-state value to avoid calling >> __malloc_initialize_hook multiple times. This is all a bit >> under-documented unfortunately. > > How so? It's only called from ptmalloc_init now AFAICT and running > ptmalloc_init concurrently won't work anyway. The sequence would be something like this: application calls malloc, ptmalloc_init is called via malloc_hook_ini, that invokes __malloc_initialize_hook, and that calls realloc, which then calls ptmalloc_init again via realloc_hook_ini. But since the __malloc_initialize_hook call is the very last thing called from ptmalloc_init, a boolean flag should be enough. Thanks, Florian
diff --git a/malloc/arena.c b/malloc/arena.c index 7eb110445e..3cd4391f25 100644 --- a/malloc/arena.c +++ b/malloc/arena.c @@ -290,10 +290,16 @@ libc_hidden_proto (_dl_open_hook); static void ptmalloc_init (void) { - if (__malloc_initialized >= 0) - return; + int oldval = catomic_compare_and_exchange_val_acq (&__malloc_initialized, + -1, 0); - __malloc_initialized = 0; + if (oldval == 1) + return; + else if (oldval == 0) + { + while (__malloc_initialized != 1); + return; + } #ifdef USE_MTAG if ((TUNABLE_GET_FULL (glibc, mem, tagging, int32_t, NULL) & 1) != 0) diff --git a/malloc/malloc.c b/malloc/malloc.c index 0e2e1747e0..cc3d1f41e8 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -3562,7 +3562,7 @@ libc_hidden_def (__libc_memalign) void * __libc_valloc (size_t bytes) { - if (__malloc_initialized < 0) + if (__malloc_initialized <= 0) ptmalloc_init (); void *address = RETURN_ADDRESS (0); @@ -3573,7 +3573,7 @@ __libc_valloc (size_t bytes) void * __libc_pvalloc (size_t bytes) { - if (__malloc_initialized < 0) + if (__malloc_initialized <= 0) ptmalloc_init (); void *address = RETURN_ADDRESS (0); @@ -5077,7 +5077,7 @@ __malloc_trim (size_t s) { int result = 0; - if (__malloc_initialized < 0) + if (__malloc_initialized <= 0) ptmalloc_init (); mstate ar_ptr = &main_arena; @@ -5212,7 +5212,7 @@ __libc_mallinfo2 (void) struct mallinfo2 m; mstate ar_ptr; - if (__malloc_initialized < 0) + if (__malloc_initialized <= 0) ptmalloc_init (); memset (&m, 0, sizeof (m)); @@ -5263,7 +5263,7 @@ __malloc_stats (void) mstate ar_ptr; unsigned int in_use_b = mp_.mmapped_mem, system_b = in_use_b; - if (__malloc_initialized < 0) + if (__malloc_initialized <= 0) ptmalloc_init (); _IO_flockfile (stderr); int old_flags2 = stderr->_flags2; @@ -5432,7 +5432,7 @@ __libc_mallopt (int param_number, int value) mstate av = &main_arena; int res = 1; - if (__malloc_initialized < 0) + if (__malloc_initialized <= 0) ptmalloc_init (); __libc_lock_lock (av->mutex); @@ -5691,7 +5691,7 @@ __malloc_info (int options, FILE *fp) - if (__malloc_initialized < 0) + if (__malloc_initialized <= 0) ptmalloc_init (); fputs ("<malloc version=\"1\">\n", fp);