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

[Nate Gallaher <nate at jaybridge dot com>]

I am not sure that is a complete solution.  I believe that any fastbin
content dereference is a potential race point.

For example, in _int_malloc:

3268     mfastbinptr* fb = &fastbin (av, idx);
3269     mchunkptr pp = *fb;
3270     do
3271       {
3272         victim = pp;
3273         if (victim == NULL)
3274           break;
3275       }
3276     while ((pp = catomic_compare_and_exchange_val_acq (fb,
victim->fd, victim))
3277            != victim);

If we assume that there are entries in the fastbin (so pp is
non-NULL), then the following happens:

Thread #1:
 * Executes lines 3268 through 3274.  pp and victim are non-NULL.

Context swap to Thread #2:
 * Executes code that frees enough memory into the fastbin to trigger
the bin consolidation.
 * Bin consolidation frees the page that pp and victim pointed into
using systrim().  This page is returned to the system.

Context swap to Thread #1:
 * In setting up for the compare and swap on line 3276, we dereference
victim to get victim->fd.  Since victim points to memory that was
released to the system, we get a segfault.


Is my understanding correct?

Thanks,

Nate Gallaher

On Thu, Dec 19, 2013 at 5:49 PM, Maxim Kuvyrkov <maxim@kugelworks.com> wrote:
> Hi Ondrej,
>
> I'm reviewing your patch (and trying to make sense of _int_free code).  Will report back in couple of days.
>
> Thanks,
>
> --
> Maxim Kuvyrkov
> www.kugelworks.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.
>>>
>>> Solution is simply drop that check.
>>>
>>>      [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;
>