Bug 951827 - Part 1: Don't undo hooks to functions that have since changed. r=ehsan
authorDavid Major <dmajor@mozilla.com>
Fri, 14 Feb 2014 14:52:29 -0800
changeset 170030 96cf7c13a8cbcc760d438ee02027e5d29edb0e3b
parent 170029 776b40c41bd9681e61da13416552b4ff88b1c1d4
child 170031 236ed76eea8b854cb572dfc9e3e9bb076ec29af0
push id270
push userpvanderbeken@mozilla.com
push dateThu, 06 Mar 2014 09:24:21 +0000
reviewersehsan
bugs951827
milestone30.0a1
Bug 951827 - Part 1: Don't undo hooks to functions that have since changed. r=ehsan
toolkit/xre/nsWindowsDllInterceptor.h
--- a/toolkit/xre/nsWindowsDllInterceptor.h
+++ b/toolkit/xre/nsWindowsDllInterceptor.h
@@ -71,29 +71,36 @@ class WindowsDllNopSpacePatcher
   HMODULE mModule;
 
   // Dumb array for remembering the addresses of functions we've patched.
   // (This should be nsTArray, but non-XPCOM code uses this class.)
   static const size_t maxPatchedFns = 128;
   byteptr_t mPatchedFns[maxPatchedFns];
   int mPatchedFnsLen;
 
+  static const uint16_t opTrampolineShortJump = 0xf9eb;
+
 public:
   WindowsDllNopSpacePatcher()
     : mModule(0)
     , mPatchedFnsLen(0)
   {}
 
   ~WindowsDllNopSpacePatcher()
   {
     // Restore the mov edi, edi to the beginning of each function we patched.
 
     for (int i = 0; i < mPatchedFnsLen; i++) {
       byteptr_t fn = mPatchedFns[i];
 
+      // If other code has changed this function, it is not safe to modify.
+      if (*((uint16_t*)fn) != opTrampolineShortJump) {
+        continue;
+      }
+
       // Ensure we can write to the code.
       DWORD op;
       if (!VirtualProtectEx(GetCurrentProcess(), fn, 2, PAGE_EXECUTE_READWRITE, &op)) {
         // printf("VirtualProtectEx failed! %d\n", GetLastError());
         continue;
       }
 
       // mov edi, edi
@@ -185,17 +192,17 @@ public:
     fn[-5] = 0xe9; // jmp
     *((intptr_t*)(fn - 4)) = hookDest - (uintptr_t)(fn); // target displacement
 
     // Set origFunc here, because after this point, hookDest might be called,
     // and hookDest might use the origFunc pointer.
     *origFunc = fn + 2;
 
     // Short jump up into our long jump.
-    *((uint16_t*)(fn)) = 0xf9eb; // jmp $-5
+    *((uint16_t*)(fn)) = opTrampolineShortJump; // jmp $-5
 
     // I think this routine is safe without this, but it can't hurt.
     FlushInstructionCache(GetCurrentProcess(),
                           /* ignored */ nullptr,
                           /* ignored */ 0);
 
     return true;
   }
@@ -225,16 +232,35 @@ public:
 #if defined(_M_IX86)
       size_t nBytes = 1 + sizeof(intptr_t);
 #elif defined(_M_X64)
       size_t nBytes = 2 + sizeof(intptr_t);
 #else
 #error "Unknown processor type"
 #endif
       byteptr_t origBytes = *((byteptr_t *)p);
+
+      // If CreateTrampoline failed, we may have an empty trampoline.
+      if (!origBytes) {
+        continue;
+      }
+
+      // If other code has changed this function, it is not safe to modify.
+#if defined(_M_IX86)
+      if (*origBytes != opTrampolineRelativeJump) {
+        continue;
+      }
+#elif defined(_M_X64)
+      if (*((uint16_t*)origBytes) != opTrampolineRegLoad) {
+        continue;
+      }
+#else
+#error "Unknown processor type"
+#endif
+
       // ensure we can modify the original code
       DWORD op;
       if (!VirtualProtectEx(GetCurrentProcess(), origBytes, nBytes, PAGE_EXECUTE_READWRITE, &op)) {
         //printf ("VirtualProtectEx failed! %d\n", GetLastError());
         continue;
       }
       // Remove the hook by making the original function jump directly
       // in the trampoline.
@@ -314,16 +340,19 @@ public:
 
     return true;
   }
 
 protected:
   const static int kPageSize = 4096;
   const static int kHookSize = 128;
 
+  const static uint8_t opTrampolineRelativeJump = 0xe9;
+  const static uint16_t opTrampolineRegLoad = 0xbb49;
+
   HMODULE mModule;
   byteptr_t mHookPage;
   int mMaxHooks;
   int mCurHooks;
 
   void CreateTrampoline(void *origFunction,
                         intptr_t dest,
                         void **outTramp)
@@ -556,35 +585,33 @@ protected:
 
 #if defined(_M_IX86)
     if (pJmp32 >= 0) {
       // Jump directly to the original target of the jump instead of jumping to the
       // original function.
       // Adjust jump target displacement to jump location in the trampoline.
       *((intptr_t*)(tramp+pJmp32+1)) += origBytes - tramp;
     } else {
-      tramp[nBytes] = 0xE9; // jmp
+      tramp[nBytes] = opTrampolineRelativeJump; // jmp
       *((intptr_t*)(tramp+nBytes+1)) = (intptr_t)trampDest - (intptr_t)(tramp+nBytes+5); // target displacement
     }
 #elif defined(_M_X64)
     // If JMP32 opcode found, we don't insert to trampoline jump 
     if (pJmp32 >= 0) {
       // mov r11, address
-      tramp[pJmp32]   = 0x49;
-      tramp[pJmp32+1] = 0xbb;
+      *((uint16_t*)(tramp+pJmp32)) = opTrampolineRegLoad;
       *((intptr_t*)(tramp+pJmp32+2)) = (intptr_t)directJmpAddr;
 
       // jmp r11
       tramp[pJmp32+10] = 0x41;
       tramp[pJmp32+11] = 0xff;
       tramp[pJmp32+12] = 0xe3;
     } else {
       // mov r11, address
-      tramp[nBytes] = 0x49;
-      tramp[nBytes+1] = 0xbb;
+      *((uint16_t*)(tramp+nBytes)) = opTrampolineRegLoad;
       *((intptr_t*)(tramp+nBytes+2)) = (intptr_t)trampDest;
 
       // jmp r11
       tramp[nBytes+10] = 0x41;
       tramp[nBytes+11] = 0xff;
       tramp[nBytes+12] = 0xe3;
     }
 #endif
@@ -596,23 +623,21 @@ protected:
     DWORD op;
     if (!VirtualProtectEx(GetCurrentProcess(), origFunction, nBytes, PAGE_EXECUTE_READWRITE, &op)) {
       //printf ("VirtualProtectEx failed! %d\n", GetLastError());
       return;
     }
 
 #if defined(_M_IX86)
     // now modify the original bytes
-    origBytes[0] = 0xE9; // jmp
+    origBytes[0] = opTrampolineRelativeJump; // jmp
     *((intptr_t*)(origBytes+1)) = dest - (intptr_t)(origBytes+5); // target displacement
 #elif defined(_M_X64)
     // mov r11, address
-    origBytes[0] = 0x49;
-    origBytes[1] = 0xbb;
-
+    *((uint16_t*)(origBytes)) = opTrampolineRegLoad;
     *((intptr_t*)(origBytes+2)) = dest;
 
     // jmp r11
     origBytes[10] = 0x41;
     origBytes[11] = 0xff;
     origBytes[12] = 0xe3;
 #endif