- Revision
- 225140
- Author
- [email protected]
- Date
- 2017-11-24 13:02:01 -0800 (Fri, 24 Nov 2017)
Log Message
Fix CLoop::sanitizeStack() bug where it was clearing part of the JS stack in use.
https://bugs.webkit.org/show_bug.cgi?id=179936
<rdar://problem/35623998>
Reviewed by Saam Barati.
This issue was uncovered when we enabled --useDollarVM=true on the JSC tests.
See https://bugs.webkit.org/show_bug.cgi?id=179684.
Basically, in the case of the failing test we observed, op_tail_call_forward_arguments
was allocating stack space to stash arguments (to be forwarded) and new frame
info. The location of this new stash space happens to lie beyond the top of frame
of the tail call caller frame. After stashing the arguments, the code proceeded
to load the callee codeBlock. This triggered an allocation, which in turn,
triggered stack sanitization. The CLoop stack sanitizer was relying on
frame->topOfFrame() to tell it where the top of the used stack is. In this case,
that turned out to be inadequate. As a result, part of the stashed data was
zeroed out, and subsequently led to a crash.
This bug does not affect JIT builds (i.e. the ASM LLint) for 2 reasons:
1. JIT builds do stack sanitization in the LLInt code itself (different from the
CLoop implementation), and the sanitizer there is aware of the true top of
stack value (i.e. the stack pointer).
2. JIT builds don't use a parallel stack like the CLoop. The presence of the
parallel stack is one condition necessary for reproducing this issue.
The fix is to make the CLoop record the stack pointer in CLoopStack::m_currentStackPointer
every time before it calls out to native C++ code. This also brings the CLoop's
behavior closer to hardware behavior where we can know where the stack pointer
is after calling from JS back into native C++ code, which makes it easier to
reason about correctness.
Also simplified the various stack boundary calculations (removed the +1 and -1
adjustments). The CLoopStack bounds are now:
reservationTop(): the lowest reserved address that can be within stack bounds.
m_commitTop: the lowest address within stack bounds that has been committed.
lowAddress() aka m_end: the lowest stack address that JS code can use.
m_lastStackPointer: cache of the last m_currentStackPointer value.
m_currentStackPointer: the CLoopStack stack pointer value when calling from JS into C++ code.
highAddress(): the highest address just beyond the bounds of the stack.
Also deleted some unneeded code.
* interpreter/CLoopStack.cpp:
(JSC::CLoopStack::CLoopStack):
(JSC::CLoopStack::gatherConservativeRoots):
(JSC::CLoopStack::sanitizeStack):
(JSC::CLoopStack::setSoftReservedZoneSize):
* interpreter/CLoopStack.h:
(JSC::CLoopStack::setCurrentStackPointer):
(JSC::CLoopStack::lowAddress const):
(JSC::CLoopStack::baseOfStack const): Deleted.
- Not needed after we simplified the code and removed all the +1/-1 adjustments.
Now, it has the exact same value as highAddress() and can be removed.
* interpreter/CLoopStackInlines.h:
(JSC::CLoopStack::ensureCapacityFor):
(JSC::CLoopStack::currentStackPointer):
(JSC::CLoopStack::setCLoopStackLimit):
(JSC::CLoopStack::topOfFrameFor): Deleted.
- Not needed.
(JSC::CLoopStack::topOfStack): Deleted.
- Supplanted by currentStackPointer().
(JSC::CLoopStack::shrink): Deleted.
- This is unused.
* llint/LowLevelInterpreter.cpp:
(JSC::CLoop::execute):
- Introduce a StackPointerScope to restore the original CLoopStack::m_currentStackPointer
upon exitting the interpreter loop.
* offlineasm/cloop.rb:
- Added setting of CLoopStack::m_currentStackPointer at boundary points where we
call from JS into C++ code.
* tools/VMInspector.h:
- Added some default argument values. These were being used while debugging this
issue.
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (225139 => 225140)
--- trunk/Source/_javascript_Core/ChangeLog 2017-11-24 19:52:55 UTC (rev 225139)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-11-24 21:02:01 UTC (rev 225140)
@@ -1,3 +1,89 @@
+2017-11-24 Mark Lam <[email protected]>
+
+ Fix CLoop::sanitizeStack() bug where it was clearing part of the JS stack in use.
+ https://bugs.webkit.org/show_bug.cgi?id=179936
+ <rdar://problem/35623998>
+
+ Reviewed by Saam Barati.
+
+ This issue was uncovered when we enabled --useDollarVM=true on the JSC tests.
+ See https://bugs.webkit.org/show_bug.cgi?id=179684.
+
+ Basically, in the case of the failing test we observed, op_tail_call_forward_arguments
+ was allocating stack space to stash arguments (to be forwarded) and new frame
+ info. The location of this new stash space happens to lie beyond the top of frame
+ of the tail call caller frame. After stashing the arguments, the code proceeded
+ to load the callee codeBlock. This triggered an allocation, which in turn,
+ triggered stack sanitization. The CLoop stack sanitizer was relying on
+ frame->topOfFrame() to tell it where the top of the used stack is. In this case,
+ that turned out to be inadequate. As a result, part of the stashed data was
+ zeroed out, and subsequently led to a crash.
+
+ This bug does not affect JIT builds (i.e. the ASM LLint) for 2 reasons:
+ 1. JIT builds do stack sanitization in the LLInt code itself (different from the
+ CLoop implementation), and the sanitizer there is aware of the true top of
+ stack value (i.e. the stack pointer).
+ 2. JIT builds don't use a parallel stack like the CLoop. The presence of the
+ parallel stack is one condition necessary for reproducing this issue.
+
+ The fix is to make the CLoop record the stack pointer in CLoopStack::m_currentStackPointer
+ every time before it calls out to native C++ code. This also brings the CLoop's
+ behavior closer to hardware behavior where we can know where the stack pointer
+ is after calling from JS back into native C++ code, which makes it easier to
+ reason about correctness.
+
+ Also simplified the various stack boundary calculations (removed the +1 and -1
+ adjustments). The CLoopStack bounds are now:
+
+ reservationTop(): the lowest reserved address that can be within stack bounds.
+ m_commitTop: the lowest address within stack bounds that has been committed.
+ lowAddress() aka m_end: the lowest stack address that JS code can use.
+ m_lastStackPointer: cache of the last m_currentStackPointer value.
+ m_currentStackPointer: the CLoopStack stack pointer value when calling from JS into C++ code.
+ highAddress(): the highest address just beyond the bounds of the stack.
+
+ Also deleted some unneeded code.
+
+ * interpreter/CLoopStack.cpp:
+ (JSC::CLoopStack::CLoopStack):
+ (JSC::CLoopStack::gatherConservativeRoots):
+ (JSC::CLoopStack::sanitizeStack):
+ (JSC::CLoopStack::setSoftReservedZoneSize):
+ * interpreter/CLoopStack.h:
+ (JSC::CLoopStack::setCurrentStackPointer):
+ (JSC::CLoopStack::lowAddress const):
+
+ (JSC::CLoopStack::baseOfStack const): Deleted.
+ - Not needed after we simplified the code and removed all the +1/-1 adjustments.
+ Now, it has the exact same value as highAddress() and can be removed.
+
+ * interpreter/CLoopStackInlines.h:
+ (JSC::CLoopStack::ensureCapacityFor):
+ (JSC::CLoopStack::currentStackPointer):
+ (JSC::CLoopStack::setCLoopStackLimit):
+
+ (JSC::CLoopStack::topOfFrameFor): Deleted.
+ - Not needed.
+
+ (JSC::CLoopStack::topOfStack): Deleted.
+ - Supplanted by currentStackPointer().
+
+ (JSC::CLoopStack::shrink): Deleted.
+ - This is unused.
+
+ * llint/LowLevelInterpreter.cpp:
+ (JSC::CLoop::execute):
+ - Introduce a StackPointerScope to restore the original CLoopStack::m_currentStackPointer
+ upon exitting the interpreter loop.
+
+ * offlineasm/cloop.rb:
+ - Added setting of CLoopStack::m_currentStackPointer at boundary points where we
+ call from JS into C++ code.
+
+ * tools/VMInspector.h:
+ - Added some default argument values. These were being used while debugging this
+ issue.
+
2017-11-24 Yusuke Suzuki <[email protected]>
[JSC] Make empty key as deleted mark in HashMapBucket and drop m_deleted field
Modified: trunk/Source/_javascript_Core/interpreter/CLoopStack.cpp (225139 => 225140)
--- trunk/Source/_javascript_Core/interpreter/CLoopStack.cpp 2017-11-24 19:52:55 UTC (rev 225139)
+++ trunk/Source/_javascript_Core/interpreter/CLoopStack.cpp 2017-11-24 21:02:01 UTC (rev 225140)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2008, 2013-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2008-2017 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -53,7 +53,6 @@
CLoopStack::CLoopStack(VM& vm)
: m_vm(vm)
, m_topCallFrame(vm.topCallFrame)
- , m_end(0)
, m_softReservedZoneSizeInRegisters(0)
{
size_t capacity = Options::maxPerThreadStackUsage();
@@ -60,11 +59,14 @@
ASSERT(capacity && isPageAligned(capacity));
m_reservation = PageReservation::reserve(WTF::roundUpToMultipleOf(commitSize(), capacity), OSAllocator::JSVMStackPages);
- setCLoopStackLimit(highAddress());
- m_commitTop = highAddress();
-
- m_lastStackTop = baseOfStack();
+ auto* bottomOfStack = highAddress();
+ setCLoopStackLimit(bottomOfStack);
+ ASSERT(m_end == bottomOfStack);
+ m_commitTop = bottomOfStack;
+ m_lastStackPointer = bottomOfStack;
+ m_currentStackPointer = bottomOfStack;
+
m_topCallFrame = 0;
}
@@ -106,21 +108,21 @@
void CLoopStack::gatherConservativeRoots(ConservativeRoots& conservativeRoots, JITStubRoutineSet& jitStubRoutines, CodeBlockSet& codeBlocks)
{
- conservativeRoots.add(topOfStack() + 1, highAddress(), jitStubRoutines, codeBlocks);
+ conservativeRoots.add(currentStackPointer(), highAddress(), jitStubRoutines, codeBlocks);
}
void CLoopStack::sanitizeStack()
{
#if !ASAN_ENABLED
- ASSERT(topOfStack() <= baseOfStack());
-
- if (m_lastStackTop < topOfStack()) {
- char* begin = reinterpret_cast<char*>(m_lastStackTop + 1);
- char* end = reinterpret_cast<char*>(topOfStack() + 1);
+ void* stackTop = currentStackPointer();
+ ASSERT(stackTop <= highAddress());
+ if (m_lastStackPointer < stackTop) {
+ char* begin = reinterpret_cast<char*>(m_lastStackPointer);
+ char* end = reinterpret_cast<char*>(stackTop);
memset(begin, 0, end - begin);
}
- m_lastStackTop = topOfStack();
+ m_lastStackPointer = stackTop;
#endif
}
@@ -143,8 +145,8 @@
void CLoopStack::setSoftReservedZoneSize(size_t reservedZoneSize)
{
m_softReservedZoneSizeInRegisters = reservedZoneSize / sizeof(Register);
- if (m_commitTop >= (m_end + 1) - m_softReservedZoneSizeInRegisters)
- grow(m_end + 1);
+ if (m_commitTop > m_end - m_softReservedZoneSizeInRegisters)
+ grow(m_end);
}
bool CLoopStack::isSafeToRecurse() const
Modified: trunk/Source/_javascript_Core/interpreter/CLoopStack.h (225139 => 225140)
--- trunk/Source/_javascript_Core/interpreter/CLoopStack.h 2017-11-24 19:52:55 UTC (rev 225139)
+++ trunk/Source/_javascript_Core/interpreter/CLoopStack.h 2017-11-24 21:02:01 UTC (rev 225140)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2008-2009, 2013-2014, 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2008-2017 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -59,22 +59,18 @@
void gatherConservativeRoots(ConservativeRoots&, JITStubRoutineSet&, CodeBlockSet&);
void sanitizeStack();
- Register* baseOfStack() const
- {
- return highAddress() - 1;
- }
+ inline void* currentStackPointer();
+ void setCurrentStackPointer(void* sp) { m_currentStackPointer = sp; }
size_t size() const { return highAddress() - lowAddress(); }
void setSoftReservedZoneSize(size_t);
bool isSafeToRecurse() const;
- inline Register* topOfStack();
private:
-
Register* lowAddress() const
{
- return m_end + 1;
+ return m_end;
}
Register* highAddress() const
@@ -82,8 +78,6 @@
return reinterpret_cast_ptr<Register*>(static_cast<char*>(m_reservation.base()) + m_reservation.size());
}
- inline Register* topOfFrameFor(CallFrame*);
-
Register* reservationTop() const
{
char* reservationTop = static_cast<char*>(m_reservation.base());
@@ -91,7 +85,6 @@
}
bool grow(Register* newTopOfStack);
- void shrink(Register* newTopOfStack);
void releaseExcessCapacity();
void addToCommittedByteCount(long);
@@ -99,10 +92,14 @@
VM& m_vm;
CallFrame*& m_topCallFrame;
- Register* m_end;
- Register* m_commitTop;
+
+ // The following is always true:
+ // reservationTop() <= m_commitTop <= m_end <= m_currentStackPointer <= highAddress()
+ Register* m_end; // lowest address of JS allocatable stack memory.
+ Register* m_commitTop; // lowest address of committed memory.
PageReservation m_reservation;
- Register* m_lastStackTop;
+ void* m_lastStackPointer;
+ void* m_currentStackPointer;
ptrdiff_t m_softReservedZoneSizeInRegisters;
friend class LLIntOffsetsExtractor;
Modified: trunk/Source/_javascript_Core/interpreter/CLoopStackInlines.h (225139 => 225140)
--- trunk/Source/_javascript_Core/interpreter/CLoopStackInlines.h 2017-11-24 19:52:55 UTC (rev 225139)
+++ trunk/Source/_javascript_Core/interpreter/CLoopStackInlines.h 2017-11-24 21:02:01 UTC (rev 225140)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2012-2014, 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2012-2017 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -36,43 +36,24 @@
inline bool CLoopStack::ensureCapacityFor(Register* newTopOfStack)
{
- Register* newEnd = newTopOfStack - 1;
- if (newEnd >= m_end)
+ if (newTopOfStack >= m_end)
return true;
return grow(newTopOfStack);
}
-inline Register* CLoopStack::topOfFrameFor(CallFrame* frame)
+inline void* CLoopStack::currentStackPointer()
{
- if (UNLIKELY(!frame))
- return baseOfStack();
- return frame->topOfFrame() - 1;
+ // One might be tempted to assert that m_currentStackPointer <= m_topCallFrame->topOfFrame()
+ // here. That assertion would be incorrect because this function may be called from function
+ // prologues (e.g. during a stack check) where m_currentStackPointer may be higher than
+ // m_topCallFrame->topOfFrame() because the stack pointer has not been initialized to point
+ // to frame top yet.
+ return m_currentStackPointer;
}
-inline Register* CLoopStack::topOfStack()
-{
- return topOfFrameFor(m_topCallFrame);
-}
-
-inline void CLoopStack::shrink(Register* newTopOfStack)
-{
- Register* newEnd = newTopOfStack - 1;
- if (newEnd >= m_end)
- return;
- setCLoopStackLimit(newTopOfStack);
- // Note: Clang complains of an unresolved linkage to maxExcessCapacity if
- // invoke std::max() with it as an argument. To work around this, we first
- // assign the constant to a local variable, and use the local instead.
- ptrdiff_t maxExcessCapacity = CLoopStack::maxExcessCapacity;
- ptrdiff_t maxExcessInRegisters = std::max(maxExcessCapacity, m_softReservedZoneSizeInRegisters);
- if (m_end == baseOfStack() && (highAddress() - m_commitTop) >= maxExcessInRegisters)
- releaseExcessCapacity();
-}
-
inline void CLoopStack::setCLoopStackLimit(Register* newTopOfStack)
{
- Register* newEnd = newTopOfStack - 1;
- m_end = newEnd;
+ m_end = newTopOfStack;
m_vm.setCLoopStackLimit(newTopOfStack);
}
Modified: trunk/Source/_javascript_Core/llint/LowLevelInterpreter.cpp (225139 => 225140)
--- trunk/Source/_javascript_Core/llint/LowLevelInterpreter.cpp 2017-11-24 19:52:55 UTC (rev 225139)
+++ trunk/Source/_javascript_Core/llint/LowLevelInterpreter.cpp 2017-11-24 21:02:01 UTC (rev 225140)
@@ -333,8 +333,27 @@
#endif
CLoopDoubleRegister d0, d1;
+ struct StackPointerScope {
+ StackPointerScope(CLoopStack& stack)
+ : m_stack(stack)
+ , m_originalStackPointer(stack.currentStackPointer())
+ { }
+
+ ~StackPointerScope()
+ {
+ m_stack.setCurrentStackPointer(m_originalStackPointer);
+ }
+
+ private:
+ CLoopStack& m_stack;
+ void* m_originalStackPointer;
+ };
+
+ CLoopStack& cloopStack = vm->interpreter->cloopStack();
+ StackPointerScope stackPointerScope(cloopStack);
+
lr.opcode = getOpcode(llint_return_to_host);
- sp.vp = vm->interpreter->cloopStack().topOfStack() + 1;
+ sp.vp = cloopStack.currentStackPointer();
cfr.callFrame = vm->topCallFrame;
#ifndef NDEBUG
void* startSP = sp.vp;
Modified: trunk/Source/_javascript_Core/offlineasm/cloop.rb (225139 => 225140)
--- trunk/Source/_javascript_Core/offlineasm/cloop.rb 2017-11-24 19:52:55 UTC (rev 225139)
+++ trunk/Source/_javascript_Core/offlineasm/cloop.rb 2017-11-24 21:02:01 UTC (rev 225140)
@@ -1,4 +1,4 @@
-# Copyright (C) 2012, 2014 Apple Inc. All rights reserved.
+# Copyright (C) 2012-2017 Apple Inc. All rights reserved.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions
@@ -543,6 +543,7 @@
# operands: callTarget, currentFrame, currentPC
def cloopEmitCallSlowPath(operands)
$asm.putc "{"
+ $asm.putc " cloopStack.setCurrentStackPointer(sp.vp);"
$asm.putc " SlowPathReturnType result = #{operands[0].cLabel}(#{operands[1].clDump}, #{operands[2].clDump});"
$asm.putc " decodeResult(result, t0.vp, t1.vp);"
$asm.putc "}"
@@ -549,6 +550,7 @@
end
def cloopEmitCallSlowPathVoid(operands)
+ $asm.putc "cloopStack.setCurrentStackPointer(sp.vp);"
$asm.putc "#{operands[0].cLabel}(#{operands[1].clDump}, #{operands[2].clDump});"
end
@@ -1125,6 +1127,7 @@
# fortunately we don't have to here. All native function calls always
# have a fixed prototype of 1 args: the passed ExecState.
when "cloopCallNative"
+ $asm.putc "cloopStack.setCurrentStackPointer(sp.vp);"
$asm.putc "nativeFunc = #{operands[0].clValue(:nativeFunc)};"
$asm.putc "functionReturnValue = JSValue::decode(nativeFunc(t0.execState));"
$asm.putc "#if USE(JSVALUE32_64)"
Modified: trunk/Source/_javascript_Core/tools/VMInspector.h (225139 => 225140)
--- trunk/Source/_javascript_Core/tools/VMInspector.h 2017-11-24 19:52:55 UTC (rev 225139)
+++ trunk/Source/_javascript_Core/tools/VMInspector.h 2017-11-24 21:02:01 UTC (rev 225140)
@@ -69,8 +69,8 @@
JS_EXPORT_PRIVATE static bool isValidCell(Heap*, JSCell*);
JS_EXPORT_PRIVATE static bool isValidCodeBlock(ExecState*, CodeBlock*);
JS_EXPORT_PRIVATE static CodeBlock* codeBlockForFrame(CallFrame* topCallFrame, unsigned frameNumber);
- JS_EXPORT_PRIVATE static void printCallFrame(CallFrame*, unsigned framesToSkip);
- JS_EXPORT_PRIVATE static void printStack(CallFrame* topCallFrame, unsigned framesToSkip);
+ JS_EXPORT_PRIVATE static void printCallFrame(CallFrame*, unsigned framesToSkip = 0);
+ JS_EXPORT_PRIVATE static void printStack(CallFrame* topCallFrame, unsigned framesToSkip = 0);
JS_EXPORT_PRIVATE static void printValue(JSValue);
private:
