Re: [PING][PATCH][BZ #15073] Fix race in free.

[Maxim Kuvyrkov <maxim at kugelworks dot com>]

On 18/12/2013, at 11:27 pm, OndÅej BÃlka <neleai@seznam.cz> wrote:

> ping
> On Tue, Dec 10, 2013 at 08:43:49PM +0100, OndÅej BÃlka wrote:
>> Hi, 
>> 
>> While actual implementation of atomic returning to fastbin is correct
>> this could not be sayed about 'sanity' check surrounding it.
>> 
>> When we read fastbin entry there is no quarantee that memory will be
>> deallocated in meantime. 
>> 
>> Yet we try to dereference that pointer for a check that does not make
>> any sense. We check if old chunk belongs to fastbin we are adding
>> element. If old gets consolidated to bigger chunk we get a spurious
>> memory corruption error, while if we continue a subsequent compare and
>> swap will fail.

Ondrej, good job on analyzing the bug.  All above is exactly right.

>> 
>> Solution is simply drop that check.

Or fix the check.  If we have_lock for the memory state there is no way a competing thread can snatch and deallocate the fastbin chunk that we are adding to.

>> 
>> 	[BZ #15073]
>> 	* malloc/malloc.c (_int_free): Remove sanity check that causes
>> 	race condition.
>> 
>> diff --git a/malloc/malloc.c b/malloc/malloc.c
>> index b1668b5..a05d4bb 100644
>> --- a/malloc/malloc.c
>> +++ b/malloc/malloc.c
>> @@ -3785,7 +3785,6 @@ _int_free(mstate av, mchunkptr p, int have_lock)
>> 
>>     mchunkptr fd;
>>     mchunkptr old = *fb;
>> -    unsigned int old_idx = ~0u;
>>     do
>>       {
>> 	/* Another simple check: make sure the top of the bin is not the
>> @@ -3795,13 +3794,10 @@ _int_free(mstate av, mchunkptr p, int have_lock)
>> 	    errstr = "double free or corruption (fasttop)";
>> 	    goto errout;
>> 	  }
>> -	if (old != NULL)
>> -	  old_idx = fastbin_index(chunksize(old));
>> 	p->fd = fd = old;
>>       }
>>     while ((old = catomic_compare_and_exchange_val_rel (fb, p, fd)) != fd);
>> 
>> -    if (fd != NULL && __builtin_expect (old_idx != idx, 0))
>> +    if (fd != NULL)
>>       {
>> 	errstr = "invalid fastbin entry (free)";
>> 	goto errout;

Surely you intended to remove the whole if-clause.  As it stands above, the code will produce spurious "invalid fastbin entry (free)" errors for any fastbin of size 2 or more.

Also, as a general plea to community, please write comments and annotate any code that you fix.  With glibc's smart and fast algorithms and implementations comments are invaluable.

Ondrej, how about the following patch instead?  Does it look correct to you?

Thanks!

--
Maxim Kuvyrkov
www.kugelworks.com

Attachment: 0001-Fix-race-in-free-of-fastbin-chunk-BZ-15073.patch
Description: Binary data