Changeset 222058 in webkit for trunk/Source/JavaScriptCore/assembler/ProbeStack.cpp

Timestamp:

Sep 14, 2017, 4:08:54 PM (8 years ago)

Author:

[email protected]

Message:

AddressSanitizer: stack-buffer-underflow in JSC::Probe::Page::Page
​https://bugs.webkit.org/show_bug.cgi?id=176874
<rdar://problem/34436415>

Reviewed by Saam Barati.

Source/JavaScriptCore:

1. Make Probe::Stack play nice with ASan by:

1. using a local memcpy implementation that suppresses ASan on ASan builds. We don't want to use std:memcpy() which validates stack memory because we are intentionally copying stack memory beyond the current frame.

2. changing Stack::s_chunkSize to equal sizeof(uintptr_t) on ASan builds. This ensures that Page::flushWrites() only writes stack memory that was modified by a probe. The probes should only modify stack memory that belongs to JSC stack data structures. We don't want to inadvertently modify adjacent words that may belong to ASan (which may happen if s_chunkSize is larger than sizeof(uintptr_t)).

3. fixing a bug in Page dirtyBits management for when the size of the value to write is greater than s_chunkSize. The fix in generic, but in practice, this currently only manifests on 32-bit ASan builds because sizeof(uintptr_t) and s_chunkSize are 32-bit, and we may write 64-bit values.

4. making Page::m_dirtyBits 64 bits always. This maximizes the number of s_chunksPerPage we can have even on ASan builds.

2. Fixed the bottom most Probe::Context and Probe::Stack get/set methods to use std::memcpy to avoid strict aliasing issues.

3. Optimized the implementation of Page::physicalAddressFor().

4. Optimized the implementation of Stack::set() in the recording of the low watermark. We just record the lowest raw pointer now, and only compute the alignment to its chuck boundary later when the low watermark is requested.

5. Changed a value in testmasm to make the test less vulnerable to rounding issues.

No new test needed because this is already covered by testmasm with ASan enabled.

• assembler/ProbeContext.h:

(JSC::Probe::CPUState::gpr const):
(JSC::Probe::CPUState::spr const):
(JSC::Probe::Context::gpr):
(JSC::Probe::Context::spr):
(JSC::Probe::Context::fpr):
(JSC::Probe::Context::gprName):
(JSC::Probe::Context::sprName):
(JSC::Probe::Context::fprName):
(JSC::Probe::Context::gpr const):
(JSC::Probe::Context::spr const):
(JSC::Probe::Context::fpr const):
(JSC::Probe::Context::pc):
(JSC::Probe::Context::fp):
(JSC::Probe::Context::sp):
(JSC::Probe:: const): Deleted.

• assembler/ProbeStack.cpp:

(JSC::Probe::copyStackPage):
(JSC::Probe::Page::Page):
(JSC::Probe::Page::flushWrites):

• assembler/ProbeStack.h:

(JSC::Probe::Page::get):
(JSC::Probe::Page::set):
(JSC::Probe::Page::dirtyBitFor):
(JSC::Probe::Page::physicalAddressFor):
(JSC::Probe::Stack::lowWatermark):
(JSC::Probe::Stack::get):
(JSC::Probe::Stack::set):

• assembler/testmasm.cpp:

(JSC::testProbeModifiesStackValues):

Source/WTF:

Added a convenience version of roundUpToMultipleOf() so that it can be applied to
pointers without the client having to cast explicitly.

• wtf/StdLibExtras.h:

(WTF::roundUpToMultipleOf):

File:

• trunk/Source/JavaScriptCore/assembler/ProbeStack.cpp (modified) (3 diffs)

trunk/Source/JavaScriptCore/assembler/ProbeStack.cpp

@@ -28 +28 @@
 
 #include <memory>
+#include <wtf/StdLibExtras.h>
 
 #if ENABLE(MASM_PROBE)
@@ -34 +35 @@
 namespace Probe {
 
+#if ASAN_ENABLED
+// FIXME: we should consider using the copy function for both ASan and non-ASan builds.
+// https://bugs.webkit.org/show_bug.cgi?id=176961
+SUPPRESS_ASAN
+static void copyStackPage(void* dst, void* src, size_t size)
+{
+    ASSERT(roundUpToMultipleOf<sizeof(uintptr_t)>(dst) == dst);
+    ASSERT(roundUpToMultipleOf<sizeof(uintptr_t)>(src) == src);
+    
+    uintptr_t* dstPointer = reinterpret_cast<uintptr_t*>(dst);
+    uintptr_t* srcPointer = reinterpret_cast<uintptr_t*>(src);
+    for (; size; size -= sizeof(uintptr_t))
+        *dstPointer++ = *srcPointer++;
+}
+#else
+#define copyStackPage(dst, src, size) std::memcpy(dst, src, size);
+#endif
+
 Page::Page(void* baseAddress)
     : m_baseLogicalAddress(baseAddress)
+    , m_physicalAddressOffset(reinterpret_cast<uint8_t*>(&m_buffer) - reinterpret_cast<uint8_t*>(baseAddress))
 {
-    memcpy(&m_buffer, baseAddress, s_pageSize);
+    copyStackPage(&m_buffer, baseAddress, s_pageSize);
 }
 
 void Page::flushWrites()
 {
-    uintptr_t dirtyBits = m_dirtyBits;
+    uint64_t dirtyBits = m_dirtyBits;
     size_t offset = 0;
     while (dirtyBits) {
@@ -57 +77 @@
             uint8_t* src = reinterpret_cast<uint8_t*>(&m_buffer) + startOffset;
             uint8_t* dst = reinterpret_cast<uint8_t*>(m_baseLogicalAddress) + startOffset;
-            memcpy(dst, src, size);
+            copyStackPage(dst, src, size);
         }
         dirtyBits = dirtyBits >> 1;