Bug 136485 – Segmentation fault in WTF::RefPtr<JSC::JITCode>::get()

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

[Akos Kiss]

When running jsc tests on EFL/ARM64, the test cyclic-prototypes.js fails with a segfault. The minimized test case reproducing the problem is:

var o1 = { p1: 1 };
var o2 = { p2: 2 };
o2.__proto__ = o1;
var o3 = { p3: 3 };
o3.__proto__ = o2;
o1.__proto__ = o3;

Debugging with gdb gives the following:

(gdb) run --useLLInt=false cp-min-009.js
Starting program: /home/akiss/devel/WebKit/WebKitBuild/Debug/bin/jsc --useLLInt=false cp-min.js
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/aarch64-linux-gnu/libthread_db.so.1".
[New Thread 0x7fb4ca11d0 (LWP 5932)]

Program received signal SIGSEGV, Segmentation fault.
0x0000000000b31e88 in WTF::RefPtr<JSC::JITCode>::get (this=0x65756c6176f0)
    at /home/akiss/devel/WebKit/Source/WTF/wtf/RefPtr.h:57
57	        T* get() const { return m_ptr; }
(gdb) bt
#0  0x0000000000b31e88 in WTF::RefPtr<JSC::JITCode>::get (this=0x65756c6176f0)
    at /home/akiss/devel/WebKit/Source/WTF/wtf/RefPtr.h:57
#1  0x0000000000b2dbfc in JSC::CodeBlock::jitType (this=0x65756c617620)
    at /home/akiss/devel/WebKit/Source/JavaScriptCore/bytecode/CodeBlock.h:291
#2  0x0000000000bff480 in JSC::CodeBlock::hasCodeOrigins (this=0x65756c617620)
    at /home/akiss/devel/WebKit/Source/JavaScriptCore/bytecode/CodeBlock.h:561
#3  0x0000000000c0e3b8 in JSC::StackVisitor::readFrame (this=0x7fffffdd90, callFrame=0x12cb638)
    at /home/akiss/devel/WebKit/Source/JavaScriptCore/interpreter/StackVisitor.cpp:92
#4  0x0000000000c0e354 in JSC::StackVisitor::gotoNextFrame (this=0x7fffffdd90)
    at /home/akiss/devel/WebKit/Source/JavaScriptCore/interpreter/StackVisitor.cpp:68
#5  0x0000000000c09ec0 in JSC::StackVisitor::visit<JSC::GetStackTraceFunctor> (
    startFrame=0x7fffffe070, functor=...)
    at /home/akiss/devel/WebKit/Source/JavaScriptCore/interpreter/StackVisitor.h:132
#6  0x0000000000c085d4 in JSC::ExecState::iterate<JSC::GetStackTraceFunctor> (this=0x7fffffe070, 
    functor=...) at /home/akiss/devel/WebKit/Source/JavaScriptCore/interpreter/CallFrame.h:260
#7  0x0000000000c017b0 in JSC::Interpreter::getStackTrace (this=0x1932710, results=..., 
    maxStackSize=18446744073709551615)
    at /home/akiss/devel/WebKit/Source/JavaScriptCore/interpreter/Interpreter.cpp:604
#8  0x0000000000e5ea00 in JSC::VM::throwException (this=0x19227b0, exec=0x7fffffe070, error=...)
    at /home/akiss/devel/WebKit/Source/JavaScriptCore/runtime/VM.cpp:649
#9  0x0000000000e5ef10 in JSC::VM::throwException (this=0x19227b0, exec=0x7fffffe070, 
    error=0x7fb443fe10) at /home/akiss/devel/WebKit/Source/JavaScriptCore/runtime/VM.cpp:697
#10 0x0000000000d99010 in JSC::globalFuncProtoSetter (exec=0x7fffffe070)
    at /home/akiss/devel/WebKit/Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:805
#11 0x0000000000eabc60 in vmEntryToNative ()
#12 0x0000000000c0306c in JSC::Interpreter::executeCall (this=0x1932710, callFrame=0x7fffffe5d0, 
    function=0x7fb439fbf0, callType=JSC::CallTypeHost, callData=..., thisValue=..., args=...)

This is looks similar to https://bugs.webkit.org/show_bug.cgi?id=136391 , but now JSC::globalFuncProtoSetter gets further (fails "only" at throwing the exception because of the cyclic protos), does not bail out very early (at the iteration, as happened in the previous bug).

The problem is that the stack space allocated by the prologue of JSC::globalFuncProtoSetter for its temporaries (more precisely, the bottom 16 bytes of it) overlaps with CallerFrameAndPC set up by vmEntryToNative. And since globalFuncProtoSetter considers that stack space exclusively as its own, it overwrites it in some cases:

(gdb) disas JSC::globalFuncProtoSetter
Dump of assembler code for function JSC::globalFuncProtoSetter(JSC::ExecState*):
   0x0000000000d98e44 <+0>:	stp	x29, x30, [sp,#-128]!
   0x0000000000d98e48 <+4>:	mov	x29, sp
...
   0x0000000000d98fcc <+392>:	ldr	x0, [x29,#40]
   0x0000000000d98fd0 <+396>:	bl	0xb0895c <JSC::ExecState::vm() const>
   0x0000000000d98fd4 <+400>:	mov	x19, x0
   0x0000000000d98fd8 <+404>:	add	x0, x29, #0x70
   0x0000000000d98fdc <+408>:	adrp	x1, 0x12cb000
   0x0000000000d98fe0 <+412>:	add	x1, x1, #0x638
   0x0000000000d98fe4 <+416>:	bl	0xb02e40 <WTF::ASCIILiteral::ASCIILiteral(char const*)>

// the above instructions are part of the code compiled from:
//    exec->vm().throwException(exec, createError(exec, ASCIILiteral("cyclic __proto__ value")));

So, globalFuncProtoSetter allocates 128 bytes (0x80) on stack, and after the first two instructions sp+0 will contain the old fp, sp+8 will contain the return address, and the remaining area (64bit units at sp+16 .. sp+120) will be used for the temporaries. The problem is that vmEntryToNative has already set up exec starting at sp+120 (+0x70). So, the constructor of ASCIILiteral will just overwrite that field, but throwException will still rely on it.

So, the stack frame of globalFuncProtoSetter and the ExecState set up by vmEntryToNative needs to be separated. IMHO.

[Akos Kiss]

Created attachment 237560 [details]
WIP patch

This is a work in progress patch (at least the Changelog has not been prepared). I think that it may fix not only this bug but https://bugs.webkit.org/show_bug.cgi?id=132740 and https://bugs.webkit.org/show_bug.cgi?id=136436 as well. I believe that all these bugs should be treated together, since they seem to be releated (all of them are caused by relying on assumptions on how the call stack is laid out).

What it does:
- makeHostFunctionCall (both 32_64 and 64 versions) does not add sizeof(CallerFrameAndPC) to sp before the call to entry anymore (which made them rely on calling conventions to fill in caller frame, but also caused the overlap of the stack area of globalFuncProtoSetter with the caller frame).
- Reorganized makeHostFunctionCall (both 32_64 and 64 versions) a bit to be more alike (helps future maintenance).
- Moved the setting of the caller frame of execCallee from JSC::operationCallEval to JIT::compileCallEval (to satisfy what's written in a comment in JIT::compileOpCall, "Caller always [...] Initializes [...] CallerFrame")
- Also changed compileCallEval (both 32_64 and 64 versions) not to rely on calling conventions and keep caller frame and stack of operationCallEval separate.

Status:
- X86-64/EFL and X86-32/EFL passed all jsc tests correctly
- the ARM64/EFL test run is ongoing, but the test cases that previously caused problems (the cyclic proto test case included) have been tested manually and passed
- a Thumb2/EFL test run is also ongoing

Acknowledgements to Michael Saboff and Zan Dobersek, since this patch took "inspiration" from their patches.

[WebKit Commit Bot]

Attachment 237560 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/jit/JITCall32_64.cpp:223:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Comment on attachment 237560 [details]
WIP patch

r-

This seems too complicated with stack pointer adjustments that seem a little arbitrary.

I'm working on a patch that should resolve this and the issues in https://bugs.webkit.org/show_bug.cgi?id=136436.  Basically restore the stackPointer to the normal value before compileCallEval().  We know that there is enough space for outgoing register arguments, that is accounted for in stackPointerOffsetFor(m_codeBlock) for architectures that have stack based arguments.  What I'm suggesting should be much simpler.

[Akos Kiss]

Created attachment 237654 [details]
Updated patch

Removed the changes that would have affected eval calls, since that has already been fixed in https://bugs.webkit.org/show_bug.cgi?id=136436 .

[WebKit Commit Bot]

Comment on attachment 237654 [details]
Updated patch

Clearing flags on attachment: 237654

Committed r173298: <http://trac.webkit.org/changeset/173298>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.