Bug 1463596: Ensure that WritableTargetFunction correctly handles changing of protection attributes across regions that straddle page boundaries and have different initial protection attributes; r=handyman
authorAaron Klotz <aklotz@mozilla.com>
Wed, 23 May 2018 16:50:49 -0600
changeset 420957 3518510139bbb70636739b271fb6fe602e5c644b
parent 420956 55e6a68a50561a86d2fee9b56626a9b04930c9fd
child 420958 b7688ef14e3ae06221387df0820affe16f9b98a1
push id34083
push userapavel@mozilla.com
push dateSat, 02 Jun 2018 23:03:25 +0000
treeherdermozilla-central@1f62ecdf59b6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershandyman
bugs1463596
milestone62.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1463596: Ensure that WritableTargetFunction correctly handles changing of protection attributes across regions that straddle page boundaries and have different initial protection attributes; r=handyman
mozglue/misc/interceptor/TargetFunction.h
--- a/mozglue/misc/interceptor/TargetFunction.h
+++ b/mozglue/misc/interceptor/TargetFunction.h
@@ -5,80 +5,156 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 #ifndef mozilla_interceptor_TargetFunction_h
 #define mozilla_interceptor_TargetFunction_h
 
 #include "mozilla/Assertions.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/Maybe.h"
+#include "mozilla/Tuple.h"
 #include "mozilla/Types.h"
+#include "mozilla/Vector.h"
 
 #include <memory>
 
 namespace mozilla {
 namespace interceptor {
 
 template <typename MMPolicy>
 class MOZ_STACK_CLASS WritableTargetFunction final
 {
+  class AutoProtect final
+  {
+    using ProtectParams = Tuple<uintptr_t, uint32_t>;
+
+  public:
+    explicit AutoProtect(const MMPolicy& aMMPolicy)
+      : mMMPolicy(aMMPolicy)
+    {
+    }
+
+    AutoProtect(const MMPolicy& aMMPolicy, uintptr_t aAddr, size_t aNumBytes,
+                uint32_t aNewProt)
+      : mMMPolicy(aMMPolicy)
+    {
+      const uint32_t pageSize = MMPolicy::GetPageSize();
+      const uintptr_t limit = aAddr + aNumBytes - 1;
+      const uintptr_t limitPageNum = limit / pageSize;
+      const uintptr_t basePageNum = aAddr / pageSize;
+      const uintptr_t numPagesToChange = limitPageNum - basePageNum + 1;
+
+      // We'll use the base address of the page instead of aAddr
+      uintptr_t curAddr = basePageNum * pageSize;
+
+      // Now change the protection on each page
+      for (uintptr_t curPage = 0; curPage < numPagesToChange;
+           ++curPage, curAddr += pageSize) {
+        uint32_t prevProt;
+        if (!aMMPolicy.Protect(reinterpret_cast<void*>(curAddr), pageSize,
+                               aNewProt, &prevProt)) {
+          Clear();
+          return;
+        }
+
+        // Save the previous protection for curAddr so that we can revert this
+        // in the destructor.
+        if (!mProtects.append(MakeTuple(curAddr, prevProt))) {
+          Clear();
+          return;
+        }
+      }
+    }
+
+    AutoProtect(AutoProtect&& aOther)
+      : mMMPolicy(aOther.mMMPolicy)
+      , mProtects(std::move(aOther.mProtects))
+    {
+      aOther.mProtects.clear();
+    }
+
+    ~AutoProtect()
+    {
+      Clear();
+    }
+
+    explicit operator bool() const
+    {
+      return !mProtects.empty();
+    }
+
+    AutoProtect(const AutoProtect&) = delete;
+    AutoProtect& operator=(const AutoProtect&) = delete;
+    AutoProtect& operator=(AutoProtect&&) = delete;
+
+  private:
+    void Clear()
+    {
+      const uint32_t pageSize = MMPolicy::GetPageSize();
+      for (auto&& entry : mProtects) {
+        uint32_t prevProt;
+        DebugOnly<bool> ok =
+          mMMPolicy.Protect(reinterpret_cast<void*>(Get<0>(entry)), pageSize,
+                            Get<1>(entry), &prevProt);
+        MOZ_ASSERT(ok);
+      }
+
+      mProtects.clear();
+    }
+
+  private:
+    const MMPolicy&           mMMPolicy;
+    // We include two entries of inline storage as that is most common in the
+    // worst case.
+    Vector<ProtectParams, 2>  mProtects;
+  };
+
 public:
   /**
    * Used to initialize an invalid WritableTargetFunction, thus signalling an error.
    */
   explicit WritableTargetFunction(const MMPolicy& aMMPolicy)
     : mMMPolicy(aMMPolicy)
     , mFunc(0)
     , mNumBytes(0)
     , mOffset(0)
     , mStartWriteOffset(0)
-    , mPrevProt(0)
     , mAccumulatedStatus(false)
+    , mProtect(aMMPolicy)
   {
   }
 
   WritableTargetFunction(const MMPolicy& aMMPolicy, uintptr_t aFunc,
                          size_t aNumBytes)
     : mMMPolicy(aMMPolicy)
     , mFunc(aFunc)
     , mNumBytes(aNumBytes)
     , mOffset(0)
     , mStartWriteOffset(0)
-    , mPrevProt(0)
     , mAccumulatedStatus(true)
+    , mProtect(aMMPolicy, aFunc, aNumBytes, PAGE_EXECUTE_READWRITE)
   {
-    aMMPolicy.Protect(reinterpret_cast<void*>(aFunc), aNumBytes,
-                      PAGE_EXECUTE_READWRITE, &mPrevProt);
   }
 
   WritableTargetFunction(WritableTargetFunction&& aOther)
     : mMMPolicy(aOther.mMMPolicy)
     , mFunc(aOther.mFunc)
     , mNumBytes(aOther.mNumBytes)
     , mOffset(aOther.mOffset)
     , mStartWriteOffset(aOther.mStartWriteOffset)
-    , mPrevProt(aOther.mPrevProt)
     , mLocalBytes(std::move(aOther.mLocalBytes))
     , mAccumulatedStatus(aOther.mAccumulatedStatus)
+    , mProtect(std::move(aOther.mProtect))
   {
-    aOther.mPrevProt = 0;
     aOther.mAccumulatedStatus = false;
   }
 
   ~WritableTargetFunction()
   {
-    if (!mPrevProt) {
-      return;
-    }
-
     MOZ_ASSERT(mLocalBytes.empty(), "Did you forget to call Commit?");
-
-    DebugOnly<bool> ok = mMMPolicy.Protect(reinterpret_cast<void*>(mFunc),
-                                           mNumBytes, mPrevProt, &mPrevProt);
-    MOZ_ASSERT(ok);
   }
 
   WritableTargetFunction(const WritableTargetFunction&) = delete;
   WritableTargetFunction& operator=(const WritableTargetFunction&) = delete;
   WritableTargetFunction& operator=(WritableTargetFunction&&) = delete;
 
   /**
    * @return true if data was successfully committed.
@@ -102,17 +178,17 @@ public:
 
     mMMPolicy.FlushInstructionCache();
     mLocalBytes.clear();
     return true;
   }
 
   explicit operator bool() const
   {
-    return mPrevProt && mAccumulatedStatus;
+    return mProtect && mAccumulatedStatus;
   }
 
   void WriteByte(const uint8_t& aValue)
   {
     if (!mLocalBytes.append(aValue)) {
       mAccumulatedStatus = false;
       return;
     }
@@ -220,31 +296,31 @@ public:
   }
 
 private:
   const MMPolicy& mMMPolicy;
   const uintptr_t mFunc;
   const size_t mNumBytes;
   uint32_t mOffset;
   uint32_t mStartWriteOffset;
-  uint32_t mPrevProt;
 
   // In an ideal world, we'd only read 5 bytes on 32-bit and 13 bytes on 64-bit,
   // to match the minimum bytes that we need to write in in order to patch the
   // target function. Since the actual opcodes will often require us to pull in
   // extra bytes above that minimum, we set the inline storage to be larger than
   // those minima in an effort to give the Vector extra wiggle room before it
   // needs to touch the heap.
 #if defined(_M_IX86)
   static const size_t kInlineStorage = 16;
 #elif defined(_M_X64)
   static const size_t kInlineStorage = 32;
 #endif
   Vector<uint8_t, kInlineStorage> mLocalBytes;
   bool mAccumulatedStatus;
+  AutoProtect mProtect;
 };
 
 template <typename MMPolicy>
 class ReadOnlyTargetBytes;
 
 template <>
 class ReadOnlyTargetBytes<MMPolicyInProcess>
 {