Bug 1546546: Part 3 - TestDllInterceptor must leave intercepted functions operable r=aklotz
authorDavid Parks <dparks@mozilla.com>
Wed, 08 May 2019 00:26:59 +0000
changeset 534872 c04cde900d273ffa3c4990dbfea94495b12e54e1
parent 534871 8f80e7afef7c35617f3637e7cb49246e647d391a
child 534873 690025a0dcd52340c05432a84131d5239df2d61a
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaklotz
bugs1546546
milestone68.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 1546546: Part 3 - TestDllInterceptor must leave intercepted functions operable r=aklotz In part 1, we disabled the unhooking of DLL-intercepted functions at shutdown. The TestDllInterceptor relied on unhooking -- it worked by hooking functions with a "nonsense function" (nullptr) and then immediately unhooking it. That restored the original function behavior. Some hooked functions (e.g. NtWriteFile) are used by functions later in the program (e.g. printf) so the functions need to maintain their behavior. This patch replaces the nonsense function with an identity function that also sets a global boolean as a side-effect. The function is written in machine code. x86-32, x86-64, and aarch64 variants are included. Differential Revision: https://phabricator.services.mozilla.com/D30244
mozglue/tests/interceptor/TestDllInterceptor.cpp
--- a/mozglue/tests/interceptor/TestDllInterceptor.cpp
+++ b/mozglue/tests/interceptor/TestDllInterceptor.cpp
@@ -80,58 +80,45 @@ static WindowsDllInterceptor::FuncHookTy
     orig_rotatePayload;
 
 static payload patched_rotatePayload(payload p) {
   patched_func_called = true;
   return orig_rotatePayload(p);
 }
 
 // Invoke aFunc by taking aArg's contents and using them as aFunc's arguments
-template <typename CallableT, typename... Args,
+template <typename OrigFuncT, typename... Args,
           typename ArgTuple = Tuple<Args...>, size_t... Indices>
-decltype(auto) Apply(CallableT&& aFunc, ArgTuple&& aArgs,
+decltype(auto) Apply(OrigFuncT& aFunc, ArgTuple&& aArgs,
                      std::index_sequence<Indices...>) {
-  return std::forward<CallableT>(aFunc)(
-      Get<Indices>(std::forward<ArgTuple>(aArgs))...);
+  return aFunc(Get<Indices>(std::forward<ArgTuple>(aArgs))...);
 }
 
-template <typename CallableT>
-bool TestFunction(CallableT aFunc);
-
 #define DEFINE_TEST_FUNCTION(calling_convention)                               \
-  template <template <typename I, typename F> class FuncHookT,                 \
-            typename InterceptorT, typename R, typename... Args,               \
-            typename... TestArgs>                                              \
-  bool TestFunction(                                                           \
-      FuncHookT<InterceptorT, R(calling_convention*)(Args...)>&& aFunc,        \
-      bool (*aPred)(R), TestArgs... aArgs) {                                   \
-    using FuncHookType =                                                       \
-        FuncHookT<InterceptorT, R(calling_convention*)(Args...)>;              \
+  template <typename R, typename... Args, typename... TestArgs>                \
+  bool TestFunction(R(calling_convention* aFunc)(Args...), bool (*aPred)(R),   \
+                    TestArgs... aArgs) {                                       \
     using ArgTuple = Tuple<Args...>;                                           \
     using Indices = std::index_sequence_for<Args...>;                          \
     ArgTuple fakeArgs{std::forward<TestArgs>(aArgs)...};                       \
-    return aPred(Apply(std::forward<FuncHookType>(aFunc),                      \
-                       std::forward<ArgTuple>(fakeArgs), Indices()));          \
+    patched_func_called = false;                                               \
+    return aPred(Apply(aFunc, std::forward<ArgTuple>(fakeArgs), Indices())) && \
+           patched_func_called;                                                \
   }                                                                            \
                                                                                \
   /* Specialization for functions returning void */                            \
-  template <template <typename I, typename F> class FuncHookT,                 \
-            typename InterceptorT, typename... Args, typename PredicateT,      \
-            typename... TestArgs>                                              \
-  bool TestFunction(                                                           \
-      FuncHookT<InterceptorT, void(calling_convention*)(Args...)>&& aFunc,     \
-      PredicateT&& aPred, TestArgs... aArgs) {                                 \
-    using FuncHookType =                                                       \
-        FuncHookT<InterceptorT, void(calling_convention*)(Args...)>;           \
+  template <typename PredT, typename... Args, typename... TestArgs>            \
+  bool TestFunction(void(calling_convention * aFunc)(Args...), PredT,          \
+                    TestArgs... aArgs) {                                       \
     using ArgTuple = Tuple<Args...>;                                           \
     using Indices = std::index_sequence_for<Args...>;                          \
     ArgTuple fakeArgs{std::forward<TestArgs>(aArgs)...};                       \
-    Apply(std::forward<FuncHookType>(aFunc), std::forward<ArgTuple>(fakeArgs), \
-          Indices());                                                          \
-    return true;                                                               \
+    patched_func_called = false;                                               \
+    Apply(aFunc, std::forward<ArgTuple>(fakeArgs), Indices());                 \
+    return patched_func_called;                                                \
   }
 
 // C++11 allows empty arguments to macros. clang works just fine. MSVC does the
 // right thing, but it also throws up warning C4003.
 #if defined(_MSC_VER) && !defined(__clang__)
 DEFINE_TEST_FUNCTION(__cdecl)
 #else
 DEFINE_TEST_FUNCTION()
@@ -141,58 +128,175 @@ DEFINE_TEST_FUNCTION()
 DEFINE_TEST_FUNCTION(__stdcall)
 DEFINE_TEST_FUNCTION(__fastcall)
 #endif  // _M_IX86
 
 // Test the hooked function against the supplied predicate
 template <typename OrigFuncT, typename PredicateT, typename... Args>
 bool CheckHook(OrigFuncT& aOrigFunc, const char* aDllName,
                const char* aFuncName, PredicateT&& aPred, Args... aArgs) {
-  if (TestFunction(std::forward<OrigFuncT>(aOrigFunc),
-                   std::forward<PredicateT>(aPred),
+  if (TestFunction(aOrigFunc, std::forward<PredicateT>(aPred),
                    std::forward<Args>(aArgs)...)) {
     printf(
         "TEST-PASS | WindowsDllInterceptor | "
         "Executed hooked function %s from %s\n",
         aFuncName, aDllName);
     return true;
   }
   printf(
       "TEST-FAILED | WindowsDllInterceptor | "
       "Failed to execute hooked function %s from %s\n",
       aFuncName, aDllName);
   return false;
 }
 
+struct InterceptorFunction {
+  static const size_t EXEC_MEMBLOCK_SIZE = 64 * 1024;  // 64K
+
+  static InterceptorFunction& Create() {
+    // Make sure the executable memory is allocated
+    if (!sBlock) {
+      Init();
+    }
+    MOZ_ASSERT(sBlock);
+
+    // Make sure we aren't making more functions than we allocated room for
+    MOZ_RELEASE_ASSERT((sNumInstances + 1) * sizeof(InterceptorFunction) <=
+                       EXEC_MEMBLOCK_SIZE);
+
+    // Grab the next InterceptorFunction from executable memory
+    InterceptorFunction& ret = *reinterpret_cast<InterceptorFunction*>(
+        sBlock + (sNumInstances++ * sizeof(InterceptorFunction)));
+
+    // Set the InterceptorFunction to the code template.
+    auto funcCode = &ret[0];
+    memcpy(funcCode, sInterceptorTemplate, TemplateLength);
+
+    // Fill in the patched_func_called pointer in the template.
+    auto pfPtr = reinterpret_cast<bool**>(&ret[PatchedFuncCalledIndex]);
+    *pfPtr = &patched_func_called;
+    return ret;
+  }
+
+  uint8_t& operator[](size_t i) { return mFuncCode[i]; }
+
+  uint8_t* GetFunction() { return mFuncCode; }
+
+  void SetStub(uintptr_t aStub) {
+    auto pfPtr = reinterpret_cast<uintptr_t*>(&mFuncCode[StubFuncIndex]);
+    *pfPtr = aStub;
+  }
+
+ private:
+  // We intercept functions with short machine-code functions that set a boolean
+  // and run the stub that launches the original function.  Each entry in the
+  // array is the code for one of those interceptor functions.  We cannot
+  // free this memory until the test shuts down.
+  // The templates have spots for the address of patched_func_called
+  // and for the address of the stub function.  Their indices in the byte
+  // array are given as constants below and they appear as blocks of
+  // 0xff bytes in the templates.
+#if defined(_M_X64)
+  //  0: 48 b8 ff ff ff ff ff ff ff ff    movabs rax, &patched_func_called
+  //  a: c6 00 01                         mov    BYTE PTR [rax],0x1
+  //  d: 48 b8 ff ff ff ff ff ff ff ff    movabs rax, &stub_func_ptr
+  // 17: ff e0                            jmp    rax
+  static constexpr uint8_t sInterceptorTemplate[] = {
+      0x48, 0xB8, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
+      0xFF, 0xC6, 0x00, 0x01, 0x48, 0xB8, 0xFF, 0xFF, 0xFF,
+      0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xE0};
+  static const size_t PatchedFuncCalledIndex = 0x2;
+  static const size_t StubFuncIndex = 0xf;
+#elif defined(_M_IX86)
+  // 0: c6 05 ff ff ff ff 01     mov    BYTE PTR &patched_func_called, 0x1
+  // 7: 68 ff ff ff ff           push   &stub_func_ptr
+  // c: c3                       ret
+  static constexpr uint8_t sInterceptorTemplate[] = {
+      0xC6, 0x05, 0xFF, 0xFF, 0xFF, 0xFF, 0x01,
+      0x68, 0xFF, 0xFF, 0xFF, 0xFF, 0xC3};
+  static const size_t PatchedFuncCalledIndex = 0x2;
+  static const size_t StubFuncIndex = 0x8;
+#elif defined(_M_ARM64)
+  //  0: 31 00 80 52    movz w17, #0x1
+  //  4: 90 00 00 58    ldr  x16, #16
+  //  8: 11 02 00 39    strb w17, [x16]
+  //  c: 90 00 00 58    ldr  x16, #16
+  // 10: 00 02 1F D6    br   x16
+  // 14: &patched_func_called
+  // 1c: &stub_func_ptr
+  static constexpr uint8_t sInterceptorTemplate[] = {
+      0x31, 0x00, 0x80, 0x52, 0x90, 0x00, 0x00, 0x58, 0x11, 0x02, 0x00, 0x39,
+      0x90, 0x00, 0x00, 0x58, 0x00, 0x02, 0x1F, 0xD6, 0xFF, 0xFF, 0xFF, 0xFF,
+      0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
+  static const size_t PatchedFuncCalledIndex = 0x14;
+  static const size_t StubFuncIndex = 0x1c;
+#else
+#  error "Missing template for architecture"
+#endif
+
+  static const size_t TemplateLength = sizeof(sInterceptorTemplate);
+  uint8_t mFuncCode[TemplateLength];
+
+  InterceptorFunction() = delete;
+  InterceptorFunction(const InterceptorFunction&) = delete;
+  InterceptorFunction& operator=(const InterceptorFunction&) = delete;
+
+  static void Init() {
+    MOZ_ASSERT(!sBlock);
+    sBlock = reinterpret_cast<uint8_t*>(
+        ::VirtualAlloc(nullptr, EXEC_MEMBLOCK_SIZE, MEM_RESERVE | MEM_COMMIT,
+                       PAGE_EXECUTE_READWRITE));
+  }
+
+  static uint8_t* sBlock;
+  static size_t sNumInstances;
+};
+
+uint8_t* InterceptorFunction::sBlock = nullptr;
+size_t InterceptorFunction::sNumInstances = 0;
+
+constexpr uint8_t InterceptorFunction::sInterceptorTemplate[];
+
 // Hook the function and optionally attempt calling it
 template <typename OrigFuncT, size_t N, typename PredicateT, typename... Args>
 bool TestHook(const char (&dll)[N], const char* func, PredicateT&& aPred,
               Args... aArgs) {
   auto orig_func(
       mozilla::MakeUnique<WindowsDllInterceptor::FuncHookType<OrigFuncT>>());
 
   bool successful = false;
-  {
-    WindowsDllInterceptor TestIntercept;
-    TestIntercept.Init(dll);
-    successful = orig_func->Set(TestIntercept, func, nullptr);
-  }
+  WindowsDllInterceptor TestIntercept;
+  TestIntercept.Init(dll);
+
+  InterceptorFunction& interceptorFunc = InterceptorFunction::Create();
+  successful = orig_func->Set(
+      TestIntercept, func,
+      reinterpret_cast<OrigFuncT>(interceptorFunc.GetFunction()));
 
   if (successful) {
+    interceptorFunc.SetStub(reinterpret_cast<uintptr_t>(orig_func->GetStub()));
     printf("TEST-PASS | WindowsDllInterceptor | Could hook %s from %s\n", func,
            dll);
     if (!aPred) {
       printf(
           "TEST-SKIPPED | WindowsDllInterceptor | "
           "Will not attempt to execute patched %s.\n",
           func);
       return true;
     }
 
-    return CheckHook(*orig_func, dll, func, std::forward<PredicateT>(aPred),
+    // Test the DLL function we just hooked.
+    HMODULE module = ::LoadLibrary(dll);
+    FARPROC funcAddr = ::GetProcAddress(module, func);
+    if (!funcAddr) {
+      return false;
+    }
+
+    return CheckHook(reinterpret_cast<OrigFuncT&>(funcAddr), dll, func,
+                     std::forward<PredicateT>(aPred),
                      std::forward<Args>(aArgs)...);
   } else {
     printf(
         "TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to hook %s from "
         "%s\n",
         func, dll);
     return false;
   }
@@ -200,34 +304,45 @@ bool TestHook(const char (&dll)[N], cons
 
 // Detour the function and optionally attempt calling it
 template <typename OrigFuncT, size_t N, typename PredicateT>
 bool TestDetour(const char (&dll)[N], const char* func, PredicateT&& aPred) {
   auto orig_func(
       mozilla::MakeUnique<WindowsDllInterceptor::FuncHookType<OrigFuncT>>());
 
   bool successful = false;
-  {
-    WindowsDllInterceptor TestIntercept;
-    TestIntercept.Init(dll);
-    successful = orig_func->SetDetour(TestIntercept, func, nullptr);
-  }
+  WindowsDllInterceptor TestIntercept;
+  TestIntercept.Init(dll);
+
+  InterceptorFunction& interceptorFunc = InterceptorFunction::Create();
+  successful = orig_func->Set(
+      TestIntercept, func,
+      reinterpret_cast<OrigFuncT>(interceptorFunc.GetFunction()));
 
   if (successful) {
+    interceptorFunc.SetStub(reinterpret_cast<uintptr_t>(orig_func->GetStub()));
     printf("TEST-PASS | WindowsDllInterceptor | Could detour %s from %s\n",
            func, dll);
     if (!aPred) {
       printf(
           "TEST-SKIPPED | WindowsDllInterceptor | "
           "Will not attempt to execute patched %s.\n",
           func);
       return true;
     }
 
-    return CheckHook(*orig_func, dll, func, std::forward<PredicateT>(aPred));
+    // Test the DLL function we just hooked.
+    HMODULE module = ::LoadLibrary(dll);
+    FARPROC funcAddr = ::GetProcAddress(module, func);
+    if (!funcAddr) {
+      return false;
+    }
+
+    return CheckHook(reinterpret_cast<OrigFuncT&>(funcAddr), dll, func,
+                     std::forward<PredicateT>(aPred));
   } else {
     printf(
         "TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Failed to detour %s "
         "from %s\n",
         func, dll);
     return false;
   }
 }
@@ -454,16 +569,19 @@ bool HasApiSetQueryApiSetPresence() {
   }
 
   // Prepare gEmptyUnicodeString for the test
   ::RtlInitUnicodeString(&gEmptyUnicodeString, gEmptyUnicodeStringLiteral);
 
   return true;
 }
 
+// Set this to true to test function unhooking.
+const bool ShouldTestUnhookFunction = false;
+
 #if defined(_M_X64)
 
 // Use VMSharingPolicyUnique for the TenByteInterceptor, as it needs to
 // reserve its trampoline memory in a special location.
 using TenByteInterceptor = mozilla::interceptor::WindowsDllInterceptor<
     mozilla::interceptor::VMSharingPolicyUnique<
         mozilla::interceptor::MMPolicyInProcess,
         mozilla::interceptor::kDefaultTrampolineSize>>;
@@ -484,49 +602,59 @@ bool TestTenByteDetour() {
     return false;
   }
 
   {  // Scope for tenByteInterceptor
     TenByteInterceptor tenByteInterceptor;
     tenByteInterceptor.TestOnlyDetourInit(
         L"ntdll.dll",
         mozilla::interceptor::DetourFlags::eTestOnlyForce10BytePatch);
-    if (!orig_NtMapViewOfSection.SetDetour(tenByteInterceptor,
-                                           "NtMapViewOfSection", nullptr)) {
+
+    InterceptorFunction& interceptorFunc = InterceptorFunction::Create();
+    if (!orig_NtMapViewOfSection.SetDetour(
+            tenByteInterceptor, "NtMapViewOfSection",
+            reinterpret_cast<decltype(&::NtMapViewOfSection)>(
+                interceptorFunc.GetFunction()))) {
       printf(
           "TEST-FAILED | WindowsDllInterceptor | "
           "Failed to hook ntdll!NtMapViewOfSection via 10-byte patch\n");
       return false;
     }
 
+    interceptorFunc.SetStub(
+        reinterpret_cast<uintptr_t>(orig_NtMapViewOfSection.GetStub()));
+
     auto pred =
         &Predicates<decltype(&::NtMapViewOfSection)>::Ignore<((NTSTATUS)0)>;
 
-    if (!CheckHook(orig_NtMapViewOfSection, "ntdll.dll", "NtMapViewOfSection",
+    if (!CheckHook(pNtMapViewOfSection, "ntdll.dll", "NtMapViewOfSection",
                    pred)) {
       // CheckHook has already printed the error message for us
       return false;
     }
   }
 
   // Now ensure that our hook cleanup worked
-  NTSTATUS status =
-      pNtMapViewOfSection(nullptr, nullptr, nullptr, 0, 0, nullptr, nullptr,
-                          ((SECTION_INHERIT)0), 0, 0);
-  if (NT_SUCCESS(status)) {
+  if (ShouldTestUnhookFunction) {
+    NTSTATUS status =
+        pNtMapViewOfSection(nullptr, nullptr, nullptr, 0, 0, nullptr, nullptr,
+                            ((SECTION_INHERIT)0), 0, 0);
+    if (NT_SUCCESS(status)) {
+      printf(
+          "TEST-FAILED | WindowsDllInterceptor | "
+          "Unexpected successful call to ntdll!NtMapViewOfSection after "
+          "removing 10-byte hook\n");
+      return false;
+    }
+
     printf(
-        "TEST-FAILED | WindowsDllInterceptor | "
-        "Unexpected successful call to ntdll!NtMapViewOfSection after removing "
-        "10-byte hook\n");
-    return false;
+        "TEST-PASS | WindowsDllInterceptor | "
+        "Successfully unhooked ntdll!NtMapViewOfSection via 10-byte patch\n");
   }
 
-  printf(
-      "TEST-PASS | WindowsDllInterceptor | "
-      "Successfully unhooked ntdll!NtMapViewOfSection via 10-byte patch\n");
   return true;
 #else
   return true;
 #endif
 }
 
 extern "C" int wmain(int argc, wchar_t* argv[]) {
   LARGE_INTEGER start;
@@ -577,24 +705,26 @@ extern "C" int wmain(int argc, wchar_t* 
     }
   }
 
   patched_func_called = false;
   ZeroMemory(&p1, sizeof(p1));
 
   p1 = rotatePayload(initial);
 
-  if (!patched_func_called) {
+  if (ShouldTestUnhookFunction != patched_func_called) {
     printf(
-        "TEST-PASS | WindowsDllInterceptor | Hook was not called after "
-        "unregistration\n");
+        "TEST-PASS | WindowsDllInterceptor | Hook was %scalled after "
+        "unregistration\n",
+        ShouldTestUnhookFunction ? "not " : "");
   } else {
     printf(
-        "TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Hook was still called "
-        "after unregistration\n");
+        "TEST-UNEXPECTED-FAIL | WindowsDllInterceptor | Hook was %scalled "
+        "after unregistration\n",
+        ShouldTestUnhookFunction ? "" : "not ");
     return 1;
   }
 
   if (p0 == p1) {
     printf(
         "TEST-PASS | WindowsDllInterceptor | Original function worked "
         "properly\n");
   } else {