Skip to content

[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

Merged
merged 1 commit into from
Nov 14, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 4 additions & 0 deletions include/swift/AST/DiagnosticsFrontend.def
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@ ERROR(error_option_requires_sanitizer, none,
"option '%0' requires a sanitizer to be enabled. Use -sanitize= to "
"enable a sanitizer", (StringRef))

WARNING(warning_option_requires_specific_sanitizer, none,
"option '%0' has no effect when '%1' sanitizer is disabled. Use -sanitize=%1 to "
"enable the sanitizer", (StringRef, StringRef))
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two reasons

  1. Using -sanitize-recover=address without -sanitize=address is a mistake in the build system and we should catch that.
  2. Clang doesn't automatically enable a sanitizer when provided -fsanitize-recover=address either. It doesn't produce an error though, but I think producing an error here is better.

Copy link
Contributor Author

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.

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've made this a warning now.


ERROR(error_option_missing_required_argument, none,
"option '%0' is missing a required argument (%1)", (StringRef, StringRef))

Expand Down
4 changes: 4 additions & 0 deletions include/swift/AST/IRGenOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ class IRGenOptions {
/// Which sanitizer is turned on.
OptionSet<SanitizerKind> Sanitizers;

/// Which sanitizer(s) have recovery instrumentation enabled.
OptionSet<SanitizerKind> SanitizersWithRecoveryInstrumentation;

/// Path prefixes that should be rewritten in debug info.
PathRemapper DebugPrefixMap;

Expand Down Expand Up @@ -239,6 +242,7 @@ class IRGenOptions {
: DWARFVersion(2), OutputKind(IRGenOutputKind::LLVMAssembly),
Verify(true), OptMode(OptimizationMode::NotSet),
Sanitizers(OptionSet<SanitizerKind>()),
SanitizersWithRecoveryInstrumentation(OptionSet<SanitizerKind>()),
DebugInfoLevel(IRGenDebugInfoLevel::None),
DebugInfoFormat(IRGenDebugInfoFormat::None),
DisableClangModuleSkeletonCUs(false), UseJIT(false),
Expand Down
9 changes: 9 additions & 0 deletions include/swift/Option/Options.td
Original file line number Diff line number Diff line change
Expand Up @@ -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.">;
Copy link
Contributor

Choose a reason for hiding this comment

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

How should I understand this flag compared to e.g. tsan's halt_on_error=0? If I set sanitize-recover=address does it unconditionally recover or can I say ASAN_OPTIONS= halt_on_error=1?

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?

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

The halt_on_error flag is actually not TSan specific. The other sanitizers have it too.

If I set sanitize-recover=address does it unconditionally recover or can I say ASAN_OPTIONS= halt_on_error=1?

No, it does not unconditionally recover. What it does is cause ASan's instrumentation to allow for error recovery. The runtime halt_on_error option is used to decide what to do when ASan detects an issue.

Today, setting ASAN_OPTIONS=halt_on_error=0 doesn't always work. If you compile without the -sanitize-recover=address option (equivalent to the current behavior of the swift compiler) then the generated instrumentation doesn't allow for error recovery. What this means is that if you set ASAN_OPTIONS=halt_on_error=0 at runtime and if an ASan issue is caught via instrumentation then the process will always halt regardless of how halt_on_error is set. However, if ASan catches an issue via one of its interceptors (e.g. memcpy) then halt_on_error runtime option is respected.

With -sanitize-recover=address the generated instrumentation allows for error recovery which means that the halt_on_error runtime option always does what you would expect.

ASan's default for halt_on_error is true which means this issue only effects people who choose to not use the default behavior.

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?

halt_on_error defaults to false for TSan. The difference here is that TSan's instrumentation pass always allows for error recovery but ASan's instrumentation pass does not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation! Any reason not to accept -sanitize-recover=thread then for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>">,
Expand Down
4 changes: 4 additions & 0 deletions include/swift/Option/SanitizerOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ OptionSet<SanitizerKind> parseSanitizerArgValues(
const llvm::Triple &Triple, DiagnosticEngine &Diag,
llvm::function_ref<bool(llvm::StringRef, bool)> sanitizerRuntimeLibExists);

OptionSet<SanitizerKind> parseSanitizerRecoverArgValues(
const llvm::opt::Arg *A, const OptionSet<SanitizerKind> &enabledSanitizers,
DiagnosticEngine &Diags, bool emitWarnings);

/// Parses a -sanitize-coverage= argument's value.
llvm::SanitizerCoverageOptions parseSanitizerCoverageArgValue(
const llvm::opt::Arg *A,
Expand Down
8 changes: 8 additions & 0 deletions lib/Driver/Driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1636,6 +1636,14 @@ void Driver::buildOutputInfo(const ToolChain &TC, const DerivedArgList &Args,
return TC.sanitizerRuntimeLibExists(Args, sanitizerName, shared);
});

if (const Arg *A = Args.getLastArg(options::OPT_sanitize_recover_EQ)) {
// Just validate the args. The frontend will parse these again and actually
// use them. To avoid emitting warnings multiple times we surpress warnings
// here but not in the frontend.
(void)parseSanitizerRecoverArgValues(A, OI.SelectedSanitizers, Diags,
/*emitWarnings=*/false);
}

if (const Arg *A = Args.getLastArg(options::OPT_sanitize_coverage_EQ)) {

// Check that the sanitizer coverage flags are supported if supplied.
Expand Down
1 change: 1 addition & 0 deletions lib/Driver/ToolChains.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ static void addCommonFrontendArgs(const ToolChain &TC, const OutputInfo &OI,
inputArgs.AddLastArg(arguments, options::OPT_profile_coverage_mapping);
inputArgs.AddLastArg(arguments, options::OPT_warnings_as_errors);
inputArgs.AddLastArg(arguments, options::OPT_sanitize_EQ);
inputArgs.AddLastArg(arguments, options::OPT_sanitize_recover_EQ);
inputArgs.AddLastArg(arguments, options::OPT_sanitize_coverage_EQ);
inputArgs.AddLastArg(arguments, options::OPT_static);
inputArgs.AddLastArg(arguments, options::OPT_swift_version);
Expand Down
6 changes: 6 additions & 0 deletions lib/Frontend/CompilerInvocation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,12 @@ static bool ParseSILArgs(SILOptions &Opts, ArgList &Args,
IRGenOpts.Sanitizers = Opts.Sanitizers;
}

if (const Arg *A = Args.getLastArg(options::OPT_sanitize_recover_EQ)) {
IRGenOpts.SanitizersWithRecoveryInstrumentation =
parseSanitizerRecoverArgValues(A, Opts.Sanitizers, Diags,
/*emitWarnings=*/true);
}

if (auto A = Args.getLastArg(OPT_enable_verify_exclusivity,
OPT_disable_verify_exclusivity)) {
Opts.VerifyExclusivity
Expand Down
10 changes: 8 additions & 2 deletions lib/IRGen/IRGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,14 @@ static void addSwiftMergeFunctionsPass(const PassManagerBuilder &Builder,

static void addAddressSanitizerPasses(const PassManagerBuilder &Builder,
legacy::PassManagerBase &PM) {
PM.add(createAddressSanitizerFunctionPass());
PM.add(createModuleAddressSanitizerLegacyPassPass());
auto &BuilderWrapper =
static_cast<const PassManagerBuilderWrapper &>(Builder);
auto recover =
bool(BuilderWrapper.IRGOpts.SanitizersWithRecoveryInstrumentation &
SanitizerKind::Address);
PM.add(createAddressSanitizerFunctionPass(/*CompileKernel=*/false, recover));
PM.add(createModuleAddressSanitizerLegacyPassPass(/*CompileKernel=*/false,
recover));
}

static void addThreadSanitizerPass(const PassManagerBuilder &Builder,
Expand Down
43 changes: 43 additions & 0 deletions lib/Option/SanitizerOptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,49 @@ OptionSet<SanitizerKind> swift::parseSanitizerArgValues(
return sanitizerSet;
}

OptionSet<SanitizerKind> swift::parseSanitizerRecoverArgValues(
const llvm::opt::Arg *A, const OptionSet<SanitizerKind> &enabledSanitizers,
DiagnosticEngine &Diags, bool emitWarnings) {
OptionSet<SanitizerKind> sanitizerRecoverSet;

// Find the sanitizer kind.
for (const char *arg : A->getValues()) {
Optional<SanitizerKind> optKind = parse(arg);

// Unrecognized sanitizer option
if (!optKind.hasValue()) {
Diags.diagnose(SourceLoc(), diag::error_unsupported_option_argument,
A->getOption().getPrefixedName(), arg);
continue;
}
SanitizerKind kind = optKind.getValue();

// Only support ASan for now.
if (kind != SanitizerKind::Address) {
Diags.diagnose(SourceLoc(), diag::error_unsupported_option_argument,
A->getOption().getPrefixedName(), arg);
continue;
}

// Check that the sanitizer is enabled.
if (!(enabledSanitizers & kind)) {
SmallString<128> b;
if (emitWarnings) {
Diags.diagnose(SourceLoc(),
diag::warning_option_requires_specific_sanitizer,
(A->getOption().getPrefixedName() + toStringRef(kind))
.toStringRef(b),
toStringRef(kind));
}
continue;
}

sanitizerRecoverSet |= kind;
}

return sanitizerRecoverSet;
}

std::string swift::getSanitizerList(const OptionSet<SanitizerKind> &Set) {
std::string list;
#define SANITIZER(_, kind, name, file) \
Expand Down
18 changes: 18 additions & 0 deletions test/Driver/sanitize_recover.swift
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
16 changes: 16 additions & 0 deletions test/IRGen/address_sanitizer_recover.swift
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
4 changes: 4 additions & 0 deletions test/Sanitizers/asan_interface.h
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"
98 changes: 98 additions & 0 deletions test/Sanitizers/asan_recover.swift
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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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