[PATCH 3/3] Improve zone support for OSX

[Jason Evans]

On Apr 5, 2012, at 11:42 AM, Mike Hommey wrote:
> On Thu, Apr 05, 2012 at 10:52:07AM -0700, Jason Evans wrote:
>> On Apr 5, 2012, at 2:45 AM, Mike Hommey wrote:
>>> On Tue, Mar 20, 2012 at 01:18:13PM -0400, Justin Lebar wrote:
>>>> On Tue, Mar 20, 2012 at 1:10 PM, Mike Hommey
>>>> <mh+jemalloc at glandium.org> wrote:
>>>>> On Tue, Mar 20, 2012 at 01:04:54PM -0400, Justin Lebar wrote:
>>>>>> btw, I'm skeptical of the value of supporting OSX 10.5, because
>>>>>> 10.5 occasionally passes invalid pointers to ozone_size [1].
>>>>>> 
>>>>>> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=694335
>>>>> 
>>>>> For one, not all jemalloc users are going to have the problem,
>>>>> which is limited to some APIs.
>>>> 
>>>> True, but we of course don't know the full set of circumstances
>>>> under which OSX will pass an invalid pointer.
>>>> 
>>>>> Also, AIUI, the current jemalloc code does the right thing for the
>>>>> ozone_size, which is not the case in the old jemalloc fork we use
>>>>> in Firefox, so depending on what is done with the result of
>>>>> ozone_size, it might be okay.
>>>> 
>>>> What's the difference in behavior?
>>>> 
>>>>> BTW, I guess you wanted to point to
>>>>> https://bugzilla.mozilla.org/show_bug.cgi?id=702250#c30
>>>> 
>>>> Yes, thanks.  And my analysis there is that 10.5 tries to free the
>>>> internal pointer.  I'd be impressed if jemalloc handled this
>>>> correctly!  (And even more impressed if it worked in general, for
>>>> non-huge allocations too.  :)
>>> 
>>> From my trivial testing, it actually works for allocations > (1 <<
>>> 15).
>>> 
>>> That is: char *ptr = malloc((1 << 15) + 1); free(ptr + 32); does
>>> free the pointer, while char *ptr = malloc(1 << 15); free(ptr + 32);
>>> doesn't.
>>> 
>>> Neither crash (which is better than the crash we get with our fork
>>> :) )
>>> 
>>> Jason, how actually safe is jemalloc now, when trying to free a
>>> pointer inside an allocation it gave out?
>> 
>> There are assertions that catch attempts to free interior pointers for
>> small allocations, but as far as I know, freeing interior pointers for
>> small allocations will otherwise work.  Freeing interior pointers for
>> large and huge allocations will always cause some sort of metadata
>> corruption.  I expect that both of your test cases are actually
>> causing corruption.  Do you have assertions enabled?  I think (hope)
>> there are assertions that will fail in both cases.
> 
> You're unfortunately right, with assertions, it fails:
> <jemalloc>: ../src/arena.c:1437: Failed assertion: "((uintptr_t)ptr &
> PAGE_MASK) == 0"

This is an assertion in isalloc().  If the assertion were absent, then a different assertion in arena_dalloc() would notice that the pointer isn't page-aligned.  Absent that assertion, things would just happen to work for objects that don't fit in thread cache (> 32 KiB), but for thread-cached free, the application could end up getting the unaligned pointer back as a new allocation, thus resulting in a "short" allocation, potential alignment issues, etc.

> But I was under the impression that the isvalloc check was
> making things safer.

The ivsalloc check makes sure that the pointer is within a chunk of memory that jemalloc owns, but it doesn't provide any additional safety.

Jason