[compiler-rt][interception][asan][win] Improve error reporting

[alvinhochun]

Summary

Add a callback from interception to allow asan on Windows to produce
better error messages. If an unrecoverable error occured when
intercepting functions, print a message before terminating.

Additionally, when encountering unknown instructions, a more helpful
message containing the address and the bytes of the unknown instruction
is now printed to help identify the issue and make it easier to propose
a fix.

Depends on D149549

[vitalybuka]

compiler-rt/lib/interception/interception_win.cpp

130

We want to avoid dependency of interceptors on most of sanitizer_common
For most users these messages is just a spam, they unlikely will rebuild and fix sanitizers.
So it's rather helpful but unnecessary debug leftovers for folks like us.

I prefer we don't land this one and dependent patch.

[alvinhochun]

compiler-rt/lib/interception/interception_win.cpp

130

The behaviour of interception_win when it encounters an unknown instruction, before this patch, is to terminate the process without printing any messages, which is very unhelpful and requires more effort than it should to diagnose. One of the changes in this patch is to make it not terminate the process, but to fail the interception like it would for any other reasons, so that if the functions it fails to intercept are not critical to the operation of asan it can still run with somewhat reduced coverage.

For most users actually these messages should not show up at all. If they show up for any users running on Windows then it means we do have a real problem that needs to be fixed. The point of these messages is, if the users file an issue with the output included it would be much easier for someone to come up with a patch because they can just check the printed instruction bytes, instead of having to debug on the affected systems.

Note that I wrote "running on Windows" -- IIRC asan-instrumented programs currently doesn't run on Wine because the functions have different instructions at the start that interception_win currently doesn't support (@mstorsjo may be able to confirm this). With this patch users can get a better idea what is wrong (or file a better bug report), and we can again fix the issues more easily with the printed bytes. Others may also start running tests on CIs under Wine and be able to report or help fix these issues in case new unsupported instructions comes up.

Also, I would really like this patch to land together with D148991. In that patch we start hooking libc++.dll and libunwind.dll for i686 MinGW. There is a chance Clang may generate different instructions and I would hate for asan to break because of it. We do have nightly checks in llvm-mingw to help catch this, and again the additional output messages can help us fix the issue more easily.

At least that's what I think. The dependency on sanitizer_common is unfortunate but I don't know how it can be avoided. Though it is only limited to Windows, can this be acceptable?

[vitalybuka]

compiler-rt/lib/interception/interception_win.cpp

130

The behavior of interception_win when it encounters an unknown instruction, before this patch, is to terminate the process without printing any messages, which is very unhelpful and requires more effort than it should to diagnose. One of the changes in this patch is to make it not terminate the process, but to fail the interception like it would for any other reasons, so that if the functions it fails to intercept are not critical to the operation of asan it can still run with somewhat reduced coverage.

This part of change LGTM.
BTW would be nice to keep Report and "make non-fatal" in separate patches.

For most users actually these messages should not show up at all. If they show up for any users running on Windows then it means we do have a real problem that needs to be fixed. The point of these messages is, if the users file an issue with the output included it would be much easier for someone to come up with a patch because they can just check the printed instruction bytes, instead of having to debug on the affected systems.

So even less reasons to introduce this dependency?

Note that I wrote "running on Windows" -- IIRC asan-instrumented programs currently doesn't run on Wine because the functions have different instructions at the start that interception_win currently doesn't support (@mstorsjo may be able to confirm this). With this patch users can get a better idea what is wrong (or file a better bug report), and we can again fix the issues more easily with the printed bytes. Others may also start running tests on CIs under Wine and be able to report or help fix these issues in case new unsupported instructions comes up.

We kind of have this functionality in llvm-project/compiler-rt/lib/asan/asan_interceptors.h:140
maybe we can extend to:

VReport(1, "AddressSanitizer: failed to intercept '%s, details: %s'\n", #name, getLastInterceptionError());

Another way to break dependency is to call from asan:

void __interception::SetOnErrorCallback(void(*)(const char*, uptr addr));
``

> 
> Also, I would really like this patch to land together with D148991. In that patch we start hooking `libc++.dll` and `libunwind.dll` for i686 MinGW. There is a chance Clang may generate different instructions and I would hate for asan to break because of it. We do have nightly checks in llvm-mingw to help catch this, and again the additional output messages can help us fix the issue more easily.
> 
> At least that's what I think. The dependency on sanitizer_common is unfortunate but I don't know how it can be avoided. Though it is only limited to Windows, can this be acceptable?

This is windows only, however there is stuff like e.g. safestack which depends on interception but not on common. I don't think we have that on Windows, but you are making this a problem for future committers who decide to support that.

compiler-rt/lib/interception/tests/CMakeLists.txt

109

maybe add gmock on all platforms to have less conditions in CMake?

[alvinhochun]

Split the bit making unknown instructions non-fatal into D149549, and change the way errors are reported to avoid direct dependency from interception to sanitizer_common. Now asan registers a callback in interceptor to call __sanitizer::Report.

[alvinhochun]

compiler-rt/lib/interception/tests/CMakeLists.txt

109

Now the test doesn't require gmock.

[vitalybuka]

compiler-rt/lib/asan/asan_mac.cpp

47 ↗

(On Diff #518305)

Can we instead move InitializePlatformInterceptors() in to the beginning of InitializeAsanInterceptors()?

compiler-rt/lib/interception/interception_win.cpp

159–163

Can we avoid a macro?
static inline should work?

Can you call it ReportError, to avoid confusion for Report in common

[alvinhochun]

compiler-rt/lib/asan/asan_mac.cpp

47 ↗

(On Diff #518305)

Yeah, looks like we can do that.

compiler-rt/lib/interception/interception_win.cpp

159–163

The problem is calling a variadic function, there is no way that I know of to just forward the arguments without using va_list. There is currently not a version of __sanitizer::Report that takes va_list, so I would have to add one, which is more work than just using a macro here.

I can rename the macro to ReportError, yes.

[vitalybuka]

compiler-rt/lib/asan/asan_mac.cpp

47 ↗

(On Diff #518305)

Marked as done, but not done?

compiler-rt/lib/interception/interception_win.cpp

159–163

This should work https://en.cppreference.com/w/cpp/language/parameter_pack

template<class... Ts>
void ReportError(Ts... args) {
      if (ErrorReportCallback)            
        ErrorReportCallback(args...); 
}

[alvinhochun]

compiler-rt/lib/interception/interception_win.cpp

159–163

Ah , didn't know we can do this. However, if I take this approach then we will lose the printf format check warnings provided by __attribute__((format)) (doesn't work when applied to the template function with parameter pack). I think these checks are quite worth having.

[alvinhochun]

Address some comments

[vitalybuka]

compiler-rt/lib/interception/interception_win.cpp

159–163

I don's see why is that? When template instantiated all types are known to call ErrorReportCallback

If it's broken, my preference is to rather abandon format check, as we have just a few calls. but avoid macro.

However, because it's just 2 calls, I don't see a problem
removing ReportError and inlining calls

669–670

we use lowercase for hex in sanitizers

[alvinhochun]

compiler-rt/lib/interception/interception_win.cpp

159–163

Variadic arguments are converted using default argument promotions, but template arguments don't get this promotion which confuses the format checks. There are also a couple of GCC-compat warnings.

warning: GCC does not allow 'format' attribute in this position on a function definition [-Wgcc-compat]
warning: GCC requires a function with the 'format' attribute to be variadic [-Wgcc-compat]
warning: format specifies type 'unsigned int' but the argument has type 'u8' (aka 'unsigned char') [-Wformat]

I'm curious, why should we avoid this macro? VReport and VPrintf in sanitizer_common.h are similar macros, and they make use of Printf and Report which also have the format check attributes. In my opinion the alternatives to having this macro are worse. Do note that the child patch adds two more usages of ReportError.

669–670

Sure, I can change it before committing if you are fine with that.

[vitalybuka]

compiler-rt/lib/asan/asan_win.cpp

162 ↗

(On Diff #518673)

Maybe VReport?

compiler-rt/lib/interception/interception_win.cpp

159–163

Variadic arguments are converted using default argument promotions, but template arguments don't get this promotion which confuses the format checks. There are also a couple of GCC-compat warnings.

warning: GCC does not allow 'format' attribute in this position on a function definition [-Wgcc-compat]
warning: GCC requires a function with the 'format' attribute to be variadic [-Wgcc-compat]
warning: format specifies type 'unsigned int' but the argument has type 'u8' (aka 'unsigned char') [-Wformat]

I'm curious, why should we avoid this macro? VReport and VPrintf in sanitizer_common.h are similar macros, and they make use of Printf and Report which also have the format check attributes. In my opinion the alternatives to having this macro are worse. Do note that the child patch adds two more usages of ReportError.

Reasons are harder to debug, bad stack traces, weird compilation error.
I don't see reasons to have them there as well.

Either way I don't think the issue is important, with or without macro is OK to me.

669–670

Yes, please change.

[vitalybuka]

This revision now requires changes to proceed.

Sorry, just selected wrong item

[alvinhochun]

compiler-rt/lib/asan/asan_win.cpp

162 ↗

(On Diff #518673)

VReport is a macro so not possible. Besides, I think these messages are important enough to not require the verbosity check for now.

compiler-rt/lib/interception/interception_win.cpp

 //                                              <instr>
 //                                              jmp QWORD [addr2]
 //                                      addr2:  .bytes <body>
 //===----------------------------------------------------------------------===//
 
 #include "interception.h"
 
 #if SANITIZER_WINDOWS
+#include "sanitizer_common/sanitizer_common.h"
+#include "sanitizer_common/sanitizer_libc.h"
 #include "sanitizer_common/sanitizer_platform.h"
 #define WIN32_LEAN_AND_MEAN
 #include <windows.h>
 
 namespace __interception {
 
 static const int kAddressLength = FIRST_32_SECOND_64(4, 8);
 static const int kJumpInstructionLength = 5;
 static const int kShortJumpInstructionLength = 2;
 UNUSED static const int kIndirectJumpInstructionLength = 6;
 static const int kBranchLength =
     FIRST_32_SECOND_64(kJumpInstructionLength, kIndirectJumpInstructionLength);
 static const int kDirectBranchLength = kBranchLength + kAddressLength;
 
 static void InterceptionFailed() {
-  // Do we have a good way to abort with an error message here?
+  Report("interception_win: failed due to an unrecoverable error.\n");
+  // 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();
 }
 
 static bool DistanceIsWithin2Gig(uptr from, uptr target) {
 #if SANITIZER_WINDOWS64
   if (from < target)
     return target - from <= (uptr)0x7FFFFFFFU;
   else
     return from - target <= (uptr)0x80000000U;
 #else
   // In a 32-bit address space, the address calculation will wrap, so this check
   // is unnecessary.
   return true;
 #endif
 }
 
 static uptr GetMmapGranularity() {
   SYSTEM_INFO si;
   GetSystemInfo(&si);
   return si.dwAllocationGranularity;
 }
 
#else
   }
 
   switch (*(u32*)address) {
     case 0x2444B60F:  // 0F B6 44 24 XX : movzx eax, byte ptr [esp + XX]
       return 5;
   }
 #endif
 
-  // 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();
+  // Unknown instruction! This might happen when we add a new interceptor, use
+  // a new compiler version, or if Windows changed how some functions are
+  // compiled. In either case, we print the address and 8 bytes of instructions
+  // to notify the user about the error and to help identify the unknown
+  // instruction. Don't treat this as a fatal error, though we can break the
+  // debugger if one has been attached.
+  char instruction[8 * 3 + 1] = {0};
+  for (int i = 0; i < 8; i++) {
+    internal_snprintf(instruction + (i * 3), 4, " %02X", *(u8 *)(address + i));
+  }
+  Report("interception_win: unhandled instruction at %p:%s\n", (void *)address,
+         instruction);
+  if (IsDebuggerPresent())
+    __debugbreak();
   return 0;
 }
 
 // Returns 0 on error.
 static size_t RoundUpToInstrBoundary(size_t size, uptr address) {
   size_t cursor = 0;
   while (cursor < size) {
     size_t instruction_size = GetInstructionSize(address + cursor);
     if (!instruction_size)
       return 0;
     cursor += instruction_size;
   }
   return cursor;
 }
 
 static bool CopyInstructions(uptr to, uptr from, size_t size) {
   size_t cursor = 0;
   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) {
       uptr delta = to - from;
       uptr relocated_offset = *(u32*)(to + cursor + rel_offset) - delta;
 #if SANITIZER_WINDOWS64
       if (relocated_offset + 0x80000000U >= 0xFFFFFFFFU)
         return false;

compiler-rt/lib/interception/tests/CMakeLists.txt

set(INTERCEPTION_TEST_LINK_FLAGS_COMMON
   ${SANITIZER_TEST_CXX_LIBRARIES})
 
 # -gline-tables-only must be enough for these tests, so use it if possible.
 if(COMPILER_RT_TEST_COMPILER_ID MATCHES "Clang")
   list(APPEND INTERCEPTION_TEST_CFLAGS_COMMON -gline-tables-only)
 else()
   list(APPEND INTERCEPTION_TEST_CFLAGS_COMMON -g)
 endif()
+set(INTERCEPTION_TEST_GMOCK_SOURCE "")
+if(WIN32)
+  list(APPEND INTERCEPTION_TEST_CFLAGS_COMMON ${COMPILER_RT_GMOCK_CFLAGS})
+  set(INTERCEPTION_TEST_GMOCK_SOURCE ${COMPILER_RT_GMOCK_SOURCE})
+endif()
 if(MSVC)
   list(APPEND INTERCEPTION_TEST_CFLAGS_COMMON -gcodeview)
   list(APPEND INTERCEPTION_TEST_LINK_FLAGS_COMMON
     -Wl,-largeaddressaware
     -Wl,-nodefaultlib:libcmt,-defaultlib:msvcrt,-defaultlib:oldnames
     )
 endif()
 if(MINGW)

 
 # Adds interception tests for architecture.
 macro(add_interception_tests_for_arch arch)
   set(INTERCEPTION_TEST_OBJECTS)
   get_interception_lib_for_arch(${arch} INTERCEPTION_COMMON_LIB)
   generate_compiler_rt_tests(INTERCEPTION_TEST_OBJECTS
     InterceptionUnitTests "Interception-${arch}-Test" ${arch}
     RUNTIME ${INTERCEPTION_COMMON_LIB}
-    SOURCES ${INTERCEPTION_UNITTESTS} ${COMPILER_RT_GTEST_SOURCE}
+    SOURCES ${INTERCEPTION_UNITTESTS} ${COMPILER_RT_GTEST_SOURCE} ${INTERCEPTION_TEST_GMOCK_SOURCE}
     COMPILE_DEPS ${INTERCEPTION_TEST_HEADERS}
     DEPS llvm_gtest
     CFLAGS ${INTERCEPTION_TEST_CFLAGS_COMMON}
     LINK_FLAGS ${INTERCEPTION_TEST_LINK_FLAGS_COMMON})
 endmacro()
 
 if(COMPILER_RT_CAN_EXECUTE_TESTS AND NOT ANDROID AND NOT APPLE)
   # We use just-built clang to build interception unittests, so we must
   # be sure that produced binaries would work.
   if(APPLE)
     add_interceptor_lib("RTInterception.test.osx"
                         $<TARGET_OBJECTS:RTInterception.osx>)
   else()
     foreach(arch ${INTERCEPTION_UNITTEST_SUPPORTED_ARCH})
       add_interceptor_lib("RTInterception.test.${arch}"
                           $<TARGET_OBJECTS:RTInterception.${arch}>)
+      if(WIN32)
+        target_sources("RTInterception.test.${arch}" PUBLIC
+                       $<TARGET_OBJECTS:RTSanitizerCommon.${arch}>)
+      endif()
     endforeach()
   endif()
   foreach(arch ${INTERCEPTION_UNITTEST_SUPPORTED_ARCH})
     add_interception_tests_for_arch(${arch})
   endforeach()
 endif()

compiler-rt/lib/interception/tests/interception_win_test.cpp

 #include "interception/interception.h"
 
 #include "gtest/gtest.h"
 
 // Too slow for debug build
 #if !SANITIZER_DEBUG
 #if SANITIZER_WINDOWS
 
+#include <string>
+
+#include "gmock/gmock.h"
+#include "sanitizer_common/sanitizer_common.h"
 #define WIN32_LEAN_AND_MEAN
 #include <windows.h>
 
 namespace __interception {
 namespace {
 
 enum FunctionPrefixKind {
   FunctionPrefixNone,

 const u8 kPatchableCode14[] = {
     0x55,                           // push    ebp
     0x89, 0xe5,                     // mov     ebp,esp
     0x53,                           // push    ebx
     0x57,                           // push    edi
     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;
 
 int InterceptorFunction(int x);
 
 /// Allocate code memory more than 2GB away from Base.
 u8 *AllocateCode2GBAway(u8 *Base) {
#endif
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode1, override, prefix));
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode2, override, prefix));
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode3, override, prefix));
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode4, override, prefix));
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode5, override, prefix));
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode6, override, prefix));
 }
 
+TEST(Interception, UnsupportedInstructionWithTrampoline) {
+  using ::testing::MatchesRegex;
+  TestOverrideFunction override = OverrideFunctionWithTrampoline;
+  FunctionPrefixKind prefix = FunctionPrefixPadding;
+
+  static std::string reportString;
+  reportString.clear();
+
+  SetPrintfAndReportCallback(
+      [](const char *message) { reportString.append(message); });
+  EXPECT_FALSE(TestFunctionPatching(kUnsupportedCode1, override, prefix));
+  SetPrintfAndReportCallback(nullptr);
+
+  EXPECT_THAT(reportString.c_str(),
+              MatchesRegex("^==\\d+==interception_win: unhandled instruction "
+                           "at 0x\\w+: 0F 0B 0F 0B 0F 0B 0F 0B\n$"));
+}
+
 TEST(Interception, PatchableFunctionPadding) {
   TestOverrideFunction override = OverrideFunction;
   FunctionPrefixKind prefix = FunctionPrefixPadding;
 
   EXPECT_TRUE(TestFunctionPatching(kPatchableCode1, override, prefix));
   EXPECT_TRUE(TestFunctionPatching(kPatchableCode2, override, prefix));
 #if SANITIZER_WINDOWS64
   EXPECT_FALSE(TestFunctionPatching(kPatchableCode3, override, prefix));