Bug 199053 - [bmalloc] IsoHeap's initialization is racy with IsoHeap::isInitialized
Summary: [bmalloc] IsoHeap's initialization is racy with IsoHeap::isInitialized
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-19 18:39 PDT by Yusuke Suzuki
Modified: 2019-06-19 21:42 PDT (History)
9 users (show)

See Also:


Attachments
Patch (104.88 KB, patch)
2019-06-19 19:12 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-06-19 18:39:31 PDT
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
Comment 1 Yusuke Suzuki 2019-06-19 18:40:49 PDT
<rdar://problem/51517867>
Comment 2 Yusuke Suzuki 2019-06-19 19:12:58 PDT
Created attachment 372514 [details]
Patch
Comment 3 Saam Barati 2019-06-19 20:16:38 PDT
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?
Comment 4 Saam Barati 2019-06-19 20:17:43 PDT
(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
Comment 5 Yusuke Suzuki 2019-06-19 21:19:46 PDT
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.
Comment 6 Yusuke Suzuki 2019-06-19 21:42:00 PDT
Committed r246630: <https://trac.webkit.org/changeset/246630>