[Patch] [BZ 13939] malloc deadlock
Jeff Law
law@redhat.com
Fri Aug 10 04:11:00 GMT 2012
On 07/02/2012 03:29 PM, Maxim Kuvyrkov wrote:
> On 3/07/2012, at 9:21 AM, Jeff Law wrote:
>
>> On 07/02/2012 02:00 PM, Maxim Kuvyrkov wrote:
>>>
>>> I don't recommend going this way as it is error-prone for bugs from
>>> future changes. A reasonable programmer would expect ar_ptr->mutex
>>> to lock the entirety of ar_ptr, including ar_ptr->next.
>> Precisely. Thus if we're trying to be robust WRT future changes and avoiding problems with ar->next changing after the lock is released but before we call arena_get2 (which is what it looks like libc_memalign is trying to do), then we need to get the value from ar->next before we release the lock.
>>
>> Note that libc_malloc and libc_calloc's current implementations can't race on ar->next because the lock is still held. Of course it's the holding of that lock which leads to the deadlock that we're trying to fix right now.
>>
>> libc_pvalloc and libc_valloc's current implementation would have problems if there are races with writes to ar->next as they unlock the arena then blindly use the current value of ar->next.
>
> I see now, you are right again. I would appreciate you adding a comment to one of the instances of "prev = ar_ptr->next" saying something like "Get ar_ptr->next before releasing the lock.".
You asked for a header comment on reused_arena. Done.
You asked for avoiding special casing the main_arena by passing in the
arena to be avoided. Done.
You asked for a comment WRT getting the arena's next pointer prior to
releasing its lock (to avoid potential races on the arena's next
pointer). Done.
OK?
Thanks,
Jeff
-------------- next part --------------
2012-08-09 Jeff Law <law@redhat.com>
[BZ #13939]
* malloc.c/arena.c (reused_arena): New parameter, avoid_arena.
When avoid_arena is set, don't retry in the that arena. Pick the
next one, whatever it might be.
(arena_get2): New parameter avoid_arena, pass through to reused_arena.
(arena_lock): Pass in new parameter to arena_get2.
* malloc/malloc.c (__libc_memalign): Pass in new parameter to
arena_get2.
(__libc_malloc): Unify retrying after main arena failure with
__libc_memalign version.
(__libc_valloc, __libc_pvalloc, __libc_calloc): Likewise.
diff --git a/NEWS b/NEWS
index eae9834..bc983c7 100644
--- a/NEWS
+++ b/NEWS
@@ -9,8 +9,8 @@ Version 2.17
* The following bugs are resolved with this release:
- 6778, 6808, 13717, 14042, 14166, 14150, 14151, 14154, 14157, 14173, 14283,
- 14298, 14307, 14328, 14331, 14336, 14337, 14347, 14349
+ 6778, 6808, 13717, 13939, 14042, 14166, 14150, 14151, 14154, 14157, 14173,
+ 14283, 14298, 14307, 14328, 14331, 14336, 14337, 14347, 14349
* Support for STT_GNU_IFUNC symbols added for s390 and s390x.
Optimized versions of memcpy, memset, and memcmp added for System z10 and
diff --git a/malloc/arena.c b/malloc/arena.c
index 33c4ff3..7270bbe 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -120,14 +120,14 @@ int __malloc_initialized = -1;
if(ptr) \
(void)mutex_lock(&ptr->mutex); \
else \
- ptr = arena_get2(ptr, (size)); \
+ ptr = arena_get2(ptr, (size), NULL); \
} while(0)
#else
# define arena_lock(ptr, size) do { \
if(ptr && !mutex_trylock(&ptr->mutex)) { \
THREAD_STAT(++(ptr->stat_lock_direct)); \
} else \
- ptr = arena_get2(ptr, (size)); \
+ ptr = arena_get2(ptr, (size), NULL); \
} while(0)
#endif
@@ -778,9 +778,11 @@ get_free_list (void)
return result;
}
-
+/* Lock and return an arena that can be reused for memory allocation.
+ Avoid AVOID_ARENA as we have already failed to allocate memory in
+ it and it is currently locked. */
static mstate
-reused_arena (void)
+reused_arena (mstate avoid_arena)
{
mstate result;
static mstate next_to_use;
@@ -797,6 +799,11 @@ reused_arena (void)
}
while (result != next_to_use);
+ /* Avoid AVOID_ARENA as we have already failed to allocate memory
+ in that arena and it is currently locked. */
+ if (result == avoid_arena)
+ result = result->next;
+
/* No arena available. Wait for the next in line. */
(void)mutex_lock(&result->mutex);
@@ -811,7 +818,7 @@ reused_arena (void)
static mstate
internal_function
-arena_get2(mstate a_tsd, size_t size)
+arena_get2(mstate a_tsd, size_t size, mstate avoid_arena)
{
mstate a;
@@ -856,7 +863,7 @@ arena_get2(mstate a_tsd, size_t size)
catomic_decrement (&narenas);
}
else
- a = reused_arena ();
+ a = reused_arena (avoid_arena);
}
#else
if(!a_tsd)
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 28039b4..1e4f929 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -2865,9 +2865,11 @@ __libc_malloc(size_t bytes)
victim = _int_malloc(ar_ptr, bytes);
(void)mutex_unlock(&ar_ptr->mutex);
} else {
- /* ... or sbrk() has failed and there is still a chance to mmap() */
- ar_ptr = arena_get2(ar_ptr->next ? ar_ptr : 0, bytes);
- (void)mutex_unlock(&main_arena.mutex);
+ /* ... or sbrk() has failed and there is still a chance to mmap()
+ Grab ar_ptr->next prior to releasing its lock. */
+ mstate prev = ar_ptr->next ? ar_ptr : 0;
+ (void)mutex_unlock(&ar_ptr->mutex);
+ ar_ptr = arena_get2(prev, bytes, ar_ptr);
if(ar_ptr) {
victim = _int_malloc(ar_ptr, bytes);
(void)mutex_unlock(&ar_ptr->mutex);
@@ -3043,10 +3045,11 @@ __libc_memalign(size_t alignment, size_t bytes)
p = _int_memalign(ar_ptr, alignment, bytes);
(void)mutex_unlock(&ar_ptr->mutex);
} else {
- /* ... or sbrk() has failed and there is still a chance to mmap() */
+ /* ... or sbrk() has failed and there is still a chance to mmap()
+ Grab ar_ptr->next prior to releasing its lock. */
mstate prev = ar_ptr->next ? ar_ptr : 0;
(void)mutex_unlock(&ar_ptr->mutex);
- ar_ptr = arena_get2(prev, bytes);
+ ar_ptr = arena_get2(prev, bytes, ar_ptr);
if(ar_ptr) {
p = _int_memalign(ar_ptr, alignment, bytes);
(void)mutex_unlock(&ar_ptr->mutex);
@@ -3083,23 +3086,27 @@ __libc_valloc(size_t bytes)
if(!ar_ptr)
return 0;
p = _int_valloc(ar_ptr, bytes);
- (void)mutex_unlock(&ar_ptr->mutex);
if(!p) {
/* Maybe the failure is due to running out of mmapped areas. */
if(ar_ptr != &main_arena) {
+ (void)mutex_unlock(&ar_ptr->mutex);
ar_ptr = &main_arena;
(void)mutex_lock(&ar_ptr->mutex);
p = _int_memalign(ar_ptr, pagesz, bytes);
(void)mutex_unlock(&ar_ptr->mutex);
} else {
- /* ... or sbrk() has failed and there is still a chance to mmap() */
- ar_ptr = arena_get2(ar_ptr->next ? ar_ptr : 0, bytes);
+ /* ... or sbrk() has failed and there is still a chance to mmap()
+ Grab ar_ptr->next prior to releasing its lock. */
+ mstate prev = ar_ptr->next ? ar_ptr : 0;
+ (void)mutex_unlock(&ar_ptr->mutex);
+ ar_ptr = arena_get2(prev, bytes, ar_ptr);
if(ar_ptr) {
p = _int_memalign(ar_ptr, pagesz, bytes);
(void)mutex_unlock(&ar_ptr->mutex);
}
}
- }
+ } else
+ (void)mutex_unlock (&ar_ptr->mutex);
assert(!p || chunk_is_mmapped(mem2chunk(p)) ||
ar_ptr == arena_for_chunk(mem2chunk(p)));
@@ -3127,24 +3134,27 @@ __libc_pvalloc(size_t bytes)
arena_get(ar_ptr, bytes + 2*pagesz + MINSIZE);
p = _int_pvalloc(ar_ptr, bytes);
- (void)mutex_unlock(&ar_ptr->mutex);
if(!p) {
/* Maybe the failure is due to running out of mmapped areas. */
if(ar_ptr != &main_arena) {
+ (void)mutex_unlock(&ar_ptr->mutex);
ar_ptr = &main_arena;
(void)mutex_lock(&ar_ptr->mutex);
p = _int_memalign(ar_ptr, pagesz, rounded_bytes);
(void)mutex_unlock(&ar_ptr->mutex);
} else {
- /* ... or sbrk() has failed and there is still a chance to mmap() */
- ar_ptr = arena_get2(ar_ptr->next ? ar_ptr : 0,
- bytes + 2*pagesz + MINSIZE);
+ /* ... or sbrk() has failed and there is still a chance to mmap()
+ Grab ar_ptr->next prior to releasing its lock. */
+ mstate prev = ar_ptr->next ? ar_ptr : 0;
+ (void)mutex_unlock(&ar_ptr->mutex);
+ ar_ptr = arena_get2(prev, bytes + 2*pagesz + MINSIZE, ar_ptr);
if(ar_ptr) {
p = _int_memalign(ar_ptr, pagesz, rounded_bytes);
(void)mutex_unlock(&ar_ptr->mutex);
}
}
- }
+ } else
+ (void)mutex_unlock(&ar_ptr->mutex);
assert(!p || chunk_is_mmapped(mem2chunk(p)) ||
ar_ptr == arena_for_chunk(mem2chunk(p)));
@@ -3209,8 +3219,6 @@ __libc_calloc(size_t n, size_t elem_size)
#endif
mem = _int_malloc(av, sz);
- /* Only clearing follows, so we can unlock early. */
- (void)mutex_unlock(&av->mutex);
assert(!mem || chunk_is_mmapped(mem2chunk(mem)) ||
av == arena_for_chunk(mem2chunk(mem)));
@@ -3218,21 +3226,24 @@ __libc_calloc(size_t n, size_t elem_size)
if (mem == 0) {
/* Maybe the failure is due to running out of mmapped areas. */
if(av != &main_arena) {
+ (void)mutex_unlock(&av->mutex);
(void)mutex_lock(&main_arena.mutex);
mem = _int_malloc(&main_arena, sz);
(void)mutex_unlock(&main_arena.mutex);
} else {
- /* ... or sbrk() has failed and there is still a chance to mmap() */
- (void)mutex_lock(&main_arena.mutex);
- av = arena_get2(av->next ? av : 0, sz);
- (void)mutex_unlock(&main_arena.mutex);
+ /* ... or sbrk() has failed and there is still a chance to mmap()
+ Grab av->next prior to releasing its lock. */
+ mstate prev = av->next ? av : 0;
+ (void)mutex_unlock(&av->mutex);
+ av = arena_get2(prev, sz, av);
if(av) {
mem = _int_malloc(av, sz);
(void)mutex_unlock(&av->mutex);
}
}
if (mem == 0) return 0;
- }
+ } else
+ (void)mutex_unlock(&av->mutex);
p = mem2chunk(mem);
/* Two optional cases in which clearing not necessary */
More information about the Libc-alpha
mailing list