Changeset 189517 in webkit

Timestamp:

Sep 8, 2015 5:19:15 PM (9 years ago)

Author:

mark.lam@apple.com

Message:

GC stack scan should include ABI red zone.
​https://bugs.webkit.org/show_bug.cgi?id=148976

Reviewed by Geoffrey Garen and Benjamin Poulain.

Source/JavaScriptCore:

The x86_64 ABI section 3.2.2[1] and ARM64 ABI[2] both state that there is a
128 byte red zone below the stack pointer (reserved by the OS), and that
"functions may use this area for temporary data that is not needed across
function calls".

Hence, it is possible for a thread to store JSCell pointers in the red zone
area, and the conservative GC thread scanner needs to scan that area as well.

Note: the red zone should not be scanned for the GC thread itself (in
gatherFromCurrentThread()). This because we're guaranteed that there will
be GC frames below the lowest (top of stack) frame that we need to scan.
Hence, we are guaranteed that there are no red zone areas there containing
JSObject pointers of relevance.

No test added for this issue because the issue relies on:

1. the compiler tool chain generating code that stores local variables containing the sole reference to a JS object (that needs to be kept alive) in the stack red zone, and
2. GC has to run on another thread while that red zone containing the JS object reference is in use.

These conditions require a race that cannot be reliably reproduced.

[1]: ​http://people.freebsd.org/~obrien/amd64-elf-abi.pdf
[2]: ​https://developer.apple.com/library/ios/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARM64FunctionCallingConventions.html#//apple_ref/doc/uid/TP40013702-SW7

• heap/MachineStackMarker.cpp:

(JSC::MachineThreads::Thread::Thread):
(JSC::MachineThreads::Thread::createForCurrentThread):
(JSC::MachineThreads::Thread::freeRegisters):
(JSC::osRedZoneAdjustment):
(JSC::MachineThreads::Thread::captureStack):

Source/WTF:

• wtf/StackBounds.h:

(WTF::StackBounds::origin):
(WTF::StackBounds::end):
(WTF::StackBounds::size):

Location:

trunk/Source

Files:

• JavaScriptCore/ChangeLog (modified) (1 diff)
• JavaScriptCore/heap/MachineStackMarker.cpp (modified) (4 diffs)
• WTF/ChangeLog (modified) (1 diff)
• WTF/wtf/StackBounds.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-09-08  Mark Lam  <mark.lam@apple.com>
+
+        GC stack scan should include ABI red zone.
+        https://bugs.webkit.org/show_bug.cgi?id=148976
+
+        Reviewed by Geoffrey Garen and Benjamin Poulain.
+
+        The x86_64 ABI section 3.2.2[1] and ARM64 ABI[2] both state that there is a
+        128 byte red zone below the stack pointer (reserved by the OS), and that
+        "functions may use this area for temporary data that is not needed across
+        function calls".
+
+        Hence, it is possible for a thread to store JSCell pointers in the red zone
+        area, and the conservative GC thread scanner needs to scan that area as well.
+
+        Note: the red zone should not be scanned for the GC thread itself (in
+        gatherFromCurrentThread()).  This because we're guaranteed that there will
+        be GC frames below the lowest (top of stack) frame that we need to scan.
+        Hence, we are guaranteed that there are no red zone areas there containing
+        JSObject pointers of relevance.
+
+        No test added for this issue because the issue relies on:
+        1. the compiler tool chain generating code that stores local variables
+           containing the sole reference to a JS object (that needs to be kept
+           alive) in the stack red zone, and
+        2. GC has to run on another thread while that red zone containing the
+           JS object reference is in use. 
+
+        These conditions require a race that cannot be reliably reproduced.
+
+        [1]: http://people.freebsd.org/~obrien/amd64-elf-abi.pdf
+        [2]: https://developer.apple.com/library/ios/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARM64FunctionCallingConventions.html#//apple_ref/doc/uid/TP40013702-SW7
+
+        * heap/MachineStackMarker.cpp:
+        (JSC::MachineThreads::Thread::Thread):
+        (JSC::MachineThreads::Thread::createForCurrentThread):
+        (JSC::MachineThreads::Thread::freeRegisters):
+        (JSC::osRedZoneAdjustment):
+        (JSC::MachineThreads::Thread::captureStack):
+
 2015-09-08  Geoffrey Garen  <ggaren@apple.com>
 

trunk/Source/JavaScriptCore/heap/MachineStackMarker.cpp

@@ -161 +161 @@
     WTF_MAKE_FAST_ALLOCATED;
 
-    Thread(const PlatformThread& platThread, void* base)
+    Thread(const PlatformThread& platThread, void* base, void* end)
         : platformThread(platThread)
         , stackBase(base)
+        , stackEnd(end)
     {
 #if USE(PTHREADS) && !OS(WINDOWS) && !OS(DARWIN) && defined(SA_RESTART)
@@ -196 +197 @@
     static Thread* createForCurrentThread()
     {
-        return new Thread(getCurrentPlatformThread(), wtfThreadData().stack().origin());
+        auto stackBounds = wtfThreadData().stack();
+        return new Thread(getCurrentPlatformThread(), stackBounds.origin(), stackBounds.end());
     }
 
@@ -242 +244 @@
     PlatformThread platformThread;
     void* stackBase;
+    void* stackEnd;
 #if OS(WINDOWS)
     HANDLE platformThreadHandle;
@@ -511 +514 @@
 }
 
+static inline int osRedZoneAdjustment()
+{
+    int redZoneAdjustment = 0;
+#if !OS(WINDOWS)
+#if CPU(X86_64)
+    // See http://people.freebsd.org/~obrien/amd64-elf-abi.pdf Section 3.2.2.
+    redZoneAdjustment = -128;
+#elif CPU(ARM64)
+    // See https://developer.apple.com/library/ios/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARM64FunctionCallingConventions.html#//apple_ref/doc/uid/TP40013702-SW7
+    redZoneAdjustment = -128;
+#endif
+#endif // OS(DARWIN)
+    return redZoneAdjustment;
+}
+
 std::pair<void*, size_t> MachineThreads::Thread::captureStack(void* stackTop)
 {
-    void* begin = stackBase;
-    void* end = reinterpret_cast<void*>(
-        WTF::roundUpToMultipleOf<sizeof(void*)>(reinterpret_cast<uintptr_t>(stackTop)));
-    if (begin > end)
-        std::swap(begin, end);
-    return std::make_pair(begin, static_cast<char*>(end) - static_cast<char*>(begin));
+    char* begin = reinterpret_cast_ptr<char*>(stackBase);
+    char* end = reinterpret_cast_ptr<char*>(WTF::roundUpToMultipleOf<sizeof(void*)>(reinterpret_cast<uintptr_t>(stackTop)));
+    ASSERT(begin >= end);
+
+    char* endWithRedZone = end + osRedZoneAdjustment();
+    ASSERT(WTF::roundUpToMultipleOf<sizeof(void*)>(reinterpret_cast<uintptr_t>(endWithRedZone)) == reinterpret_cast<uintptr_t>(endWithRedZone));
+
+    if (endWithRedZone < stackEnd)
+        endWithRedZone = reinterpret_cast_ptr<char*>(stackEnd);
+
+    std::swap(begin, endWithRedZone);
+    return std::make_pair(begin, endWithRedZone - begin);
 }
 

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2015-09-08  Mark Lam  <mark.lam@apple.com>
+
+        GC stack scan should include ABI red zone.
+        https://bugs.webkit.org/show_bug.cgi?id=148976
+
+        Reviewed by Geoffrey Garen and Benjamin Poulain.
+
+        * wtf/StackBounds.h:
+        (WTF::StackBounds::origin):
+        (WTF::StackBounds::end):
+        (WTF::StackBounds::size):
+
 2015-09-04  Csaba Osztrogonác  <ossy@webkit.org>
 

trunk/Source/WTF/wtf/StackBounds.h

@@ -55 +55 @@
     }
 
+    void* end() const
+    {
+        ASSERT(m_bound);
+        return m_bound;
+    }
+    
     size_t size() const
     {