base: Add more diagnostics to COM hook injection.

On 32-bit Windows builds where DCHECK is enabled, ComInitCheckHook
hot-patches CoCreateInstance() to DCHECK that it is called in
appropriate circumstances. The hot-patching method can only be used once
(details in base/win/com_init_check_hook.cc), and Chrome's usage may
conflict with other drivers that may want to hot-patch the same method
(example in https://crbug.com/737090).

This CL adds answers to the following questions that may arise when
investigating hot-patch failures.

1) Is hot-patching failing due to a bug where the HookManager singleton
   is instantiated twice? This can happen if base is linked in twice.
   This question is answered by printing the bytes that would be written
   to the hot-patch area, so they can be compared with the current
   values.
2) Is hot-patching failing due to a previous failure to revert the
   hot-patch? This question is answered by DCHECKing that reverting the
   hot-patch never fails. (The code previously just assumed that.)

Bug: 877868
Change-Id: I94f81b3816417447d31fc700c6b81d8325222d1a
Reviewed-on: https://chromium-review.googlesource.com/1189143
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586375}

base/win/com_init_check_hook.cc

@@ -141,6 +141,12 @@ class HookManager {
         reinterpret_cast<decltype(original_co_create_instance_body_function_)>(
             co_create_instance_padded_address_ + 7);
 
+    uint32_t dchecked_co_create_instance_address =
+        reinterpret_cast<uint32_t>(&HookManager::DCheckedCoCreateInstance);
+    uint32_t jmp_offset_base_address = co_create_instance_padded_address_ + 5;
+    structured_hotpatch_.relative_address =
+        dchecked_co_create_instance_address - jmp_offset_base_address;
+
     HotpatchPlaceholderFormat format = GetHotpatchPlaceholderFormat(
         reinterpret_cast<const void*>(co_create_instance_padded_address_));
     if (format == HotpatchPlaceholderFormat::UNKNOWN) {
@@ -150,40 +156,42 @@ class HookManager {
       return;
     } else if (format == HotpatchPlaceholderFormat::EXTERNALLY_PATCHED) {
       hotpatch_placeholder_format_ = format;
-      NOTREACHED() << "CoCreateInstance appears to be previously patched. ("
+      NOTREACHED() << "CoCreateInstance appears to be previously patched. <"
                    << FirstSevenBytesToString(
                           co_create_instance_padded_address_)
-                   << ")";
+                   << "> Attempted to write <"
+                   << FirstSevenBytesToString(
+                          reinterpret_cast<uint32_t>(&structured_hotpatch_))
+                   << ">";
       return;
     }
 
-    uint32_t dchecked_co_create_instance_address =
-        reinterpret_cast<uint32_t>(&HookManager::DCheckedCoCreateInstance);
-    uint32_t jmp_offset_base_address = co_create_instance_padded_address_ + 5;
-    StructuredHotpatch structured_hotpatch;
-    structured_hotpatch.relative_address =
-        dchecked_co_create_instance_address - jmp_offset_base_address;
-
     DCHECK_EQ(hotpatch_placeholder_format_, HotpatchPlaceholderFormat::UNKNOWN);
     DWORD patch_result = internal::ModifyCode(
         reinterpret_cast<void*>(co_create_instance_padded_address_),
-        reinterpret_cast<void*>(&structured_hotpatch),
-        sizeof(structured_hotpatch));
+        reinterpret_cast<void*>(&structured_hotpatch_),
+        sizeof(structured_hotpatch_));
     if (patch_result == NO_ERROR)
       hotpatch_placeholder_format_ = format;
   }
 
   void RevertHook() {
     lock_.AssertAcquired();
+
+    DWORD revert_result = NO_ERROR;
     switch (hotpatch_placeholder_format_) {
       case HotpatchPlaceholderFormat::INT3:
-        internal::ModifyCode(
+        if (WasHotpatchChanged())
+          return;
+        revert_result = internal::ModifyCode(
             reinterpret_cast<void*>(co_create_instance_padded_address_),
             reinterpret_cast<const void*>(&g_hotpatch_placeholder_int3),
             sizeof(g_hotpatch_placeholder_int3));
         break;
       case HotpatchPlaceholderFormat::NOP:
-        internal::ModifyCode(
+        if (WasHotpatchChanged())
+          return;
+        revert_result = internal::ModifyCode(
             reinterpret_cast<void*>(co_create_instance_padded_address_),
             reinterpret_cast<const void*>(&g_hotpatch_placeholder_nop),
             sizeof(g_hotpatch_placeholder_nop));
@@ -192,6 +200,8 @@ class HookManager {
       case HotpatchPlaceholderFormat::UNKNOWN:
         break;
     }
+    DCHECK_EQ(revert_result, static_cast<DWORD>(NO_ERROR))
+        << "Failed to revert CoCreateInstance hot-patch";
 
     hotpatch_placeholder_format_ = HotpatchPlaceholderFormat::UNKNOWN;
 
@@ -230,6 +240,22 @@ class HookManager {
     return HotpatchPlaceholderFormat::UNKNOWN;
   }
 
+  bool WasHotpatchChanged() {
+    if (::memcmp(reinterpret_cast<void*>(co_create_instance_padded_address_),
+                 reinterpret_cast<const void*>(&structured_hotpatch_),
+                 sizeof(structured_hotpatch_)) == 0) {
+      return false;
+    }
+
+    NOTREACHED() << "CoCreateInstance patch overwritten. Expected: <"
+                 << FirstSevenBytesToString(co_create_instance_padded_address_)
+                 << ">, Actual: <"
+                 << FirstSevenBytesToString(
+                        reinterpret_cast<uint32_t>(&structured_hotpatch_))
+                 << ">";
+    return true;
+  }
+
   static HRESULT __stdcall DCheckedCoCreateInstance(const CLSID& rclsid,
                                                     IUnknown* pUnkOuter,
                                                     DWORD dwClsContext,
@@ -269,6 +295,7 @@ class HookManager {
   uint32_t co_create_instance_padded_address_ = 0;
   HotpatchPlaceholderFormat hotpatch_placeholder_format_ =
       HotpatchPlaceholderFormat::UNKNOWN;
+  StructuredHotpatch structured_hotpatch_;
   static decltype(
       ::CoCreateInstance)* original_co_create_instance_body_function_;
 

base/win/com_init_check_hook_unittest.cc

@@ -130,5 +130,46 @@ TEST(ComInitCheckHook, ExternallyHooked) {
 #endif
 }
 
+TEST(ComInitCheckHook, UnexpectedChangeDuringHook) {
+#if defined(COM_INIT_CHECK_HOOK_ENABLED)
+  HMODULE ole32_library = ::LoadLibrary(L"ole32.dll");
+  ASSERT_TRUE(ole32_library);
+
+  uint32_t co_create_instance_padded_address =
+      reinterpret_cast<uint32_t>(
+          GetProcAddress(ole32_library, "CoCreateInstance")) -
+      5;
+  const unsigned char* co_create_instance_bytes =
+      reinterpret_cast<const unsigned char*>(co_create_instance_padded_address);
+  const unsigned char original_byte = co_create_instance_bytes[0];
+  const unsigned char unexpected_byte = 0xdb;
+  ASSERT_EQ(static_cast<DWORD>(NO_ERROR),
+            internal::ModifyCode(
+                reinterpret_cast<void*>(co_create_instance_padded_address),
+                reinterpret_cast<const void*>(&unexpected_byte),
+                sizeof(unexpected_byte)));
+
+  EXPECT_DCHECK_DEATH({
+    ComInitCheckHook com_check_hook;
+
+    internal::ModifyCode(
+        reinterpret_cast<void*>(co_create_instance_padded_address),
+        reinterpret_cast<const void*>(&unexpected_byte),
+        sizeof(unexpected_byte));
+  });
+
+  // If this call fails, really bad things are going to happen to other tests
+  // so CHECK here.
+  CHECK_EQ(static_cast<DWORD>(NO_ERROR),
+           internal::ModifyCode(
+               reinterpret_cast<void*>(co_create_instance_padded_address),
+               reinterpret_cast<const void*>(&original_byte),
+               sizeof(original_byte)));
+
+  ::FreeLibrary(ole32_library);
+  ole32_library = nullptr;
+#endif
+}
+
 }  // namespace win
 }  // namespace base