[compiler-rt] 7b5571f - [compiler-rt][interception][win] Don't crash on unknown instructions

[Alvin Wong via llvm-commits]

Author: Alvin Wong
Date: 2023-05-04T22:41:26+08:00
New Revision: 7b5571f3fc79ed77d9868258ba1047b26dc63dfe

URL: https://github.com/llvm/llvm-project/commit/7b5571f3fc79ed77d9868258ba1047b26dc63dfe
DIFF: https://github.com/llvm/llvm-project/commit/7b5571f3fc79ed77d9868258ba1047b26dc63dfe.diff

LOG: [compiler-rt][interception][win] Don't crash on unknown instructions

Do not treat unknown instructions as a fatal error. In most cases,
failure to intercept a function is reported by the caller, though
requires setting verbosity to 1 or higher to be visible.

Better error message reporting for asan will be added in a separate
patch.

Differential Revision: https://reviews.llvm.org/D149549

Added: 
    

Modified: 
    compiler-rt/lib/interception/interception_win.cpp
    compiler-rt/lib/interception/tests/interception_win_test.cpp

Removed: 
    


################################################################################
diff  --git a/compiler-rt/lib/interception/interception_win.cpp b/compiler-rt/lib/interception/interception_win.cpp
index ed840d90e6fea..aa413ee3fcb60 100644
--- a/compiler-rt/lib/interception/interception_win.cpp
+++ b/compiler-rt/lib/interception/interception_win.cpp
@@ -143,6 +143,8 @@ static const int kDirectBranchLength = kBranchLength + kAddressLength;
 
 static void InterceptionFailed() {
   // Do we have a good way to abort with an error message here?
+  // This acts like an abort when no debugger is attached. According to an old
+  // comment, calling abort() leads to an infinite recursion in CheckFailed.
   __debugbreak();
 }
 
@@ -658,9 +660,9 @@ static size_t GetInstructionSize(uptr address, size_t* rel_offset = nullptr) {
   // Unknown instruction!
   // FIXME: Unknown instruction failures might happen when we add a new
   // interceptor or a new compiler version. In either case, they should result
-  // in visible and readable error messages. However, merely calling abort()
-  // leads to an infinite recursion in CheckFailed.
-  InterceptionFailed();
+  // in visible and readable error messages.
+  if (::IsDebuggerPresent())
+    __debugbreak();
   return 0;
 }
 
@@ -681,6 +683,8 @@ static bool CopyInstructions(uptr to, uptr from, size_t size) {
   while (cursor != size) {
     size_t rel_offset = 0;
     size_t instruction_size = GetInstructionSize(from + cursor, &rel_offset);
+    if (!instruction_size)
+      return false;
     _memcpy((void*)(to + cursor), (void*)(from + cursor),
             (size_t)instruction_size);
     if (rel_offset) {

diff  --git a/compiler-rt/lib/interception/tests/interception_win_test.cpp b/compiler-rt/lib/interception/tests/interception_win_test.cpp
index 7d8866a48031b..34283dd0dc362 100644
--- a/compiler-rt/lib/interception/tests/interception_win_test.cpp
+++ b/compiler-rt/lib/interception/tests/interception_win_test.cpp
@@ -307,6 +307,13 @@ const u8 kPatchableCode14[] = {
     0x56,                           // push    esi
 };
 
+const u8 kUnsupportedCode1[] = {
+    0x0f, 0x0b,                     // ud2
+    0x0f, 0x0b,                     // ud2
+    0x0f, 0x0b,                     // ud2
+    0x0f, 0x0b,                     // ud2
+};
+
 // A buffer holding the dynamically generated code under test.
 u8* ActiveCode;
 const size_t ActiveCodeLength = 4096;
@@ -717,6 +724,13 @@ TEST(Interception, PatchableFunctionWithTrampoline) {
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode6, override, prefix));
 }
 
+TEST(Interception, UnsupportedInstructionWithTrampoline) {
+  TestOverrideFunction override = OverrideFunctionWithTrampoline;
+  FunctionPrefixKind prefix = FunctionPrefixPadding;
+
+  EXPECT_FALSE(TestFunctionPatching(kUnsupportedCode1, override, prefix));
+}
+
 TEST(Interception, PatchableFunctionPadding) {
   TestOverrideFunction override = OverrideFunction;
   FunctionPrefixKind prefix = FunctionPrefixPadding;