This is an archive of the discontinued LLVM Phabricator instance.

Differential D87105

[sanitizers][Windows] Better debug output and continuable option for interception failures on Windows
Needs RevisionPublic

Authored by mcgov on Sep 3 2020, 11:43 AM.

Details

Reviewers
rnk
vitalybuka
mstorsjo
Summary

Currently interception failures just debugbreak, this is confusing to users since the failure can manifest in unexpected ways when a debugger is not attached. When a debugger is attached, there is no good error message explaining what has gone wrong.

This patch fixes this issue and outputs an clear error to the debugger, as well as provides an option for continuing past the interception failure if the user doesn't need the function that has failed to intercept.

Diff Detail

Event Timeline

mcgov requested review of this revision.Sep 3 2020, 11:43 AM
mcgov planned changes to this revision.
mcgov created this revision.
mcgov updated this revision to Diff 289776.Sep 3 2020, 11:55 AM

fixed, break should occur if the env variable is not set OR if there was an error finding the variable, so anytime the return code is 0.

mcgov updated this revision to Diff 289779.Sep 3 2020, 11:59 AM

removed whitespace diff

vitalybuka added inline comments.Sep 4 2020, 5:20 AM
b\compiler-rt\lib\interception\interception_win.cpp
149

can we use sanitizer_common flags?

mcgov added inline comments.Sep 4 2020, 10:48 AM
b\compiler-rt\lib\interception\interception_win.cpp
149

This is a good point, I was trying to only use things I was sure was initialized in the winapi but using an asan_option is cleaner.

One hiccup, adding this header to interception_win... There's something additional needed for the common_flag symbol to not be undefined in asan_dll_thunk. Do I need to add something to resolve this at runtime in that thunk library? Any tips here are appreciated

vitalybuka added inline comments.Sep 6 2020, 9:08 PM
b\compiler-rt\lib\interception\interception_win.cpp
149

Not sure how this works on windows, but in general we init interceptors after common_flags()

vitalybuka added inline comments.Sep 16 2020, 1:53 PM
b\compiler-rt\lib\interception\interception_win.cpp
149

This stuff is common for all sanitizers so ASAN_ is not good prefix.

149

I think you can add

static bool break_on_interception_failure;
void __interception::SetDebugBreakOnIntercepionFailure(bool v) {
   break_on_interception_failure = v;
}

And then in asan,msan... get it from flags() and forward it here

164

On linux we print and move forward. Usually failed interceptor is no a problem as program may not even call it.
Why it needs to be DebugBreake in a first place?

mcgov added inline comments.Sep 24 2020, 9:07 AM
b\compiler-rt\lib\interception\interception_win.cpp
149

I think the issue isn't the init order, just adding the common_flags symbol to a library that previously didn't use it.

149

This is great, thank you! I'll give it a shot.

164

It doesn't really need to be one and I like the idea of just printing something and moving forward. The current failure mode is to crash, which prompts the user to investigate in a debugger to get this output spew. Just printing out a warning would be better, I'll look into it before testing that other change above.

rnk added a comment.Oct 29 2020, 4:11 PM

I'm in favor of the direction of the change, too, fwiw.

vitalybuka requested changes to this revision.Feb 17 2021, 1:56 PM
This revision now requires changes to proceed.Feb 17 2021, 1:56 PM

Revision Contents

PathSize
b\compiler-rt\lib\interception\interception_win.cpp
a\compiler-rt\lib\interception\interception_win.cpp
27 lines

Diff 289774

b\compiler-rt\lib\interception\interception_win.cpp

Context not available.
// //
// [trampoline] // [trampoline]
// addr1: .bytes <hook> // addr1: .bytes <hook>
// real: <instr> // real: <instr>
// <instr> // <instr>
// jmp QWORD [addr2] // jmp QWORD [addr2]
// addr2: .bytes <body> // addr2: .bytes <body>
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#include "interception.h" #include "interception.h"
#if SANITIZER_WINDOWS #if SANITIZER_WINDOWS
#include "sanitizer_common/sanitizer_platform.h" #include "sanitizer_common/sanitizer_platform.h"
#define WIN32_LEAN_AND_MEAN #define WIN32_LEAN_AND_MEAN
#include <windows.h> #include <windows.h>
namespace __interception {
namespace __interception {
static const int kAddressLength = FIRST_32_SECOND_64(4, 8); static const int kAddressLength = FIRST_32_SECOND_64(4, 8);
static const int kJumpInstructionLength = 5; static const int kJumpInstructionLength = 5;
static const int kShortJumpInstructionLength = 2; static const int kShortJumpInstructionLength = 2;
static const int kIndirectJumpInstructionLength = 6; static const int kIndirectJumpInstructionLength = 6;
static const int kBranchLength = static const int kBranchLength =
FIRST_32_SECOND_64(kJumpInstructionLength, kIndirectJumpInstructionLength); FIRST_32_SECOND_64(kJumpInstructionLength, kIndirectJumpInstructionLength);
static const int kDirectBranchLength = kBranchLength + kAddressLength; static const int kDirectBranchLength = kBranchLength + kAddressLength;
static void InterceptionFailed() { static void InterceptionFailed() {
// Do we have a good way to abort with an error message here? /* If the user wants to continue past an interception failure, they can set
__debugbreak(); * this variable. If not, these failures should be fatal, prompting them to
* open a debugger. */
char dummy_buffer[1] = {0};
bool break_on_interception_failure =
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

can we use sanitizer_common flags?

vitalybuka: can we use sanitizer_common flags?
mcgovAuthorUnsubmitted
Not Done
ReplyInline Actions

This is a good point, I was trying to only use things I was sure was initialized in the winapi but using an asan_option is cleaner.

One hiccup, adding this header to interception_win... There's something additional needed for the common_flag symbol to not be undefined in asan_dll_thunk. Do I need to add something to resolve this at runtime in that thunk library? Any tips here are appreciated

mcgov: This is a good point, I was trying to only use things I was sure was initialized in the winapi…
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

Not sure how this works on windows, but in general we init interceptors after common_flags()

vitalybuka: Not sure how this works on windows, but in general we init interceptors after common_flags()
mcgovAuthorUnsubmitted
Done
ReplyInline Actions

I think the issue isn't the init order, just adding the common_flags symbol to a library that previously didn't use it.

mcgov: I think the issue isn't the init order, just adding the common_flags symbol to a library that…
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

I think you can add

static bool break_on_interception_failure;
void __interception::SetDebugBreakOnIntercepionFailure(bool v) {
   break_on_interception_failure = v;
}

And then in asan,msan... get it from flags() and forward it here

vitalybuka: I think you can add ``` static bool break_on_interception_failure; void __interception…
mcgovAuthorUnsubmitted
Done
ReplyInline Actions

This is great, thank you! I'll give it a shot.

mcgov: This is great, thank you! I'll give it a shot.
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

This stuff is common for all sanitizers so ASAN_ is not good prefix.

vitalybuka: This stuff is common for all sanitizers so ASAN_ is not good prefix.
GetEnvironmentVariableA("ASAN_WIN_CONTINUE_ON_INTERCEPTION_FAILURE",
dummy_buffer, 0) == 0 &&
GetLastError() == ERROR_ENVVAR_NOT_FOUND;
bool debugger_is_present = IsDebuggerPresent();
if (debugger_is_present) {
OutputDebugStringA(
"====================================================================\n"
"Warning!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n"
"AddressSanitizer (ASan) interception failed for a targeted function.\n"
"Please file a bug report which includes your Windows build number.\n"
"This is listed as 'OS Version' in the command output from \n"
"systeminfo.\n"
"====================================================================\n"
);
}
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

On linux we print and move forward. Usually failed interceptor is no a problem as program may not even call it.
Why it needs to be DebugBreake in a first place?

vitalybuka: On linux we print and move forward. Usually failed interceptor is no a problem as program may…
mcgovAuthorUnsubmitted
Done
ReplyInline Actions

It doesn't really need to be one and I like the idea of just printing something and moving forward. The current failure mode is to crash, which prompts the user to investigate in a debugger to get this output spew. Just printing out a warning would be better, I'll look into it before testing that other change above.

mcgov: It doesn't really need to be one and I like the idea of just printing something and moving…
if (break_on_interception_failure || debugger_is_present) {
DebugBreak();
}
} }
static bool DistanceIsWithin2Gig(uptr from, uptr target) { static bool DistanceIsWithin2Gig(uptr from, uptr target) {
#if SANITIZER_WINDOWS64 #if SANITIZER_WINDOWS64
if (from < target) if (from < target)
return target - from <= (uptr)0x7FFFFFFFU; return target - from <= (uptr)0x7FFFFFFFU;
else else
return from - target <= (uptr)0x80000000U; return from - target <= (uptr)0x80000000U;
#else #else
// In a 32-bit address space, the address calculation will wrap, so this check // In a 32-bit address space, the address calculation will wrap, so this check
// is unnecessary. // is unnecessary.
return true; return true;
#endif #endif
} }
static uptr GetMmapGranularity() { static uptr GetMmapGranularity() {
Context not available.