Bug 199997

Summary: JSC: assertion failure in SpeculativeJIT::compileGetByValOnIntTypedArray
Product: WebKit Reporter: Samuel Groß <saelo>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, darin, ews-feeder, fpizlo, mark.lam, msaboff, product-security, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: Safari 12   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch saam: review+, saam: commit-queue+

Description Samuel Groß 2019-07-22 02:44:04 PDT
The following JavaScript program crashes current JSC built in debug configurations:

function v0(v1, t) {
    let v5 = v1;
    for (let v10 = 0; v10 < 8; v10++) {
        const v11 = v1 instanceof Uint32Array;
        const v12 = v1[65537];
    }
    const v13 = v5[0];
}
const v15 = new Uint32Array(1024);
for (let v19 = 0; v19 < 10000; v19++) {
    const v20 = v0(v15);
}

Crashes with:

ASSERTION FAILED: node->arrayMode().alreadyChecked(m_jit.graph(), node, m_state.forNode(m_graph.varArgChild(node, 0)))
../../Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp(2966) : void JSC::DFG::SpeculativeJIT::compileGetByValOnIntTypedArray(JSC::DFG::Node *, JSC::TypedArrayType)
1   0x107f95d29 WTFCrash
2   0x10652311b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x1070bfc5f JSC::DFG::SpeculativeJIT::compileGetByValOnIntTypedArray(JSC::DFG::Node*, JSC::TypedArrayType)
4   0x1072711e1 JSC::DFG::SpeculativeJIT::compile(JSC::DFG::Node*)
5   0x1070b7f79 JSC::DFG::SpeculativeJIT::compileCurrentBlock()
6   0x1070b8d52 JSC::DFG::SpeculativeJIT::compile()
7   0x106f1d954 JSC::DFG::JITCompiler::compileBody()
8   0x106f211fa JSC::DFG::JITCompiler::compileFunction()
9   0x10705f813 JSC::DFG::Plan::compileInThreadImpl()
10  0x10705d256 JSC::DFG::Plan::compileInThread(JSC::DFG::ThreadData*)
11  0x107312c6b JSC::DFG::Worklist::ThreadBody::work()
12  0x107f9bf19 WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0::operator()() const
13  0x107f9bb59 WTF::Detail::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0, void>::call()
14  0x1070ac5ea WTF::Function<void ()>::operator()() const
15  0x107fd8600 WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*)
16  0x10806d7d5 WTF::wtfThreadEntryPoint(void*)
17  0x7fff5b3d72eb _pthread_body
18  0x7fff5b3da249 _pthread_start
19  0x7fff5b3d640d thread_start

Roughly what seems to happen here is that during structure check hoisting, the structure check for the Uint32Array is moved to the beginning of the function and is replaced with a CheckStructureOrEmpty node (presumably due to the addition variable v5). Afterwards, AI types the argument as `Uint32Array | Empty`. This doesn't change until SpeculativeJIT lowering at which point the assertion triggers as the type of the input should just be `Uint32Array` but still is `Uint32Array | Empty`. However, I don't think the empty value can occur at this point in the program (an argument to a function). Moreover, even if it did this would probably only result in a nullpointer dereference. I'm reporting this as a security bug just in case I'm missing something here, but I do not believe this bug to be exploitable.
Comment 1 Radar WebKit Bug Importer 2019-07-22 02:44:16 PDT
<rdar://problem/53388642>
Comment 2 Saam Barati 2019-08-02 17:36:48 PDT
Thanks for the report.

This is not a security issue. Our static analysis in AI is conservative, the bug here is we're asserting that AI is precise. We shouldn't assert such things.
Comment 3 Michael Saboff 2019-08-05 10:41:48 PDT
Created attachment 375534 [details]
Patch
Comment 4 Yusuke Suzuki 2019-08-05 11:00:23 PDT
r=me too.
Comment 5 Darin Adler 2019-08-05 11:47:45 PDT
Since this is not a security issue, should we move it out of the security-sensitive component?
Comment 6 Michael Saboff 2019-08-05 13:02:53 PDT
Committed r248271: <https://trac.webkit.org/changeset/248271>