-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[ASan] Do not instrument catch block parameters on Windows #159618
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
ece2a2b
bff409d
043848d
2326be0
3497540
e4551b1
9b99935
04956f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
// RUN: %clangxx_asan %s -o %t | ||
// RUN: %run %t | FileCheck %s | ||
|
||
// This test tests that declaring a parameter in a catch-block does not produce a false positive | ||
// ASan error on Windows. | ||
|
||
// This code is based on the repro in https://github.com/google/sanitizers/issues/749 | ||
#include <cstdio> | ||
#include <exception> | ||
|
||
void throwInFunction() { throw std::exception("test2"); } | ||
|
||
int main() { | ||
// case 1: direct throw | ||
try { | ||
throw std::exception("test1"); | ||
} catch (const std::exception &ex) { | ||
puts(ex.what()); | ||
// CHECK: test1 | ||
} | ||
|
||
// case 2: throw in function | ||
try { | ||
throwInFunction(); | ||
} catch (const std::exception &ex) { | ||
puts(ex.what()); | ||
// CHECK: test2 | ||
} | ||
|
||
printf("Success!\n"); | ||
// CHECK: Success! | ||
return 0; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1397,6 +1397,16 @@ void AddressSanitizer::instrumentMemIntrinsic(MemIntrinsic *MI, | |
MI->eraseFromParent(); | ||
} | ||
|
||
// Check if an alloca is a catch block parameter | ||
static bool isCatchParameter(const AllocaInst &AI) { | ||
for (const Use &U : AI.uses()) { | ||
if (isa<CatchPadInst>(U.getUser())) { | ||
return true; | ||
} | ||
} | ||
return false; | ||
} | ||
|
||
/// Check if we want (and can) handle this alloca. | ||
bool AddressSanitizer::isInterestingAlloca(const AllocaInst &AI) { | ||
auto [It, Inserted] = ProcessedAllocas.try_emplace(&AI); | ||
|
@@ -1417,7 +1427,11 @@ bool AddressSanitizer::isInterestingAlloca(const AllocaInst &AI) { | |
// swifterror allocas are register promoted by ISel | ||
!AI.isSwiftError() && | ||
// safe allocas are not interesting | ||
!(SSGI && SSGI->isSafe(AI))); | ||
!(SSGI && SSGI->isSafe(AI)) && | ||
// Mitigation for https://github.com/google/sanitizers/issues/749 | ||
// We don't instrument Windows catch-block parameters to avoid | ||
// interfering with exception handling assumptions. | ||
!(TargetTriple.isOSWindows() && isCatchParameter(AI))); | ||
|
||
|
||
It->second = IsInteresting; | ||
return IsInteresting; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have a small lit test instead of the runtime one? Like a simple function with a catchpad instruction, whose argument is an alloca, showing no asan instrumentation is added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, this should have an IR instrumentation test using
opt
in addition to this integration test.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, I'll look to build that, though it'd be my first time constructing something other than an integration test in this codebase. I should be able to figure out it out, I'll reach out if not.
If there's an example IR test case that I could use for reference, please send it my way. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The localescape.ll test case is a good example of what an IR-only test case looks like, but you'd want to start by taking the C++ from the execution test, feeding it through Clang, emitting LLVM IR (
-emit-llvm -S
), extracting a single function with acatchpad
, and using that as the input. The property we're looking for is that thealloca
used by thecatchpad
remains in the IR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay here, I've been a bit stuck trying to validate that my
.ll
s shows the right thing.I'm following up here with the details: #159618 (comment).
Could use assistance, thanks a lot!