Skip to content
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
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.

I think this runtime test can now be dropped? Crashes are usually tested with opt tests nowadays (now added).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in theory. Although I'd prefer to keep it because it matches the minimized example in the report we're looking to fix: google/sanitizers#749

So, keeping that runtime test helps me 'prove' that we've fixed it. I won't insist though, so if you feel strongly that I remove it, I gladly will. Please let me know, thanks!

// 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
Original file line number Diff line number Diff line change
Expand Up @@ -848,6 +848,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 @@ -3001,6 +3002,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) {
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;
}
}
}
}
}
}
Comment on lines 3009 to 3020
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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;
}
}
}
}
}
}
for (Instruction &I : instructions(F)) {
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;
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I manually incorporated this.
I accidentally split it into 2 commits:
65f1741
and
bd511b5


bool AddressSanitizer::suppressInstrumentationSiteForDebug(int &Instrumented) {
bool ShouldInstrument =
Expand Down Expand Up @@ -3045,6 +3064,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
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
; This test ensures that catch parameters are not instrumented on Windows.

; This file was generated using the following source
;
; ```C++
; #include <exception>
; #include <cstdio>
;
; int main() {
; try {
; throw 1;
; } catch (const int ex) {
; printf("%d\n", ex);
; return -1;
; }
; return 0;
; }
;
; ```
; then running the following sequence of commands
;
; ```
; clang.exe -g0 -O0 -emit-llvm -c main.cpp -o main.bc
; llvm-extract.exe -func=main main.bc -o main_func.bc
; llvm-dis.exe main_func.bc -o main_func_dis.ll
; ```
; and finally manually trimming the resulting `.ll` file to remove
; unnecessary metadata, and manually adding the `sanitize_address` annotation;
; needed for the ASan pass to run.

; RUN: opt < %s -passes=asan -S | FileCheck %s
; CHECK: %ex = alloca i32, align 4
; CHECK: catchpad within %{{.*}} [ptr @"??_R0H@8", i32 0, ptr %ex]
Copy link
Contributor

Choose a reason for hiding this comment

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

Testing that %ex does not go through sanitization looks reasonable, thanks for the addition. We usually favour reduced test cases. In this case, as we are only interested in exercizing a used catchpad (i.e., with a active catch funclet), the following could be a valid, slightly reduced IR (e.g., unneeded attributes omitted):

target triple = "x86_64-pc-windows-msvc"

@"??_R0H@8" = external global ptr

define i32 @test() sanitize_address personality ptr @__CxxFrameHandler3 {
entry:
  %ex = alloca i32, align 4
  invoke void @throw()
          to label %unreachable unwind label %catch.dispatch

catch.dispatch:                                   ; preds = %entry
  %0 = catchswitch within none [label %catch] unwind to caller

catch:                                            ; preds = %catch.dispatch
  %1 = catchpad within %0 [ptr @"??_R0H@8", i32 0, ptr %ex]
  call void @opaque() [ "funclet"(token %1) ]
  catchret from %1 to label %return

return:                                           ; preds = %catch
  ret i32 0

unreachable:                                      ; preds = %entry
  unreachable
}

declare void @throw() noreturn
declare void @opaque()
declare i32 @__CxxFrameHandler3(...)

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, I manually incorporated that here: fe312ec


target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc"

%rtti.TypeDescriptor2 = type { ptr, ptr, [3 x i8] }
%eh.ThrowInfo = type { i32, i32, i32, i32 }

@"??_R0H@8" = external global %rtti.TypeDescriptor2
@_TI1H = external unnamed_addr constant %eh.ThrowInfo, section ".xdata"
@"??_C@_03PMGGPEJJ@?$CFd?6?$AA@" = external dso_local unnamed_addr constant [4 x i8], align 1

; Function Attrs: mustprogress noinline norecurse optnone uwtable sanitize_address
define dso_local noundef i32 @main() #0 personality ptr @__CxxFrameHandler3 {
entry:
%retval = alloca i32, align 4
%tmp = alloca i32, align 4
%ex = alloca i32, align 4
store i32 0, ptr %retval, align 4
store i32 1, ptr %tmp, align 4
invoke void @_CxxThrowException(ptr %tmp, ptr @_TI1H) #2
to label %unreachable unwind label %catch.dispatch

catch.dispatch: ; preds = %entry
%0 = catchswitch within none [label %catch] unwind to caller

catch: ; preds = %catch.dispatch
%1 = catchpad within %0 [ptr @"??_R0H@8", i32 0, ptr %ex]
%2 = load i32, ptr %ex, align 4
%call = call i32 (ptr, ...) @printf(ptr noundef @"??_C@_03PMGGPEJJ@?$CFd?6?$AA@", i32 noundef %2) [ "funclet"(token %1) ]
store i32 -1, ptr %retval, align 4
catchret from %1 to label %catchret.dest

catchret.dest: ; preds = %catch
br label %return

try.cont: ; No predecessors!
store i32 0, ptr %retval, align 4
br label %return

return: ; preds = %try.cont, %catchret.dest
%3 = load i32, ptr %retval, align 4
ret i32 %3

unreachable: ; preds = %entry
unreachable
}

declare dso_local void @_CxxThrowException(ptr, ptr)

declare dso_local i32 @__CxxFrameHandler3(...)

; Function Attrs: mustprogress noinline optnone uwtable
declare dso_local i32 @printf(ptr noundef, ...) #1

attributes #0 = { mustprogress noinline norecurse optnone uwtable sanitize_address }
attributes #1 = { mustprogress noinline optnone uwtable }
attributes #2 = { noreturn }
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: missing newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, incorporated here: fe312ec

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch

Don't instrument that catch on Windows :D