[Sanitizers] Add Driver/Frontend option to enable sanitizer instrumentation that supports error recovery #28126

danliew-apple · 2019-11-07T04:48:02Z

The new option -sanitize-recover= takes a list of sanitizers that
recovery instrumentation should be enabled for. Currently we only
support it for Address Sanitizer.

If the option is not specified then the generated instrumentation does
not allow error recovery.

This option mirrors the -fsanitize-recover= option of Clang.

We don't enable recoverable instrumentation by default because it may
lead to code size blow up (control flow has to be resumable).

rdar://problem/56346688

yln

This comes with tests for: driver, IRGen, and full integration-style execution tests. Good work!

LGTM. Let's also get approval from the Swift folks.

lib/Option/SanitizerOptions.cpp

lib/IRGen/IRGen.cpp

benlangmuir · 2019-11-07T17:23:04Z

include/swift/Option/Options.td

+      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.">;


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?

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.

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

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.

danliew-apple · 2019-11-07T19:25:56Z

@swift-ci Please smoke test

danliew-apple · 2019-11-07T21:47:20Z

Interesting. The test doesn't seem to work on Linux

12:38:45 : 'RUN: at line 21';   '/usr/bin/python' '/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test@2/branch-master/swift/utils/PathSanitizingFileCheck' --sanitize BUILD_DIR='/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test@2/branch-master/buildbot_linux/swift-linux-x86_64' --sanitize SOURCE_DIR='/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test@2/branch-master/swift' --use-filecheck '/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test@2/branch-master/buildbot_linux/llvm-linux-x86_64/bin/FileCheck'  --check-prefixes=CHECK-COMMON-STDOUT,CHECK-NO-RECOVER-STDOUT -input-file=/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test@2/branch-master/buildbot_linux/swift-linux-x86_64/test-linux-x86_64/Sanitizers/Output/asan_recover.swift.tmp_asan_no_recover.stdout /home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test@2/branch-master/swift/test/Sanitizers/asan_recover.swift
12:38:45 --
12:38:45 Exit Code: 1
12:38:45 
12:38:45 Command Output (stdout):
12:38:45 --
12:38:45 /home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test@2/branch-master/buildbot_linux/swift-linux-x86_64/test-linux-x86_64/Sanitizers/Output/asan_recover.swift.tmp_asan_recover
12:38:45 
12:38:45 --
12:38:45 Command Output (stderr):
12:38:45 --
12:38:45 /usr/bin/ld.gold: warning: Cannot export local symbol '__asan_extra_spill_area'
12:38:45 
/home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test@2/branch-master/swift/test/Sanitizers/asan_recover.swift:72:25: error: CHECK-COMMON-STDERR: expected string not found in input
12:38:45 // CHECK-COMMON-STDERR: #0 0x{{.+}} in main asan_recover.swift:[[@LINE+1]]
12:38:45                         ^
12:38:45 /home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test@2/branch-master/buildbot_linux/swift-linux-x86_64/test-linux-x86_64/Sanitizers/Output/asan_recover.swift.tmp_asan_recover.stderr:2:52: note: scanning from here
12:38:45 ==26270==ERROR: AddressSanitizer: use-after-poison on address 0x60d000000040 at pc 0x55bef441207e bp 0x7fff4935c230 sp 0x7fff4935c228
12:38:45                                                    ^
12:38:45 /home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test@2/branch-master/buildbot_linux/swift-linux-x86_64/test-linux-x86_64/Sanitizers/Output/asan_recover.swift.tmp_asan_recover.stderr:2:52: note: with "@LINE+1" equal to "73"
12:38:45 ==26270==ERROR: AddressSanitizer: use-after-poison on address 0x60d000000040 at pc 0x55bef441207e bp 0x7fff4935c230 sp 0x7fff4935c228
12:38:45                                                    ^
12:38:45 /home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test@2/branch-master/buildbot_linux/swift-linux-x86_64/test-linux-x86_64/Sanitizers/Output/asan_recover.swift.tmp_asan_recover.stderr:4:107: note: possible intended match here
12:38:45  #0 0x55bef441207d in main /home/buildnode/jenkins/workspace/swift-PR-Linux-smoke-test@2/branch-master/swift/test/Sanitizers/asan_recover.swift:73:47

Apart from the Linker complaining (@kubamracek @yln should I worry about this?) it looks like on Linux the symbolized source location is the full path rather than just the filename. That should be easy to fix.

danliew-apple · 2019-11-07T22:03:00Z

@swift-ci Please smoke test

yln · 2019-11-07T22:24:02Z

Linker: I don't understand what this means, but I think we can ignore it for this PR.
Full path names on Linux: we should handle this the same way as other tests do. Do they use{{.*}}?

danliew-apple · 2019-11-07T23:25:23Z

Linker: I don't understand what this means, but I think we can ignore it for this PR.
Full path names on Linux: we should handle this the same way as other tests do. Do they use{{.*}}?

Well something similar:

beccadax

I have some questions and ideas, but if none of them are valid, I think this is fine as is.

beccadax · 2019-11-08T23:51:00Z

include/swift/AST/DiagnosticsFrontend.def

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

+ERROR(error_option_requires_specific_sanitizer, none,
+      "option '%0' requires '%1' sanitizer to be enabled. Use -sanitize=%1 to "
+      "enable the sanitizer", (StringRef, StringRef))


Why not automatically enable the matching sanitizer?

Two reasons

Using -sanitize-recover=address without -sanitize=address is a mistake in the build system and we should catch that.

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.

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

I've made this a warning now.

lib/IRGen/IRGen.cpp

test/Sanitizers/asan_recover.swift

…tation that supports error recovery. The new option `-sanitize-recover=` takes a list of sanitizers that recovery instrumentation should be enabled for. Currently we only support it for Address Sanitizer. If the option is not specified then the generated instrumentation does not allow error recovery. This option mirrors the `-fsanitize-recover=` option of Clang. We don't enable recoverable instrumentation by default because it may lead to code size blow up (control flow has to be resumable). The motivation behind this change is that today, setting `ASAN_OPTIONS=halt_on_error=0` at runtime 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 `the 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 is also respected when the ASan issue is caught by instrumentation. ASan's default for `halt_on_error` is true which means this issue only effects people who choose to not use the default behavior. rdar://problem/56346688

danliew-apple · 2019-11-12T19:36:09Z

@swift-ci Please smoke test

danliew-apple · 2019-11-12T19:37:35Z

test/Sanitizers/asan_recover.swift

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


@brentdax Here's the check I've added.

danliew-apple · 2019-11-12T21:07:06Z

@brentdax @benlangmuir I've done another pass other the PR and the smoke tests pass. Could I get an approval on this PR or some feedback if there are still problems that need addressing?

danliew-apple requested review from compnerd, yln and devincoughlin November 7, 2019 04:48

danliew-apple added the enhancement label Nov 7, 2019

danliew-apple requested a review from kubamracek November 7, 2019 04:49

danliew-apple force-pushed the rdar_56346688 branch from 61a5477 to 7426a79 Compare November 7, 2019 04:58

yln reviewed Nov 7, 2019

View reviewed changes

lib/Option/SanitizerOptions.cpp Show resolved Hide resolved

lib/IRGen/IRGen.cpp Outdated Show resolved Hide resolved

yln requested review from beccadax and benlangmuir November 7, 2019 16:50

danliew-apple requested a review from ravikandhadai November 7, 2019 17:12

benlangmuir reviewed Nov 7, 2019

View reviewed changes

danliew-apple force-pushed the rdar_56346688 branch from 7426a79 to 2b24b47 Compare November 7, 2019 19:15

danliew-apple force-pushed the rdar_56346688 branch from 2b24b47 to 08ffc91 Compare November 7, 2019 22:02

danliew-apple removed the request for review from compnerd November 7, 2019 23:36

beccadax reviewed Nov 9, 2019

View reviewed changes

danliew-apple force-pushed the rdar_56346688 branch from 08ffc91 to 63e7290 Compare November 12, 2019 19:34

danliew-apple commented Nov 12, 2019

View reviewed changes

benlangmuir approved these changes Nov 13, 2019

View reviewed changes

delcypher merged commit 5446d33 into swiftlang:master Nov 14, 2019

drodriguez mentioned this pull request Nov 14, 2019

[windows] SourceKit tests with VFS fixes #28250

Merged

AnthonyLatsis added improvement and removed enhancement labels Nov 13, 2022

[Sanitizers] Add Driver/Frontend option to enable sanitizer instrumentation that supports error recovery #28126

[Sanitizers] Add Driver/Frontend option to enable sanitizer instrumentation that supports error recovery #28126

Uh oh!

Conversation

danliew-apple commented Nov 7, 2019

Uh oh!

yln left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danliew-apple commented Nov 7, 2019

Uh oh!

danliew-apple commented Nov 7, 2019

Uh oh!

danliew-apple commented Nov 7, 2019

Uh oh!

yln commented Nov 7, 2019

Uh oh!

danliew-apple commented Nov 7, 2019

Uh oh!

beccadax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danliew-apple commented Nov 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danliew-apple commented Nov 12, 2019

Uh oh!

Uh oh!