Bug 227988 – Grab the lock in FTL::Thunks::keyForSlowPathCallThunk

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Samuel Groß]

Created attachment 433579 [details]
Crashing sample

The FTL::Thunks class [0] appears to be used to store shared fragments of code used by the FTL JIT. It provides two methods:

    MacroAssemblerCodeRef<JITThunkPtrTag> getSlowPathCallThunk(VM& vm, const SlowPathCallKey& key)
    {
        Locker locker { m_lock };
        return generateIfNecessary(vm, m_slowPathCallThunks, key, slowPathCallThunkGenerator);  // This does a HashTable::add
    }

    SlowPathCallKey keyForSlowPathCallThunk(MacroAssemblerCodePtr<JITThunkPtrTag> ptr)
    {
        return keyForThunk(m_slowPathCallThunks, ptr);      // This does a HashTable::lookup
    }

The keyForSlowPathCallThunk method seems to be called on the main thread during repatching (see stack trace below), while the getSlowPathCallThunk method can be called on a background JIT thread (this can e.g. be seen by setting a breakpoint on getSlowPathCallThunk or one of its callers, e.g. SlowPathCallContext::makeCall [1] and checking the thread that hits it). As such, there is a race condition here: the main thread doesn't attempt to take the lock when performing the lookup while the HashTable::add performed by the background thread can lead to a HashTable::rehash, and thus e.g. to the backing memory being freed. Frequently, winning this race seems to cause a RELEASE_ASSERT in FTL::Thunks::keyForThunk [2] to be hit (presumably because the rehash happens in place, and so the element isn't found), however, it can also cause memory corruption, as shown by the following ASAN report:

=================================================================
==936821==ERROR: AddressSanitizer: heap-use-after-free on address 0x6140000197a8 at pc 0x00000049bc5a bp 0x7ffffe183130 sp 0x7ffffe1828f8
READ of size 40 at 0x6140000197a8 thread T0
    #0 0x49bc59 in __asan_memcpy (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x49bc59)
    #1 0x223b276 in JSC::FTL::ThunkMap<JSC::FTL::SlowPathCallKey>::KeyType JSC::FTL::keyForThunk<JSC::FTL::ThunkMap<JSC::FTL::SlowPathCallKey> >(JSC::FTL::ThunkMap<JSC::FTL::SlowPathCallKey>&, JSC::MacroAssemblerCodePtr<(WTF::PtrTag)26129>) (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x223b276)
    #2 0x21ccc0c in JSC::ftlThunkAwareRepatchCall(JSC::CodeBlock*, JSC::CodeLocationCall<(WTF::PtrTag)26432>, JSC::FunctionPtr<(WTF::PtrTag)1>) (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x21ccc0c)
    #3 0x21db0e2 in JSC::repatchPutByID(JSC::JSGlobalObject*, JSC::CodeBlock*, JSC::JSValue, JSC::Structure*, JSC::CacheableIdentifier, JSC::PutPropertySlot const&, JSC::StructureStubInfo&, JSC::PutKind) (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x21db0e2)
    #4 0x1fe115e in JSC::operationPutByIdDirectNonStrictOptimize(JSC::JSGlobalObject*, JSC::StructureStubInfo*, long, long, unsigned long) (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x1fe115e)
    #5 0x7f32f87be072  (<unknown module>)

0x6140000197a8 is located 360 bytes inside of 400-byte region [0x614000019640,0x6140000197d0)
freed by thread T1 (JITWorker) here:
    #0 0x49c58d in free (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x49c58d)
    #1 0x1c2c397 in WTF::HashTable<JSC::MacroAssemblerCodePtr<(WTF::PtrTag)26129>, WTF::KeyValuePair<JSC::MacroAssemblerCodePtr<(WTF::PtrTag)26129>, JSC::FTL::SlowPathCallKey>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<JSC::MacroAssemblerCodePtr<(WTF::PtrTag)26129>, JSC::FTL::SlowPathCallKey> >, WTF::DefaultHash<JSC::MacroAssemblerCodePtr<(WTF::PtrTag)26129> >, WTF::HashMap<JSC::MacroAssemblerCodePtr<(WTF::PtrTag)26129
>, JSC::FTL::SlowPathCallKey, WTF::DefaultHash<JSC::MacroAssemblerCodePtr<(WTF::PtrTag)26129> >, WTF::HashTraits<JSC::MacroAssemblerCodePtr<(WTF::PtrTag)26129> >, WTF::HashTraits<JSC::FTL::SlowPathCallKey>, WTF::HashTableTraits>::KeyValuePairTraits, WTF::HashTraits<JSC::MacroAssemblerCodePtr<(WTF::PtrTag)26129> > >::rehash(unsigned int, WTF::KeyValuePair<JSC::MacroAssemblerCodePtr<(WTF::PtrTag)26129>, JSC::FTL::SlowPathCallK
ey>*) (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x1c2c397)

previously allocated by thread T2 (JITWorker) here:
    #0 0x49c80d in malloc (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x49c80d)
    #1 0x49c334a in bmalloc::DebugHeap::malloc(unsigned long, bmalloc::FailureAction) (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x49c334a)
    #2 0x1c24a0b in WTF::SharedTaskFunctor<void (JSC::LinkBuffer&), JSC::FTL::SlowPathCallContext::makeCall(JSC::VM&, JSC::FunctionPtr<(WTF::PtrTag)1>)::$_5>::run(JSC::LinkBuffer&) (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x1c24a0b)
    #3 0x5ada3b in JSC::LinkBuffer::finalizeCodeWithoutDisassemblyImpl() (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x5ada3b)
    #4 0xe8cfc4 in JSC::MacroAssemblerCodeRef<(WTF::PtrTag)357> JSC::LinkBuffer::finalizeCodeWithoutDisassembly<(WTF::PtrTag)357>() (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0xe8cfc4)
    #5 0x1bd3a8e in JSC::FTL::link(JSC::FTL::State&) (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x1bd3a8e)
    #6 0x100fddb in JSC::DFG::Plan::compileInThreadImpl() (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x100fddb)
    #7 0x205101c in JSC::JITPlan::compileInThread(JSC::JITWorklistThread*) (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x205101c)
    #8 0x21ba282 in JSC::JITWorklistThread::work() (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x21ba282)
    #9 0x47e33da in WTF::Detail::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0, void>::call() (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x47e33da)
    #10 0x48429df in WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x48429df)
    #11 0x498a915 in WTF::wtfThreadEntryPoint(void*) (/home/builder/webkit/FuzzBuild/Release/bin/jsc+0x498a915)

This likely happens when the background thread frees the m_table member after rehashing the table and after the main thread loaded the old value of the pointer during a lookup.

The attached sample, found by Fuzzilli, appears to trigger this race sometimes, but very rarely. The sample is unfortunately not minimized as it isn't deterministic.
I managed to trigger the above ASAN crash by compiling JSC on Linux with
    ./Tools/Scripts/build-jsc --jsc-only --release --cmakeargs="-DENABLE_STATIC_JSC=ON -DCMAKE_C_COMPILER='/usr/bin/clang-10' -DCMAKE_CXX_COMPILER='/usr/bin/clang++-10' -DCMAKE_C_FLAGS='-fsanitize=address' -DCMAKE_CXX_FLAGS='-fsanitize=address' -DCMAKE_EXE_LINKER_FLAGS='-fsanitize=address'"
and repeatedly executing the attached sample with
    while true; do ASAN_OPTIONS=detect_leaks=0 ./FuzzBuild/Release/bin/jsc --thresholdForJITSoon=10 --thresholdForJITAfterWarmUp=10 --thresholdForOptimizeAfterWarmUp=100 --threshold ForOptimizeAfterLongWarmUp=100 --thresholdForOptimizeAfterLongWarmUp=100 --thresholdForFTLOptimizeAfterWarmUp=1000 --thresholdForFTLOptimizeSoon=1000 ftl_crash.js; done
If it helps, I can see if it's possible to place some sleeps into the HashTable code to make the race easier to trigger.

The bug was likely introduced by this commit: https://github.com/WebKit/WebKit/commit/934b072e307a52ef7c4bc75f6862635d7189e772

[0] https://github.com/WebKit/WebKit/blob/a1f561e19cfd520000fcb4f24f88d53b2a54d8b1/Source/JavaScriptCore/ftl/FTLThunks.h#L77
[1] https://github.com/WebKit/WebKit/blob/a1f561e19cfd520000fcb4f24f88d53b2a54d8b1/Source/JavaScriptCore/ftl/FTLSlowPathCall.cpp#L127
[2] https://github.com/WebKit/WebKit/blob/a1f561e19cfd520000fcb4f24f88d53b2a54d8b1/Source/JavaScriptCore/ftl/FTLThunks.h#L73

[Radar WebKit Bug Importer]

<rdar://problem/80627901>

[Saam Barati]

Created attachment 433707 [details]
patch

[Mark Lam]

Comment on attachment 433707 [details]
patch

r=me

[EWS]

Committed r280008 (239750@main): <https://commits.webkit.org/239750@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433707 [details].