Re: [Patch] [BZ 13939] malloc deadlock

[Maxim Kuvyrkov <maxim at codesourcery dot com>]

On 10/08/2012, at 4:11 PM, Jeff Law wrote:

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

OK for trunk with the NEWS hunk updated to refer to 2.18.

Joseph, I don't think I have formal approval privileges (or do I?), so would you please rubber stamp this review?

David, you may want to consider this patch for 2.17 branch.  The fix should apply cleanly to 2.17.  If you do backport it, please remember to update NEWS hunk on both 2.17 and trunk.

Jeff, thanks for fixing this!

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics