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

[OndÅej BÃlka <neleai at seznam dot cz>]

On Sun, Dec 22, 2013 at 09:08:42AM +1300, Maxim Kuvyrkov wrote:
> 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.
>
As most malloc checks decrease readibility I favor deletion. Adding a
check for lock solves problem but as _int_free always called from
__libc_free with have_lock = 0 it would render check mostly useless.

> >> 
> >> 	[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.
>
Yes, I meant that.
 
> 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?
>
Correct yes, question is how that check is useful.