Bug 191252 - Null pointer dereference in JSC::WriteBarrierBase()
Summary: Null pointer dereference in JSC::WriteBarrierBase()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-05 01:25 PST by Kamil Frankowicz
Modified: 2018-12-15 21:49 PST (History)
9 users (show)

See Also:

Attachments
Crashing test case (jsc) (68 bytes, text/plain)
2018-11-05 01:25 PST, Kamil Frankowicz 		no flags Details
Patch (25.06 KB, patch)
2018-11-06 01:09 PST, Yusuke Suzuki 		keith_miller: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>