Skip to content

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

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+55 −0
Open

[ASan] Do not instrument catch block parameters on Windows #159618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ece2a2b
do not asan-instrument catch parameters on windows
davidmrdavid Sep 18, 2025
bff409d
add basic unit test: catches exception 'inline' and in another frame
davidmrdavid Sep 18, 2025
043848d
apply clang-format on `compiler-rt/test/asan/TestCases/Windows/basic_…
davidmrdavid Sep 18, 2025
2326be0
optimization: disable catch-parameter instrumentation via a linear pa…
davidmrdavid Sep 23, 2025
3497540
Merge branch 'main' into dev/dajusto/dont-asanize-catch-params-on-win…
davidmrdavid Sep 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions compiler-rt/test/asan/TestCases/Windows/basic_exception_handling.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// RUN: %clangxx_asan %s -o %t
Copy link
Contributor

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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!

Copy link
Collaborator

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 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.

Copy link
Contributor Author

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 .lls shows the right thing.
I'm following up here with the details: #159618 (comment).

Could use assistance, thanks a lot!

// 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;
}
22 changes: 22 additions & 0 deletions llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,6 +850,7 @@ struct AddressSanitizer {
bool maybeInsertAsanInitAtFunctionEntry(Function &F);
bool maybeInsertDynamicShadowAtFunctionEntry(Function &F);
void markEscapedLocalAllocas(Function &F);
void markCatchParametersAsUninteresting(Function &F);

private:
friend struct FunctionStackPoisoner;
Expand Down Expand Up @@ -2986,6 +2987,24 @@ void AddressSanitizer::markEscapedLocalAllocas(Function &F) {
}
}
}
// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)));

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguably, having all checks grouped in isAllocaInteresting may look slightly more elegant, but agree this is fine too for approval.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

for (BasicBlock &BB : F) {
for (Instruction &I : BB) {
if (auto *CatchPad = dyn_cast<CatchPadInst>(&I)) {
// Mark the parameters to a catch-block as uninteresting to avoid
// instrumenting them
for (Value *Operand : CatchPad->arg_operands()) {
if (auto *AI = dyn_cast<AllocaInst>(Operand)) {
ProcessedAllocas[AI] = false;
}
}
}
}
}
}

bool AddressSanitizer::suppressInstrumentationSiteForDebug(int &Instrumented) {
bool ShouldInstrument =
Expand Down Expand Up @@ -3030,6 +3049,9 @@ bool AddressSanitizer::instrumentFunction(Function &F,
// can be passed to that intrinsic.
markEscapedLocalAllocas(F);

if (TargetTriple.isOSWindows())
markCatchParametersAsUninteresting(F);

// We want to instrument every address only once per basic block (unless there
// are calls between uses).
SmallPtrSet<Value *, 16> TempsToInstrument;
Expand Down