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

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.h (modified) (6 diffs)

trunk/Source/JavaScriptCore/assembler/ProbeStack.h

@@ -26 +26 @@
 #pragma once
 
+#include "CPU.h"
 #include <wtf/HashMap.h>
 #include <wtf/StdLibExtras.h>
@@ -57 +58 @@
     T get(void* logicalAddress)
     {
-        return *physicalAddressFor<T*>(logicalAddress);
+        void* from = physicalAddressFor(logicalAddress);
+        typename std::remove_const<T>::type to { };
+        std::memcpy(&to, from, sizeof(to)); // Use std::memcpy to avoid strict aliasing issues.
+        return to;
+    }
+    template<typename T>
+    T get(void* logicalBaseAddress, ptrdiff_t offset)
+    {
+        return get<T>(reinterpret_cast<uint8_t*>(logicalBaseAddress) + offset);
     }
 
@@ -63 +72 @@
     void set(void* logicalAddress, T value)
     {
-        m_dirtyBits |= dirtyBitFor(logicalAddress);
-        *physicalAddressFor<T*>(logicalAddress) = value;
+        if (sizeof(T) <= s_chunkSize)
+            m_dirtyBits |= dirtyBitFor(logicalAddress);
+        else {
+            size_t numberOfChunks = roundUpToMultipleOf<sizeof(T)>(s_chunkSize) / s_chunkSize;
+            uint8_t* dirtyAddress = reinterpret_cast<uint8_t*>(logicalAddress);
+            for (size_t i = 0; i < numberOfChunks; ++i, dirtyAddress += s_chunkSize)
+                m_dirtyBits |= dirtyBitFor(dirtyAddress);
+        }
+        void* to = physicalAddressFor(logicalAddress);
+        std::memcpy(to, &value, sizeof(T)); // Use std::memcpy to avoid strict aliasing issues.
+    }
+    template<typename T>
+    void set(void* logicalBaseAddress, ptrdiff_t offset, T value)
+    {
+        set<T>(reinterpret_cast<uint8_t*>(logicalBaseAddress) + offset, value);
     }
 
@@ -75 +97 @@
 
 private:
-    uintptr_t dirtyBitFor(void* logicalAddress)
+    uint64_t dirtyBitFor(void* logicalAddress)
     {
         uintptr_t offset = reinterpret_cast<uintptr_t>(logicalAddress) & s_pageMask;
-        return static_cast<uintptr_t>(1) << (offset >> s_chunkSizeShift);
-    }
-
-    template<typename T, typename = typename std::enable_if<std::is_pointer<T>::value>::type>
-    T physicalAddressFor(void* logicalAddress)
-    {
-        uintptr_t offset = reinterpret_cast<uintptr_t>(logicalAddress) & s_pageMask;
-        void* physicalAddress = reinterpret_cast<uint8_t*>(&m_buffer) + offset;
-        return reinterpret_cast<T>(physicalAddress);
+        return static_cast<uint64_t>(1) << (offset >> s_chunkSizeShift);
+    }
+
+    void* physicalAddressFor(void* logicalAddress)
+    {
+        return reinterpret_cast<uint8_t*>(logicalAddress) + m_physicalAddressOffset;
     }
 
@@ -92 +111 @@
 
     void* m_baseLogicalAddress { nullptr };
-    uintptr_t m_dirtyBits { 0 };
-
+    ptrdiff_t m_physicalAddressOffset;
+    uint64_t m_dirtyBits { 0 };
+
+#if ASAN_ENABLED
+    // The ASan stack may contain poisoned words that may be manipulated at ASan's discretion.
+    // We would never touch those words anyway, but let's ensure that the page size is set
+    // such that the chunk size is guaranteed to be exactly sizeof(uintptr_t) so that we won't
+    // inadvertently overwrite one of ASan's words on the stack when we copy back the dirty
+    // chunks.
+    // FIXME: we should consider using the same page size for both ASan and non-ASan builds.
+    // https://bugs.webkit.org/show_bug.cgi?id=176961
+    static constexpr size_t s_pageSize = 64 * sizeof(uintptr_t); // because there are 64 bits in m_dirtyBits.
+#else // not ASAN_ENABLED
     static constexpr size_t s_pageSize = 1024;
+#endif // ASAN_ENABLED
     static constexpr uintptr_t s_pageMask = s_pageSize - 1;
-    static constexpr size_t s_chunksPerPage = sizeof(uintptr_t) * 8; // sizeof(m_dirtyBits) in bits.
+    static constexpr size_t s_chunksPerPage = sizeof(uint64_t) * 8; // number of bits in m_dirtyBits.
     static constexpr size_t s_chunkSize = s_pageSize / s_chunksPerPage;
     static constexpr uintptr_t s_chunkMask = s_chunkSize - 1;
-#if USE(JSVALUE64)
+#if ASAN_ENABLED
+    static_assert(s_chunkSize == sizeof(uintptr_t), "bad chunkSizeShift");
+    static constexpr size_t s_chunkSizeShift = is64Bit() ? 3 : 2;
+#else // no ASAN_ENABLED
     static constexpr size_t s_chunkSizeShift = 4;
-#else
-    static constexpr size_t s_chunkSizeShift = 5;
-#endif
+#endif // ASAN_ENABLED
     static_assert(s_pageSize > s_chunkSize, "bad pageSize or chunkSize");
     static_assert(s_chunkSize == (1 << s_chunkSizeShift), "bad chunkSizeShift");
-
 
     typedef typename std::aligned_storage<s_pageSize, std::alignment_of<uintptr_t>::value>::type Buffer;
@@ -121 +152 @@
     Stack(Stack&& other);
 
-    void* lowWatermark() { return m_lowWatermark; }
-
-    template<typename T>
-    typename std::enable_if<!std::is_same<double, typename std::remove_cv<T>::type>::value, T>::type get(void* address)
-    {
-        Page* page = pageFor(address);
-        return page->get<T>(address);
-    }
-
-    template<typename T, typename = typename std::enable_if<!std::is_same<double, typename std::remove_cv<T>::type>::value>::type>
-    void set(void* address, T value)
-    {
-        Page* page = pageFor(address);
-        page->set<T>(address, value);
-
+    void* lowWatermark()
+    {
         // We use the chunkAddress for the low watermark because we'll be doing write backs
         // to the stack in increments of chunks. Hence, we'll treat the lowest address of
         // the chunk as the low watermark of any given set address.
-        void* chunkAddress = Page::chunkAddressFor(address);
-        if (chunkAddress < m_lowWatermark)
-            m_lowWatermark = chunkAddress;
-    }
-
-    template<typename T>
-    typename std::enable_if<std::is_same<double, typename std::remove_cv<T>::type>::value, T>::type get(void* address)
+        return Page::chunkAddressFor(m_lowWatermark);
+    }
+
+    template<typename T>
+    T get(void* address)
     {
         Page* page = pageFor(address);
-        return bitwise_cast<double>(page->get<uint64_t>(address));
-    }
-
-    template<typename T, typename = typename std::enable_if<std::is_same<double, typename std::remove_cv<T>::type>::value>::type>
-    void set(void* address, double value)
-    {
-        set<uint64_t>(address, bitwise_cast<uint64_t>(value));
+        return page->get<T>(address);
+    }
+    template<typename T>
+    T get(void* logicalBaseAddress, ptrdiff_t offset)
+    {
+        return get<T>(reinterpret_cast<uint8_t*>(logicalBaseAddress) + offset);
+    }
+
+    template<typename T>
+    void set(void* address, T value)
+    {
+        Page* page = pageFor(address);
+        page->set<T>(address, value);
+
+        if (address < m_lowWatermark)
+            m_lowWatermark = address;
+    }
+
+    template<typename T>
+    void set(void* logicalBaseAddress, ptrdiff_t offset, T value)
+    {
+        set<T>(reinterpret_cast<uint8_t*>(logicalBaseAddress) + offset, value);
     }