-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Sanitizers] Add Driver/Frontend option to enable sanitizer instrumentation that supports error recovery #28126
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
Changes from all commits
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 |
---|---|---|
|
@@ -889,6 +889,15 @@ def sanitize_EQ : CommaJoined<["-"], "sanitize=">, | |
Flags<[FrontendOption, NoInteractiveOption]>, MetaVarName<"<check>">, | ||
HelpText<"Turn on runtime checks for erroneous behavior.">; | ||
|
||
def sanitize_recover_EQ | ||
: CommaJoined<["-"], "sanitize-recover=">, | ||
Flags<[FrontendOption, NoInteractiveOption]>, | ||
MetaVarName<"<check>">, | ||
HelpText<"Specify which sanitizer runtime checks (see -sanitize=) will " | ||
"generate instrumentation that allows error recovery. Listed " | ||
"checks should be comma separated. Default behavior is to not " | ||
"allow error recovery.">; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How should I understand this flag compared to e.g. tsan's You mentioned it only supports asan, but isn't this the default behaviour for tsan? Or is this somehow different from what tsan is doing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good questions. The
No, it does not unconditionally recover. What it does is cause ASan's instrumentation to allow for error recovery. The runtime Today, setting With ASan's default for
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the explanation! Any reason not to accept There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, the flag wouldn't do anything. The ThreadSanitizer pass doesn't have an option to change the instrumentation so there's nothing we can change in the compilation process. It seems odd to me to have a driver/frontend option that doesn't effect the compilation process. Given that we're adding a new option I'd prefer to limit the ways in which it can be used. |
||
|
||
def sanitize_coverage_EQ : CommaJoined<["-"], "sanitize-coverage=">, | ||
Flags<[FrontendOption, NoInteractiveOption]>, | ||
MetaVarName<"<type>">, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// RUN: not %swiftc_driver -driver-print-jobs -sanitize=address -sanitize-recover=foo %s 2>&1 | %FileCheck -check-prefix=SAN_RECOVER_INVALID_ARG %s | ||
// RUN: not %swiftc_driver -driver-print-jobs -sanitize=address -sanitize-recover=thread %s 2>&1 | %FileCheck -check-prefix=SAN_RECOVER_UNSUPPORTED_ARG %s | ||
// RUN: %swiftc_driver -v -sanitize-recover=address %s -o %t 2>&1 | %FileCheck -check-prefix=SAN_RECOVER_MISSING_INSTRUMENTATION_OPTION %s | ||
// RUN: %swiftc_driver -driver-print-jobs -sanitize=address -sanitize-recover=address %s 2>&1 | %FileCheck -check-prefix=ASAN_WITH_RECOVER %s | ||
// RUN: %swiftc_driver -driver-print-jobs -sanitize=address %s 2>&1 | %FileCheck -check-prefix=ASAN_WITHOUT_RECOVER --implicit-check-not='-sanitize-recover=address' %s | ||
|
||
// SAN_RECOVER_INVALID_ARG: unsupported argument 'foo' to option '-sanitize-recover=' | ||
// SAN_RECOVER_UNSUPPORTED_ARG: unsupported argument 'thread' to option '-sanitize-recover=' | ||
|
||
// SAN_RECOVER_MISSING_INSTRUMENTATION_OPTION: warning: option '-sanitize-recover=address' has no effect when 'address' sanitizer is disabled. Use -sanitize=address to enable the sanitizer | ||
// SAN_RECOVER_MISSING_INSTRUMENTATION_OPTION-NOT: warning: option '-sanitize-recover=address' has no effect when 'address' sanitizer is disabled. Use -sanitize=address to enable the sanitizer | ||
|
||
// ASAN_WITH_RECOVER: swift | ||
// ASAN_WITH_RECOVER-DAG: -sanitize=address | ||
// ASAN_WITH_RECOVER-DAG: -sanitize-recover=address | ||
|
||
// ASAN_WITHOUT_RECOVER: swift | ||
// ASAN_WITHOUT_RECOVER: -sanitize=address |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
// RUN: %target-swift-frontend -emit-ir -sanitize=address -sanitize-recover=address %s | %FileCheck %s -check-prefix=ASAN_RECOVER | ||
// RUN: %target-swift-frontend -emit-ir -sanitize=address %s | %FileCheck %s -check-prefix=ASAN_NO_RECOVER | ||
// RUN: %target-swift-frontend -emit-ir -sanitize-recover=address %s | %FileCheck %s -check-prefix=NO_ASAN_RECOVER | ||
|
||
// ASAN_RECOVER: declare void @__asan_loadN_noabort( | ||
// ASAN_NO_RECOVER: declare void @__asan_loadN( | ||
|
||
let size:Int = 128; | ||
let x = UnsafeMutablePointer<UInt8>.allocate(capacity: size) | ||
x.initialize(repeating: 0, count: size) | ||
x.advanced(by: 0).pointee = 5; | ||
print("Read first element:\(x.advanced(by: 0).pointee)") | ||
x.deallocate(); | ||
|
||
// There should be no ASan instrumentation in this case. | ||
// NO_ASAN_RECOVER-NOT: declare void @__asan_load |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
// This file is a swift bridging header to ASan's interface. It exists so | ||
// we don't need to worry about where the header lives and instead let Clang | ||
// figure out where the header lives. | ||
#include "sanitizer/asan_interface.h" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
// REQUIRES: executable_test | ||
// REQUIRES: asan_runtime | ||
// UNSUPPORTED: windows | ||
|
||
// Check with recovery instrumentation and runtime option to continue execution. | ||
// RUN: %target-swiftc_driver %s -target %sanitizers-target-triple -g -sanitize=address -sanitize-recover=address -import-objc-header %S/asan_interface.h -emit-ir -o %t.asan_recover.ll | ||
// RUN: %FileCheck -check-prefix=CHECK-IR -input-file=%t.asan_recover.ll %s | ||
// RUN: %target-swiftc_driver %s -target %sanitizers-target-triple -g -sanitize=address -sanitize-recover=address -import-objc-header %S/asan_interface.h -o %t_asan_recover | ||
// RUN: %target-codesign %t_asan_recover | ||
// RUN: env %env-ASAN_OPTIONS=halt_on_error=0 %target-run %t_asan_recover > %t_asan_recover.stdout 2> %t_asan_recover.stderr | ||
// RUN: %FileCheck --check-prefixes=CHECK-COMMON-STDERR,CHECK-RECOVER-STDERR -input-file=%t_asan_recover.stderr %s | ||
// RUN: %FileCheck --check-prefixes=CHECK-COMMON-STDOUT,CHECK-RECOVER-STDOUT -input-file=%t_asan_recover.stdout %s | ||
|
||
// Check with recovery instrumentation but without runtime option to continue execution. | ||
// RUN: not env %env-ASAN_OPTIONS=abort_on_error=0,halt_on_error=1 %target-run %t_asan_recover > %t_asan_no_runtime_recover.stdout 2> %t_asan_no_runtime_recover.stderr | ||
// RUN: %FileCheck --check-prefixes=CHECK-COMMON-STDERR -input-file=%t_asan_no_runtime_recover.stderr %s | ||
// RUN: %FileCheck --check-prefixes=CHECK-COMMON-STDOUT,CHECK-NO-RECOVER-STDOUT -input-file=%t_asan_no_runtime_recover.stdout %s | ||
|
||
// Check that without recovery instrumentation and runtime option to continue execution that error recovery does not happen. | ||
// RUN: %target-swiftc_driver %s -target %sanitizers-target-triple -g -sanitize=address -import-objc-header %S/asan_interface.h -o %t_asan_no_recover | ||
// RUN: %target-codesign %t_asan_no_recover | ||
// RUN: not env %env-ASAN_OPTIONS=abort_on_error=0,halt_on_error=0 %target-run %t_asan_no_recover > %t_asan_no_recover.stdout 2> %t_asan_no_recover.stderr | ||
// RUN: %FileCheck --check-prefixes=CHECK-COMMON-STDERR -input-file=%t_asan_no_recover.stderr %s | ||
// RUN: %FileCheck --check-prefixes=CHECK-COMMON-STDOUT,CHECK-NO-RECOVER-STDOUT -input-file=%t_asan_no_recover.stdout %s | ||
|
||
// We need to test reads via instrumentation not via runtime so try to check | ||
// for calls to unwanted interceptor/runtime functions. | ||
// CHECK-IR-NOT: call {{.+}} @__asan_memcpy | ||
// CHECK-IR-NOT: call {{.+}} @memcpy | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @brentdax Here's the check I've added. |
||
|
||
// FIXME: We need this so we can flush stdout but this won't | ||
// work on other Platforms (e.g. Windows). | ||
#if os(Linux) | ||
import Glibc | ||
#else | ||
import Darwin.C | ||
#endif | ||
|
||
// CHECK-COMMON-STDOUT: START | ||
print("START") | ||
fflush(stdout) | ||
|
||
let size:Int = 128; | ||
|
||
// In this test we need multiple issues to occur that ASan can detect. | ||
// Allocating a buffer and artificially poisoning it seems like the best way to | ||
// test this because there's no undefined behavior happening. Hopefully this | ||
// means that the program behaviour after ASan catches an issue should be | ||
// consistent. If we did something different like two use-after-free issues the | ||
// behaviour could be very unpredicatable resulting in a flakey test. | ||
var x = UnsafeMutablePointer<UInt8>.allocate(capacity: size) | ||
x.initialize(repeating: 0, count: size) | ||
__asan_poison_memory_region(UnsafeMutableRawPointer(x), size) | ||
|
||
// Perform accesses that ASan will catch. Note it is important here that | ||
// the reads are performed **in** the instrumented code so that the | ||
// instrumentation catches the access to poisoned memory. I tried doing: | ||
// | ||
// ``` | ||
// var x = x.advanced(by: 0).pointee | ||
// print(x) | ||
// ``` | ||
// | ||
// However, this generated code that called into memcpy rather than performing | ||
// a direct read which meant that ASan caught an issue via its interceptors | ||
// rather than from instrumentation, which does not test the right thing here. | ||
// | ||
// Doing: | ||
// | ||
// ``` | ||
// print("Read first element:\(x.advanced(by: 0).pointee)") | ||
// ``` | ||
// | ||
// seems to do the right thing right now but this seems really fragile. | ||
beccadax marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// First error | ||
// NOTE: Testing for stackframe `#0` should ensure that the poison read | ||
// happened in instrumentation and not in an interceptor. | ||
// CHECK-COMMON-STDERR: AddressSanitizer: use-after-poison | ||
// CHECK-COMMON-STDERR: #0 0x{{.+}} in main {{.*}}asan_recover.swift:[[@LINE+1]] | ||
print("Read first element:\(x.advanced(by: 0).pointee)") | ||
fflush(stdout) | ||
// CHECK-RECOVER-STDOUT: Read first element:0 | ||
|
||
// Second error | ||
// CHECK-RECOVER-STDERR: AddressSanitizer: use-after-poison | ||
// CHECK-RECOVER-STDERR: #0 0x{{.+}} in main {{.*}}asan_recover.swift:[[@LINE+1]] | ||
print("Read second element:\(x.advanced(by: 1).pointee)") | ||
fflush(stdout) | ||
// CHECK-RECOVER-STDOUT: Read second element:0 | ||
|
||
__asan_unpoison_memory_region(UnsafeMutableRawPointer(x), size) | ||
|
||
x.deallocate(); | ||
// CHECK-NO-RECOVER-STDOUT-NOT: DONE | ||
// CHECK-RECOVER-STDOUT: DONE | ||
print("DONE") | ||
fflush(stdout) |
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.
Why not automatically enable the matching sanitizer?
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.
Two reasons
-sanitize-recover=address
without-sanitize=address
is a mistake in the build system and we should catch that.-fsanitize-recover=address
either. It doesn't produce an error though, but I think producing an error here is better.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.
@brentdax I've actually changed my mind here. After thinking about this some more I realised there's a legitimate use case where passing
-sanitize-recover=address
without-sanitize=address
should not result in an error. Consider a scenario where-sanitize-recover=address
is globally passed to the build of a project but-sanitize-recover=address
is not enabled everywhere in the project (e.g. project is being incrementally ported to support ASan). The current version of this PR would cause a compiler error which is unhelpful. Instead I'm going to make this a warning.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.
I've made this a warning now.