[ASan] Do not instrument catch block parameters on Windows #159618
Conversation
llvmbot
added
compiler-rt
compiler-rt:asan
|
@llvm/pr-subscribers-llvm-transforms Author: David Justo (davidmrdavid) ChangesMitigation for: google/sanitizers#749 Disclosure: I'm not an ASan compiler expert yet (I'm trying to learn!), I primarily work in the runtime. Some of this PR was developed with the help of AI tools (primarily as a "fuzzy All text in the PR and in this description is written by me. Context: The msvc ASan team (👋 ) has received an internal request to improve clang's exception handling under ASan for Windows. Namely, we're interested in mitigating this bug: google/sanitizers#749 To summarize, today, clang + ASan produces a false-positive error for this program: #include <cstdio>
#include <exception>
int main()
{
try {
throw std::exception("test");
}catch (const std::exception& ex){
puts(ex.what());
}
return 0;
}The error reads as such: The root of the issue appears to be that ASan's instrumentation is incompatible with Window's assumptions for instantiating The nitty gritty details are lost on me, but I understand that to make this work without loss of ASan coverage, a "serious" refactoring is needed. In the meantime, users risk false positive errors when pairing ASan + catch-block parameters on Windows. To mitigate this I think we should avoid instrumenting catch-block parameters on Windows. It appears to me this is as "simple" as marking catch block parameters as "uninteresting" in I believe this is strictly better than today's status quo, where the runtime generates false positives. Although we're now explicitly choosing to instrument less, the benefit is that now more programs can run with ASan without funky macros that disable ASan on exception blocks. This PR: implements the mitigation above, and creates a simple new test for it. Thanks! Full diff: https://github.com/llvm/llvm-project/pull/159618.diff 2 Files Affected:
diff --git a/compiler-rt/test/asan/TestCases/Windows/basic_exception_handling.cpp b/compiler-rt/test/asan/TestCases/Windows/basic_exception_handling.cpp
new file mode 100644
index 0000000000000..94ca4b9bf2df0
--- /dev/null
+++ b/compiler-rt/test/asan/TestCases/Windows/basic_exception_handling.cpp
@@ -0,0 +1,36 @@
+// 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;
+}
\ No newline at end of file
diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index 42c3d4a4f4c46..986d3c2861af0 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -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;
|
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: David Justo (davidmrdavid) ChangesMitigation for: google/sanitizers#749 Disclosure: I'm not an ASan compiler expert yet (I'm trying to learn!), I primarily work in the runtime. Some of this PR was developed with the help of AI tools (primarily as a "fuzzy All text in the PR and in this description is written by me. Context: The msvc ASan team (👋 ) has received an internal request to improve clang's exception handling under ASan for Windows. Namely, we're interested in mitigating this bug: google/sanitizers#749 To summarize, today, clang + ASan produces a false-positive error for this program: #include <cstdio>
#include <exception>
int main()
{
try {
throw std::exception("test");
}catch (const std::exception& ex){
puts(ex.what());
}
return 0;
}The error reads as such: The root of the issue appears to be that ASan's instrumentation is incompatible with Window's assumptions for instantiating The nitty gritty details are lost on me, but I understand that to make this work without loss of ASan coverage, a "serious" refactoring is needed. In the meantime, users risk false positive errors when pairing ASan + catch-block parameters on Windows. To mitigate this I think we should avoid instrumenting catch-block parameters on Windows. It appears to me this is as "simple" as marking catch block parameters as "uninteresting" in I believe this is strictly better than today's status quo, where the runtime generates false positives. Although we're now explicitly choosing to instrument less, the benefit is that now more programs can run with ASan without funky macros that disable ASan on exception blocks. This PR: implements the mitigation above, and creates a simple new test for it. Thanks! Full diff: https://github.com/llvm/llvm-project/pull/159618.diff 2 Files Affected:
diff --git a/compiler-rt/test/asan/TestCases/Windows/basic_exception_handling.cpp b/compiler-rt/test/asan/TestCases/Windows/basic_exception_handling.cpp
new file mode 100644
index 0000000000000..94ca4b9bf2df0
--- /dev/null
+++ b/compiler-rt/test/asan/TestCases/Windows/basic_exception_handling.cpp
@@ -0,0 +1,36 @@
+// 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;
+}
\ No newline at end of file
diff --git a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
index 42c3d4a4f4c46..986d3c2861af0 100644
--- a/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
+++ b/llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
@@ -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;
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…exception_handling.cpp`
|
FYI @rnk - since you contributed to the discussion of the bug I'm trying to mitigate. Would appreciate your thoughts on this PR. Thanks! |
| // 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))); |
There was a problem hiding this comment.
Please do a simple up-front pass over all basic blocks looking for catchpads and build up a set of catchpad parameters, and make this O(1) by testing for set membership.
Allocas may have a very high number of uses, so this seems worth optimizing.
There was a problem hiding this comment.
I notice above that isAllocaPromotable is also O(#uses), and that seems bad, honestly :(
…ss over function basic blocks
| // Mitigation for https://github.com/google/sanitizers/issues/749 | ||
| // We don't instrument Windows catch-block parameters to avoid | ||
| // interfering with exception handling assumptions. | ||
| void AddressSanitizer::markCatchParametersAsUninteresting(Function &F) { |
There was a problem hiding this comment.
Following up on Reid's suggestion, I think your previous approach of checking whether the alloca is interesting looks correct to me. We should just avoid walking alloca's uses, and instead look for catchpads, whose arguments may be allocas.
We may want to maintain an additional CatchPadParamsAlloca set of such allocas, populate it once during initialization, then the extra check in isInterestingAlloca should be something like:
!(TargetTriple.isOSWindows() && CatchPadParamsAlloca.count(&AI)));There was a problem hiding this comment.
It's using the same approach as markEscapedAllocas, so I understand why it was done this way. Both approaches have an ordering dependency (up-front analysis vs after-the-fact removal), and this one uses one fewer map, so I think it's good, at least good enough to approve merging the PR rather than asking for another round of review.
There was a problem hiding this comment.
Arguably, having all checks grouped in isAllocaInteresting may look slightly more elegant, but agree this is fine too for approval.
There was a problem hiding this comment.
Thanks for the discussion here - it answers my own questions.
I agree that it would be more elegant to centralize the "opting out" of instrumentation in a place like isInterestingAlloca.
I'll keep that in mind as a future improvement. Assuming there's agreement that the current O(n) approach is fine enough, then I'll leave this as-is. Please let me know if not. Thanks!
| @@ -0,0 +1,33 @@ | |||
| // RUN: %clangxx_asan %s -o %t | |||
There was a problem hiding this comment.
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.
+1, this should have an IR instrumentation test using opt in addition to this integration test.
There was a problem hiding this comment.
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.
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 a catchpad, and using that as the input. The property we're looking for is that the alloca used by the catchpad remains in the IR.
There was a problem hiding this comment.
Apologies for the delay here, I've been a bit stuck trying to validate that my .lls shows the right thing.
I'm following up here with the details: #159618 (comment).
Could use assistance, thanks a lot!
|
Following up to this request: #159618 (comment) I could use a bit of assistance, I think I'm doing something wrong. So, I tried producing the I suspect I'm doing something wrong when generating these For debugging, this is the class MyClass {};
#include <exception>
#include <cstdio>
int main() {
try {
throw 1;
} catch (const int ex) {
printf("%d\n", ex);
return -1;
}
return 0;
}(please ignore the dead code of To obtain the To obtain the Below, I'm attaching my This doesn't seem right, does it? Am I doing something obviously wrong? Thanks! |
|
@davidmrdavid Shot in the dark: is opt.exe out of date? At least on Linux, it's common for clang to be rebuilt without building opt (or vice-versa), and then they can become out of sync. For example:
|
|
@thurstond - thanks for the tip, but I've been fully deleting my build directory between builds ( If it helps, this is how I'm building llvm: :: Create and enter build directory (`build.runtimes`)
mkdir %LLVM_ROOT%\build.runtimes
cd %LLVM_ROOT%\build.runtimes
:: Configure the build
cmake -GNinja -DCMAKE_BUILD_TYPE=Debug -DLLVM_ENABLE_PROJECTS=clang -DLLVM_ENABLE_RUNTIMES=compiler-rt ..\llvm\
:: Build LLVM
ninjaSomething weird is that, when running my local clang, I do see my own ad-hoc debug messages in I think I'm going to have to dive deeper, and try to attach a debugger on |
|
edit: please ignore, I noticed it's already in your .ll davidmrdavid@ Ah, try adding this to the top of your .ll file: (or similar; I stole this snippet from opt is designed to be portable so it doesn't know or care that you're running on Windows, unless you tell it to act as if it is Windows. |
|
@davidmrdavid Third-time lucky? :-) |
Mitigation for: google/sanitizers#749
Disclosure: I'm not an ASan compiler expert yet (I'm trying to learn!), I primarily work in the runtime. Some of this PR was developed with the help of AI tools (primarily as a "fuzzy
grepengine"), but I've manually refined and tested the output, and can speak for every line. In general, I used it only to orient myself and for "rubberducking".Context:
The msvc ASan team (👋 ) has received an internal request to improve clang's exception handling under ASan for Windows. Namely, we're interested in mitigating this bug: google/sanitizers#749
To summarize, today, clang + ASan produces a false-positive error for this program:
The error reads as such:
The root of the issue appears to be that ASan's instrumentation is incompatible with Window's assumptions for instantiating
catch-block's parameters (exin the snippet above).The nitty gritty details are lost on me, but I understand that to make this work without loss of ASan coverage, a "serious" refactoring is needed. In the meantime, users risk false positive errors when pairing ASan + catch-block parameters on Windows.
To mitigate this I think we should avoid instrumenting catch-block parameters on Windows. It appears to me this is as "simple" as marking catch block parameters as "uninteresting" in
AddressSanitizer::isInterestingAlloca. My manual tests seem to confirm this.I believe this is strictly better than today's status quo, where the runtime generates false positives. Although we're now explicitly choosing to instrument less, the benefit is that now more programs can run with ASan without funky macros that disable ASan on exception blocks.
This PR: implements the mitigation above, and creates a simple new test for it.
Thanks!