Bug 191252

Summary: Null pointer dereference in JSC::WriteBarrierBase()
Product: WebKit Reporter: Kamil Frankowicz <kamil.frankowicz>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, ews-watchlist, fpizlo, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Crashing test case (jsc)
none
Patch keith_miller: review+

Description Kamil Frankowicz 2018-11-05 01:25:44 PST
Created attachment 353836 [details]
Crashing test case (jsc)

After some fuzz testing I found a crashing test case.

Git HEAD: 43cf251237d5c47f1d0fd8978fd1254870af6721

To reproduce: jsc jsc_nullptr_WriteBarrierBase

Compiler: Clang 4.0

OS: Ubuntu 18.04 x64

ASAN:

==21789==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000018 (pc 0x7f0ed74c493e bp 0x7ffddeadda70 sp 0x7ffddeadd900 T0)
==21789==The signal is caused by a READ memory access.
==21789==Hint: address points to the zero page.
    #0 0x7f0ed74c493d in JSC::WriteBarrierBase<JSC::JSFunction, WTF::DumbPtrTraits<JSC::JSFunction> >::get() const XYZ/webkit_asan/WebKitBuild/Release/../../Source/JavaScriptCore/runtime/WriteBarrier.h:102:28
    #1 0x7f0ed74c493d in JSC::JSPromiseDeferred::reject(JSC::ExecState*, JSC::JSValue) XYZ/webkit_asan/WebKitBuild/Release/../../Source/JavaScriptCore/runtime/JSPromiseDeferred.cpp:118
    #2 0x7f0ed73d3588 in JSC::JSInternalPromiseDeferred::reject(JSC::ExecState*, JSC::JSValue) XYZ/webkit_asan/WebKitBuild/Release/../../Source/JavaScriptCore/runtime/JSInternalPromiseDeferred.cpp:71:11
    #3 0x526ef1 in GlobalObject::moduleLoaderFetch(JSC::JSGlobalObject*, JSC::ExecState*, JSC::JSModuleLoader*, JSC::JSValue, JSC::JSValue, JSC::JSValue) XYZ/webkit_asan/WebKitBuild/Release/../../Source/JavaScriptCore/jsc.cpp:1000:26
    #4 0x7f0ed73f05d6 in JSC::JSModuleLoader::fetch(JSC::ExecState*, JSC::JSValue, JSC::JSValue, JSC::JSValue) XYZ/webkit_asan/WebKitBuild/Release/../../Source/JavaScriptCore/runtime/JSModuleLoader.cpp:285:16
    #5 0x7f0ed7406103 in JSC::moduleLoaderFetch(JSC::ExecState*) XYZ/webkit_asan/WebKitBuild/Release/../../Source/JavaScriptCore/runtime/JSModuleLoader.cpp:461:36
    #6 0x7f0e8c1fe176  (<unknown module>)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV XYZ/webkit_asan/WebKitBuild/Release/../../Source/JavaScriptCore/runtime/WriteBarrier.h:102:28 in JSC::WriteBarrierBase<JSC::JSFunction, WTF::DumbPtrTraits<JSC::JSFunction> >::get() const
==21789==ABORTING
Comment 1 Alexey Proskuryakov 2018-11-05 16:05:36 PST
I could reproduce in shipping Safari/WebKit.
Comment 2 Radar WebKit Bug Importer 2018-11-05 16:06:04 PST
<rdar://problem/45824650>
Comment 3 Alexey Proskuryakov 2018-11-05 16:07:49 PST
This looks more like a consequence of infinite recursion.
Comment 4 Yusuke Suzuki 2018-11-06 00:34:31 PST
JSInternalPromiseDeferred::create fails if stack overflow happens. We should

1. rename it to `tryCreate`
2. and put error check after calling it.
Comment 5 Yusuke Suzuki 2018-11-06 01:09:01 PST
Created attachment 353950 [details]
Patch
Comment 6 Yusuke Suzuki 2018-11-06 01:10:22 PST
Currently, our module mechanism leverages JS builtin code. And right now, it is not immune to the stack overflow. Let's just `RELEASE_ASSERT` instead of nullptr dereference to avoid incorrect memory loading.

With the example test case, JSC shows expected RELEASE_ASSERT crash instead of nullptr dereference.
Comment 7 EWS Watchlist 2018-11-06 01:13:12 PST
Attachment 353950 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Yusuke Suzuki 2018-11-11 23:58:47 PST
Ping?
Comment 9 Keith Miller 2018-12-14 15:08:05 PST
Comment on attachment 353950 [details]
Patch

r=me.
Comment 10 Yusuke Suzuki 2018-12-15 21:49:06 PST
Committed r239256: <https://trac.webkit.org/changeset/239256>