[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 ↗

(On Diff #516092)

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 ↗

(On Diff #516092)

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

157–161

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

157–161

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

157–161

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

157–161

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

157–161

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

687–688

we use lowercase for hex in sanitizers

[alvinhochun]

compiler-rt/lib/interception/interception_win.cpp

157–161

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.

687–688

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

[vitalybuka]

compiler-rt/lib/asan/asan_win.cpp

162

Maybe VReport?

compiler-rt/lib/interception/interception_win.cpp

157–161

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.

687–688

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

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/asan/asan_interceptors.cpp

 #endif
 
 // ---------------------- InitializeAsanInterceptors ---------------- {{{1
 namespace __asan {
 void InitializeAsanInterceptors() {
   static bool was_called_once;
   CHECK(!was_called_once);
   was_called_once = true;
+  InitializePlatformInterceptors();
   InitializeCommonInterceptors();
   InitializeSignalInterceptors();
 
   // Intercept str* functions.
   ASAN_INTERCEPT_FUNC(strcat);
   ASAN_INTERCEPT_FUNC(strcpy);
   ASAN_INTERCEPT_FUNC(strncat);
   ASAN_INTERCEPT_FUNC(strncpy);

 #if ASAN_INTERCEPT_PTHREAD_ATFORK
   ASAN_INTERCEPT_FUNC(pthread_atfork);
 #endif
 
 #if ASAN_INTERCEPT_VFORK
   ASAN_INTERCEPT_FUNC(vfork);
 #endif
 
-  InitializePlatformInterceptors();
-
   VReport(1, "AddressSanitizer: libc interceptors initialized\n");
 }
 
 } // namespace __asan
 
 #endif  // !SANITIZER_FUCHSIA

compiler-rt/lib/asan/asan_win.cpp

return REAL(CreateThread)(security, stack_size, asan_thread_start, t,
                             thr_flags, tid);
 }
 
 // }}}
 
 namespace __asan {
 
 void InitializePlatformInterceptors() {
+  __interception::SetErrorReportCallback(Report);
+
   // The interceptors were not designed to be removable, so we have to keep this
   // module alive for the life of the process.
   HMODULE pinned;
   CHECK(GetModuleHandleExW(
       GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS | GET_MODULE_HANDLE_EX_FLAG_PIN,
       (LPCWSTR)&InitializePlatformInterceptors, &pinned));
 
   ASAN_INTERCEPT_FUNC(CreateThread);

compiler-rt/lib/interception/interception_win.h

 // Overrides a function only when it is called from a specific DLL. For example,
 // this is used to override calls to HeapAlloc/HeapFree from ucrtbase without
 // affecting other third party libraries.
 bool OverrideImportedFunction(const char *module_to_patch,
                               const char *imported_module,
                               const char *function_name, uptr new_function,
                               uptr *orig_old_func);
 
+// Sets a callback to be used for reporting errors by interception_win. The
+// callback will be called with printf-like arguments. Intended to be used with
+// __sanitizer::Report. Pass nullptr to disable error reporting (default).
+void SetErrorReportCallback(void (*callback)(const char *format, ...));
+
 #if !SANITIZER_WINDOWS64
 // Exposed for unittests
 bool OverrideFunctionWithDetour(
     uptr old_func, uptr new_func, uptr *orig_old_func);
 #endif
 
 // Exposed for unittests
 bool OverrideFunctionWithRedirectJump(

compiler-rt/lib/interception/interception_win.cpp

 //                                              <instr>
 //                                              jmp QWORD [addr2]
 //                                      addr2:  .bytes <body>
 //===----------------------------------------------------------------------===//
 
 #include "interception.h"
 
 #if SANITIZER_WINDOWS
 #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;
 
+#  if defined(_MSC_VER)
+#    define INTERCEPTION_FORMAT(f, a)
+#  else
+#    define INTERCEPTION_FORMAT(f, a) __attribute__((format(printf, f, a)))
+#  endif
+
+static void (*ErrorReportCallback)(const char *format, ...)
+    INTERCEPTION_FORMAT(1, 2);
+
+void SetErrorReportCallback(void (*callback)(const char *format, ...)) {
+  ErrorReportCallback = callback;
+}
+
+#  define ReportError(...)                \
+    do {                                  \
+      if (ErrorReportCallback)            \
+        ErrorReportCallback(__VA_ARGS__); \
+    } while (0)
+
 static void InterceptionFailed() {
-  // Do we have a good way to abort with an error message here?
+  ReportError("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)
#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.
+  // 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.
+  u8 *bytes = (u8 *)address;
+  ReportError(
+      "interception_win: unhandled instruction at %p: %02X %02X %02X %02X %02X "
+      "%02X %02X %02X\n",
+      (void *)address, bytes[0], bytes[1], bytes[2], bytes[3], bytes[4],
+      bytes[5], bytes[6], bytes[7]);
   if (::IsDebuggerPresent())
     __debugbreak();
   return 0;
 }
 
 // Returns 0 on error.
 static size_t RoundUpToInstrBoundary(size_t size, uptr address) {
   size_t cursor = 0;

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 <stdarg.h>
+
 #define WIN32_LEAN_AND_MEAN
 #include <windows.h>
 
 namespace __interception {
 namespace {
 
 enum FunctionPrefixKind {
   FunctionPrefixNone,
#endif
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode5, override, prefix));
   EXPECT_FALSE(TestFunctionPatching(kUnpatchableCode6, override, prefix));
 }
 
 TEST(Interception, UnsupportedInstructionWithTrampoline) {
   TestOverrideFunction override = OverrideFunctionWithTrampoline;
   FunctionPrefixKind prefix = FunctionPrefixPadding;
 
+  static bool reportCalled;
+  reportCalled = false;
+
+  struct Local {
+    static void Report(const char *format, ...) {
+      if (reportCalled)
+        FAIL() << "Report called more times than expected";
+      reportCalled = true;
+      ASSERT_STREQ(
+          "interception_win: unhandled instruction at %p: %02X %02X %02X %02X "
+          "%02X %02X %02X %02X\n",
+          format);
+      va_list args;
+      va_start(args, format);
+      u8 *ptr = va_arg(args, u8 *);
+      for (int i = 0; i < 8; i++) EXPECT_EQ(kUnsupportedCode1[i], ptr[i]);
+      int bytes[8];
+      for (int i = 0; i < 8; i++) {
+        bytes[i] = va_arg(args, int);
+        EXPECT_EQ(kUnsupportedCode1[i], bytes[i]);
+      }
+      va_end(args);
+    }
+  };
+
+  SetErrorReportCallback(Local::Report);
   EXPECT_FALSE(TestFunctionPatching(kUnsupportedCode1, override, prefix));
+  SetErrorReportCallback(nullptr);
+
+  if (!reportCalled)
+    ADD_FAILURE() << "Report not called";
 }
 
 TEST(Interception, PatchableFunctionPadding) {
   TestOverrideFunction override = OverrideFunction;
   FunctionPrefixKind prefix = FunctionPrefixPadding;
 
   EXPECT_TRUE(TestFunctionPatching(kPatchableCode1, override, prefix));
   EXPECT_TRUE(TestFunctionPatching(kPatchableCode2, override, prefix));