Bug 301497 – [JSC] null pointer dereference in SamplingProfiler::processUnverifiedStackTraces()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jarred Sumner]

If `calleeBits.asCell()` returns a null pointer, we get a null pointer dereference:

https://github.com/WebKit/WebKit/blob/e4254ec2a5ac077d06a3d9574e36912c95a6dcdb/Source/JavaScriptCore/runtime/SamplingProfiler.cpp#L563-L564

`JSValue::isCell` returns `true` when the `JSValue` is `0`. So this check in HeapUtil is insufficient:

https://github.com/WebKit/WebKit/blob/e4254ec2a5ac077d06a3d9574e36912c95a6dcdb/Source/JavaScriptCore/heap/HeapUtil.h#L86-L90

With UBSAN, this error shows up as:
> runtime error: member call on null pointer of type 'const JSC::HeapCell *'

A fix is to check it's not empty:

diff --git i/Source/JavaScriptCore/runtime/SamplingProfiler.cpp w/Source/JavaScriptCore/runtime/SamplingProfiler.cpp
index 2cafdc0dd6d5..dbc629bbb4ff 100644
--- i/Source/JavaScriptCore/runtime/SamplingProfiler.cpp
+++ w/Source/JavaScriptCore/runtime/SamplingProfiler.cpp
@@ -561,7 +561,7 @@ void SamplingProfiler::processUnverifiedStackTraces()
             }
 
             JSValue callee = calleeBits.asCell();
-            if (!HeapUtil::isValueGCObject(m_vm.heap, filter, callee)) {
+            if (callee.isEmpty() || !HeapUtil::isValueGCObject(m_vm.heap, filter, callee)) {
                 if (!alreadyHasExecutable)
                     stackFrame.frameType = FrameType::Unknown;
                 return;


I don't know why there's a null pointer in there to begin with though. That seems like the bigger question.

[Yusuke Suzuki]

I think this is possible since stack collection happens at arbitrary timing. Let's suppress UBSan message via check.

[Yusuke Suzuki]

As it happens only when it is nullptr, it is deterministic crash issue.

[Radar WebKit Bug Importer]

<rdar://problem/163537650>

[Yusuke Suzuki]

Pull request: https://github.com/WebKit/WebKit/pull/53074

[EWS]

Committed 302226@main (c249a7798f86): <https://commits.webkit.org/302226@main>

Reviewed commits have been landed. Closing PR #53074 and removing active labels.