Bug 199053 – [bmalloc] IsoHeap's initialization is racy with IsoHeap::isInitialized

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

Allocator offset is configured before deallocator offset is configured.
But isInitialized just checks Allocator offset.
As a result, if there are two thread A and B,

1. A has just initialized IsoHeap's allocator offset.
2. B sees it and B think IsoHeap is initialized
3. B does `std::max(handle.allocatorOffset(), handle.deallocatorOffset())`
4. Since deallocator offset is not configured yet at (1)'s point, it returns `0 - 1` => 0xffffffff
5. (3)'s result becomes 0xffffffff

[Yusuke Suzuki]

<rdar://problem/51517867>

[Yusuke Suzuki]

Created attachment 372514 [details]
Patch

[Saam Barati]

Comment on attachment 372514 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372514&action=review

Nice!!! r=me

> Source/bmalloc/bmalloc/IsoHeapInlines.h:86
> +    auto* heap = new IsoHeapImpl<Config>();
> +    setAllocatorOffset(heap->allocatorOffset());
> +    setDeallocatorOffset(heap->deallocatorOffset());
> +    auto* atomic = reinterpret_cast<std::atomic<IsoHeapImpl<Config>*>*>(&m_impl);
> +    atomic->store(heap, std::memory_order_release);

Can you comment on this ordering being important?

[Saam Barati]

(In reply to Saam Barati from comment #3)
> Comment on attachment 372514 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=372514&action=review
> 
> Nice!!! r=me
> 
> > Source/bmalloc/bmalloc/IsoHeapInlines.h:86
> > +    auto* heap = new IsoHeapImpl<Config>();
> > +    setAllocatorOffset(heap->allocatorOffset());
> > +    setDeallocatorOffset(heap->deallocatorOffset());
> > +    auto* atomic = reinterpret_cast<std::atomic<IsoHeapImpl<Config>*>*>(&m_impl);
> > +    atomic->store(heap, std::memory_order_release);
> 
> Can you comment on this ordering being important?

And by important, I mean necessary. It'd just be good to briefly describe the protocol of stores/loads we're using

[Yusuke Suzuki]

Comment on attachment 372514 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=372514&action=review

Thank you for your review!

>>> Source/bmalloc/bmalloc/IsoHeapInlines.h:86
>>> +    atomic->store(heap, std::memory_order_release);
>> 
>> Can you comment on this ordering being important?
> 
> And by important, I mean necessary. It'd just be good to briefly describe the protocol of stores/loads we're using

Sounds really nice! I've added the comment here.

[Yusuke Suzuki]

Committed r246630: <https://trac.webkit.org/changeset/246630>