[Sanitizers] Add Scudo support #28538

Lukasa · 2019-12-03T18:40:30Z

LLVM ships a hardened memory allocator called Scudo:
https://llvm.org/docs/ScudoHardenedAllocator.html. This allocator
provides additional mitigations against heap-based vulnerabilities, but
retains sufficient performance to be safely run in production
applications.

While ideal Swift applications are obviously written in pure Swift, in
practice most applications contain some amount of code written in
less-safe languages. Additionally, plenty of Swift programs themselves
contain unsafe code, particularly when attempting to implement
high-performance data structures. These sources of unsafety introduce
the risk of memory issues, and having the option to use the Scudo
allocator is a useful defense-in-depth tool.

This patch enables -sanitize=scudo as an extra swiftc flag. This
sanitizer is only supported on Linux, so no further work is required to
enable it on Windows or Apple platforms. As this "sanitizer" is only a
runtime component, we do not require any wider changes to instrument
code.

The result of this patch matches the API of clang, which supports
-fsanitize=scudo.

I do have one question: currently this patch contains no regression tests. I was fundamentally unsure of where to place these regression tests, but I think we should have them, so please do as part of your review suggest a location and form for the regression tests.

Lukasa · 2019-12-03T18:40:42Z

@swift-ci please smoke test

Lukasa · 2019-12-03T18:59:27Z

I should note that, when we update the version of llvm we're using, enabling Scudo will also enable GWP-Asan, which provides not just mitigation but active detection of problems.

The Swift PR swiftlang/swift#28538 adds support for the Scudo hardened allocator to swiftc. This patch adds support for it to Swift Package Manager, allowing the Package Manager to correctly plumb Scudo through the entire build pipeline. See the aforementioned PR for more discussion on the value of supporting Scudo.

danliew-apple · 2019-12-06T19:34:58Z

@Lukasa Thanks for the patch. There are something things that need to be addressed before this can land.

Given that this only works on Linux you need to add code in the Swift driver to refuse to use -sanitize=scudo on non Linux platforms. I can't see anything obvious in the code you added that forbids this.
This patch doesn't change anything about the compilation process. AFAIK Scudo is a replacement allocator library so I would expect that the linker would need to be told to link against the scudo runtime.
This is missing tests. You need:

A test for successful driver invocation on Linux. You should probably check that the linker flags look right (i.e. links against Scudo runtime).
A test for driver failure on non-Linux platforms.
Tests for the invalid santizer+scudo combination.
At least one test case where we compile and run a program that uses Scudo.

danliew-apple

See my initial comments.

Lukasa · 2019-12-06T19:40:01Z

Thanks @danliew-apple!

Given that this only works on Linux you need to add code in the Swift driver to refuse to use -sanitize=scudo on non Linux platforms. I can't see anything obvious in the code you added that forbids this.

I don't believe this is true. There are separate Sanitizer whitelists for Darwin and Windows which explicitly whitelist the sanitisers they support. I have observed that with this toolchain built on Darwin the Scudo flag is rejected: I believe this is true for Windows as well, though @compnerd could further clarify.

This patch doesn't change anything about the compilation process. AFAIK Scudo is a replacement allocator library so I would expect that the linker would need to be told to link against the scudo runtime.

It turns out it already is. On Linux the driver will simply pass the sanitizer option through to clang, which drives the actual linking process. This results in the sanitising being applied. Again, I've verified this in live code.

This is missing tests.

Fantastic. Can you point me to places in the code that do similar testing? I wasn't able to find them, and that made it very hard to write more.

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

@Lukasa Here are some examples:

Here's an example driver test that checks that the compiler refuses certain flags. The test is also checking that the driver passes an option to the Swift frontend. That isn't relevant in this case so you can ignore that.
This test does a bunch of things but one the things it does is check that a sanitizer runtime library is being passed to the linker.
Here's an example of a test that builds and executes code

Note that llvm-lit has a notion of features and you can use this to make a test run on only certain platforms. My suggestion is that

Tests for driver options should probably execute on all platforms. In these tests you'd explicit specify the target so that we can verify that the driver allows Scudo on Linux but rejects it for Windows on macOS.
Tests that are only going to work on Linux should be annotated with REQUIRES: OS=linux-gnu.

Lukasa · 2019-12-06T21:08:26Z

Thanks @danliew-apple, I’ll write some tests to cover this and ping for re-review when I’ve done so.

danliew-apple · 2019-12-06T21:13:30Z

I don't believe this is true. There are separate Sanitizer whitelists for Darwin and Windows which explicitly whitelist the sanitisers they support. I have observed that with this toolchain built on Darwin the Scudo flag is rejected: I believe this is true for Windows as well, though @compnerd could further clarify.

The code you link to is related to linking. The driver should fail compilation much earlier than that. If someone passes -sanitizer=scudo option for any mode of the driver and the platform doesn't support it then it should fail.

This patch doesn't change anything about the compilation process. AFAIK Scudo is a replacement allocator library so I would expect that the linker would need to be told to link against the scudo runtime.

It turns out it already is. On Linux the driver will simply pass the sanitizer option through to clang, which drives the actual linking process. This results in the sanitising being applied. Again, I've verified this in live code.

Interesting. I would not have expected that given that the Swift compiler shouldn't know what to do with this new enum member you've added. You'll need to show me working tests to convince me.

danliew-apple · 2019-12-06T21:18:02Z

@Lukasa Just to be clear I'm only giving you a review the code as I see it. I don't work on the swift compiler very frequently so I think you will need approval from others to land this. Given that this is very untested I suspect we might want to label it as experimental.

Lukasa · 2019-12-06T21:35:10Z

@danliew-apple Totally understood, and I’m very appreciative of your time. I’m confident the patch-as-written works in the straightforward cases because I tested it live, but I also totally agree that without the tests it is hard for anyone else to validate my confidence! So I absolutely appreciate the feedback. I’ll work on adding those tests, and we can worry about wider approval once I’ve dealt with this feedback.

danliew-apple · 2019-12-07T00:54:22Z

@airspeedswift @mikeash Does adopting the hardened Scudo on Linux seem a reasonable thing to support?

danliew-apple · 2019-12-07T00:57:15Z

@Lukasa I just noticed that the Scudo documentation says.. "Compiling with Scudo will also enforce PIE for the output binary". That's the behaviour for Clang. Do you know why they do this? If there's a good reason for this we might need to do the same thing for Swift. I don't know if Swift has an equivalent PIE mode.

Lukasa · 2019-12-09T10:53:25Z

@swift-ci please smoke test

Lukasa · 2019-12-09T11:29:55Z

@swift-ci please smoke test

Lukasa · 2019-12-09T11:52:51Z

@swift-ci please smoke test

Lukasa · 2019-12-09T14:23:49Z

@swift-ci please smoke test

mikeash · 2019-12-09T15:00:48Z

@danliew-apple I'm not too familiar with it, but reading through the link you provided, this seems like a great thing to support. Especially since the hard work is in LLVM and we apparently just need to support the option and pass it along. @weissi Any additional thoughts?

weissi · 2019-12-09T15:03:35Z

@mikeash / @Lukasa this looks good to me, IMHO definitely something we want :)

Lukasa · 2019-12-09T15:49:22Z

@swift-ci please smoke test

Lukasa · 2019-12-09T16:29:45Z

@swift-ci please smoke test

Lukasa · 2019-12-09T16:50:33Z

@swift-ci please smoke test

Lukasa · 2019-12-09T17:48:15Z

@swift-ci please smoke test

Lukasa · 2020-01-17T10:01:55Z

@swift-ci please smoke test

Lukasa · 2020-01-17T12:19:23Z

@swift-ci please test

swift-ci · 2020-01-17T12:21:54Z

Build failed
Swift Test Linux Platform
Git Sha - 828dbfc3a77f4dd87848bb9cb00cba0cb5b493e4

swift-ci · 2020-01-17T12:22:51Z

Build failed
Swift Test OS X Platform
Git Sha - 828dbfc3a77f4dd87848bb9cb00cba0cb5b493e4

danliew-apple

There's still an issue with the test that needs to be resolved.

I also have a very minor nit with the commit message.

The result of this patch matches the API of clang, which supports
-fsanitize=scudo.

Technically -fsanitize=scudo isn't really an API and even then its not an exact match because the command line flag name is different.

Maybe when you say

This patch enables -sanitize=scudo as an extra swiftc flag.

You could say as the next sentence.

This is similar to Clang's -fsanitize=scudo flag.

danliew-apple · 2020-01-17T18:54:31Z

test/Driver/sanitize_scudo.swift

+/*
+ * Make sure we don't accidentally add the sanitizer library path when building libraries or modules
+ */
+// RUN: %swiftc_driver -resource-dir %S/Inputs/fake-resource-dir/lib/swift/ -driver-print-jobs -emit-library -sanitize=scudo -target x86_64-unknown-linux-gnu %s 2>&1 | %FileCheck --implicit-check-not=SCUDO_LINUX %s


I think this is right: when building only libraries, we don't want to add the sanitiser because it should only be added when building the main binary. This is the same as the requirement on all the other sanitisers with runtime libraries: only the main binary actually invokes the linker to link the sanitiser runtime.

I wasn't questioning that aspect. I was questioning the FileCheck invocation which I'm fairly certain is wrong because you're not using --implicit-check-not as intended (see the FileCheck documentation). You're supposed to give it a pattern and not a check prefix which is what the current patch does.

Sure the test currently passes but that's because the Swift compiler isn't producing the text SWIFT_LINUX in its output. So the test is passing by accident and you're not testing what you actually testing what you want to test.

LLVM ships a hardened memory allocator called Scudo: https://llvm.org/docs/ScudoHardenedAllocator.html. This allocator provides additional mitigations against heap-based vulnerabilities, but retains sufficient performance to be safely run in production applications. While ideal Swift applications are obviously written in pure Swift, in practice most applications contain some amount of code written in less-safe languages. Additionally, plenty of Swift programs themselves contain unsafe code, particularly when attempting to implement high-performance data structures. These sources of unsafety introduce the risk of memory issues, and having the option to use the Scudo allocator is a useful defense-in-depth tool. This patch enables `-sanitize=scudo` as an extra `swiftc` flag. This sanitizer is only supported on Linux, so no further work is required to enable it on Windows or Apple platforms. As this "sanitizer" is only a runtime component, we do not require any wider changes to instrument code. This is similar to clang's `-fsanitize=scudo` flag. The Swift driver rejects platforms that don't support Scudo using an existing mechanism in the Driver that is not part of this patch. This mechanism is in swift::parseSanitizerArgValues(...) (lib/Option/SanitizerOptions.cpp). The mechanism determines if a sanitizer is supported by checking for the existence of the corresponding sanitizer runtime library in the compiler's resource directory. The Scudo runtime library currently only exists in the Linux compiler resource directory. This results in the driver only allowing Scudo when targeting Linux.

Lukasa · 2020-01-20T10:09:20Z

@swift-ci please smoke test

Lukasa · 2020-01-20T11:23:36Z

Thanks for that clarification @danliew-apple, that helped a lot! I have rewritten that test and also updated the commit message, please take another look.

Lukasa · 2020-01-20T12:10:45Z

@swift-ci please test

swift-ci · 2020-01-20T12:12:55Z

Build failed
Swift Test OS X Platform
Git Sha - 32f18ce0b0aa39b3b4c6a7bb1a93c37a283b9f6b

swift-ci · 2020-01-20T12:13:15Z

Build failed
Swift Test Linux Platform
Git Sha - 32f18ce0b0aa39b3b4c6a7bb1a93c37a283b9f6b

danliew-apple · 2020-01-24T19:02:49Z

@swift-ci please test

danliew-apple · 2020-01-24T19:03:47Z

@Lukasa Sorry for being slow. I've approved the changes. Thanks for being patient.

Lukasa · 2020-01-24T19:49:46Z

No need to apologise, I understand completely that we’re all busy! Thanks so much for your time on this.

swift-ci · 2020-01-24T19:55:22Z

Build failed
Swift Test Linux Platform
Git Sha - 4fb1920

danliew-apple · 2020-01-25T01:48:17Z

@swift-ci please test

compnerd · 2020-01-27T06:34:47Z

I believe that this has caused a regression on Windows: https://ci-external.swift.org/job/oss-swift-windows-x86_64/2607/

Lukasa · 2020-01-27T10:00:13Z

Good catch @compnerd, that fix looks right. Sorry about that!

The Swift PR swiftlang/swift#28538 adds support for the Scudo hardened allocator to swiftc. This patch adds support for it to Swift Package Manager, allowing the Package Manager to correctly plumb Scudo through the entire build pipeline. See the aforementioned PR for more discussion on the value of supporting Scudo.

The Swift PR swiftlang/swift#28538 adds support for the Scudo hardened allocator to swiftc. This patch adds support for it to Swift Package Manager, allowing the Package Manager to correctly plumb Scudo through the entire build pipeline. See the aforementioned PR for more discussion on the value of supporting Scudo. (cherry picked from commit 594ccaa)

The Swift PR swiftlang/swift#28538 adds support for the Scudo hardened allocator to swiftc. This patch adds support for it to Swift Package Manager, allowing the Package Manager to correctly plumb Scudo through the entire build pipeline. It also brings in the testing strategy from See the aforementioned PR for more discussion on the value of supporting Scudo. (cherry picked from commit 594ccaa)

The Swift PR swiftlang/swift#28538 adds support for the Scudo hardened allocator to swiftc. This patch adds support for it to Swift Package Manager, allowing the Package Manager to correctly plumb Scudo through the entire build pipeline. See the aforementioned PR for more discussion on the value of supporting Scudo. (cherry picked from commit 594ccaa)

* Add support for the Scudo sanitizer. + Test sanitizers with build plans The Swift PR swiftlang/swift#28538 adds support for the Scudo hardened allocator to swiftc. This patch adds support for it to Swift Package Manager, allowing the Package Manager to correctly plumb Scudo through the entire build pipeline. See the aforementioned PR for more discussion on the value of supporting Scudo. (cherry picked from commit 594ccaa) * Test sanitizers with build plans. (#2720) The current tests for sanitizers in SwiftPM are essentially "live": they build a package that will do something to trigger a sanitizer, and then run it and confirm it fails. This is a pretty fragile way to test, and it exposes SwiftPM to a number of possible test failures that don't have anything to do with the core mission (exposing the sanitizer). This patch replaces those tests with build plan tests that validate that SwiftPM correctly invokes swiftc and clang. These tests are acceptable because for now SwiftPM doesn't have any smarts with the sanitizers: it just passes them all through to swiftc and clang. This patch also deletes the current live tests, as they are no longer necessary. (cherry picked from commit 984618b)

Lukasa mentioned this pull request Dec 4, 2019

Add support for the Scudo sanitizer. swiftlang/swift-package-manager#2438

Merged

kubamracek requested review from yln, danliew-apple and devincoughlin December 6, 2019 15:52

danliew-apple requested changes Dec 6, 2019

View reviewed changes

danliew-apple requested review from airspeedswift and mikeash December 7, 2019 00:15

Lukasa force-pushed the cb-scudo branch from 0b5d285 to 790810c Compare December 9, 2019 11:29

danliew-apple requested changes Jan 17, 2020

View reviewed changes

Lukasa force-pushed the cb-scudo branch from 32f18ce to 4fb1920 Compare January 20, 2020 10:09

danliew-apple approved these changes Jan 24, 2020

View reviewed changes

danliew-apple merged commit 5652986 into swiftlang:master Jan 27, 2020

compnerd added a commit to compnerd/apple-swift that referenced this pull request Jan 27, 2020

test: attempt to fix swiftlang#28538

fbd51e2

davezarzycki mentioned this pull request Jan 27, 2020

[Testing] Unbreak Linux builds that do not have the scudo runtime #29470

Merged

Lukasa mentioned this pull request Apr 29, 2020

Add support for the Scudo sanitizer. swiftlang/swift-package-manager#2717

Merged

[Sanitizers] Add Scudo support #28538

[Sanitizers] Add Scudo support #28538

Uh oh!

Conversation

Lukasa commented Dec 3, 2019

Uh oh!

Lukasa commented Dec 3, 2019

Uh oh!

Lukasa commented Dec 3, 2019

Uh oh!

danliew-apple commented Dec 6, 2019

Uh oh!

danliew-apple left a comment

Choose a reason for hiding this comment

Uh oh!

Lukasa commented Dec 6, 2019

Uh oh!

danliew-apple commented Dec 6, 2019

Uh oh!

Lukasa commented Dec 6, 2019

Uh oh!

danliew-apple commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danliew-apple commented Dec 6, 2019

Uh oh!

Lukasa commented Dec 6, 2019

Uh oh!

danliew-apple commented Dec 7, 2019

Uh oh!

danliew-apple commented Dec 7, 2019

Uh oh!

Lukasa commented Dec 9, 2019

Uh oh!

Lukasa commented Dec 9, 2019

Uh oh!

Lukasa commented Dec 9, 2019

Uh oh!

Lukasa commented Dec 9, 2019

Uh oh!

mikeash commented Dec 9, 2019

Uh oh!

weissi commented Dec 9, 2019

Uh oh!

Lukasa commented Dec 9, 2019

Uh oh!

Lukasa commented Dec 9, 2019

Uh oh!

Lukasa commented Dec 9, 2019

Uh oh!

Lukasa commented Dec 9, 2019

Uh oh!

Lukasa commented Jan 17, 2020

Uh oh!

Lukasa commented Jan 17, 2020

Uh oh!

swift-ci commented Jan 17, 2020

Uh oh!

swift-ci commented Jan 17, 2020

Uh oh!

danliew-apple left a comment

Choose a reason for hiding this comment

Uh oh!

danliew-apple Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

Lukasa commented Jan 20, 2020

Uh oh!

Lukasa commented Jan 20, 2020

Uh oh!

Lukasa commented Jan 20, 2020

Uh oh!

swift-ci commented Jan 20, 2020

Uh oh!

swift-ci commented Jan 20, 2020

Uh oh!

danliew-apple commented Jan 24, 2020

Uh oh!

danliew-apple commented Jan 24, 2020

Uh oh!

danliew-apple commented Dec 6, 2019 •

edited

Loading