This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Skip validation of system sanitizer blacklists files if -fno-sanitizer-blacklist was specified
AcceptedPublic

Authored by saudi on Apr 28 2020, 2:07 PM.

Details

Summary

This is to avoid checking for the validity of a file that is not used.

We have some scenarios where we create a test build of a game, with CFI sanitizer activated but blacklist disabled.
Our build system uses FastBuild : it allows distributed compilation, by sending to remote workers the compiler executable + command line + preprocessed cpp.

Without this fix, the remote build fails as the remote build doesn't have a resource-dir structure with the default blacklist file.

This also contains a minor fix for the test, as the cfi sanitizer requires -flto and -fvisibility= arguments.

Diff Detail

Event Timeline

saudi created this revision.Apr 28 2020, 2:07 PM
saudi edited the summary of this revision. (Show Details)Apr 28 2020, 2:14 PM
saudi added a subscriber: aganea.Apr 28 2020, 2:17 PM

This makes sense to me but as it has potential to break all sorts of stuff I'd like sanitizer folks to see it before it lands.

clang/test/Driver/fsanitize-blacklist.c
75

How do you feel about adding a test that driver doesn't pass -fsanitize-system-blacklist option to cc1?
IIUC your test checks that driver doesn't try to parse it but in theory it might still pass it.

Hmm, one more thought - maybe we should add a new driver option (fno_system_sanitize_blacklist?) as otherwise we are getting kind of confusing CLI with combination of -fno-sanitize-blacklist and -fsanitize-blacklist.

Imagine:

clang -fsanitize=cfi -fno-sanitize-blacklist -fsanitize-blacklist=/tmp/blacklist.txt foo.c

It feels kind of unintuitive.

What do you think?

clang -fsanitize=cfi -fno-sanitize-blacklist -fsanitize-blacklist=/tmp/blacklist.txt foo.c

In that case, the code should be written in a way such as only the last option is taken into account. And vice-versa: -fsanitize-blacklist=/tmp/blacklist.txt -fno-sanitize-blacklist should disable the blacklist. This seems to be the norm across Clang & LLD. @jkorous WDYT?

saudi added a comment.Apr 29 2020, 7:54 AM

Hmm, one more thought - maybe we should add a new driver option (fno_system_sanitize_blacklist?) as otherwise we are getting kind of confusing CLI with combination of -fno-sanitize-blacklist and -fsanitize-blacklist.

Imagine:

clang -fsanitize=cfi -fno-sanitize-blacklist -fsanitize-blacklist=/tmp/blacklist.txt foo.c

It feels kind of unintuitive.

What do you think?

I agree, in the scenario where we need to compile using a custom blacklist file and ignore the system ones, we would end up with such confusing CLI configuration.
However the test file contains several examples, indicating that it is by design:
// -fno-sanitize-blacklist disables all blacklists specified earlier.
// Flag -fno-sanitize-blacklist wins if it is specified later.

I'll prepare a patch update to add -fno-system-sanitize-blacklist, as it would allow to make more explicit command lines.

saudi updated this revision to Diff 260942.Apr 29 2020, 9:31 AM

Update for suggestions in comments:

  • added -fno-system-sanitize-blacklist driver argument
  • added test for system blacklist files deactivation in the cc1 command line
saudi marked an inline comment as done.Apr 29 2020, 9:33 AM

Are you sure you want the remote build to succeed without the CFI system blacklist?

The diagnostic was added in https://reviews.llvm.org/D46403 because broken binaries were being built due to lack of the system blacklist.

saudi added a comment.EditedApr 29 2020, 1:49 PM

Are you sure you want the remote build to succeed without the CFI system blacklist?

The diagnostic was added in https://reviews.llvm.org/D46403 because broken binaries were being built due to lack of the system blacklist.

We intended to use the sanitizer in a testing environment where CFI would be activated but fully recoverable (args -fsanitize=cfi -fno-sanitize-trap=all -fsanitize-recover=all -fno-sanitize-blacklist).
This way, a developper can make a CFI build, run the game and check the logs for messages.

This workflow is at its first step, it will probably evolve and include blacklists at some point.
Note that we use -fno-sanitize-blacklist, which removes the default blacklist file even though its existence is currently checked.

morehouse added inline comments.Apr 29 2020, 4:52 PM
clang/include/clang/Driver/Options.td
1018

I think this flag is unnecessary considering -fsanitize-system-blacklist is only used by the frontend, not the clang driver.

saudi added inline comments.Apr 30 2020, 6:45 AM
clang/include/clang/Driver/Options.td
1018

This flag was intended for better command line readability if we want to disable the system blacklists but still use custom ones.
In that case we would need to mix -fno-sanitize-blacklist and -fsanitize-blacklist= arguments, which doesn't clearly show that the purpose is to deactivate the system ones.

I agree, after reading your previous comment about the dangerosity of using the sanitizer without its system blacklist, that adding this -fno-system-sanitize-blacklist could be advertising for dangerous uses.

Should-I remove it?

morehouse added inline comments.Apr 30 2020, 7:59 AM
clang/include/clang/Driver/Options.td
1018

I think the expectation is that -fno-sanitize-blacklist removes the system blacklist and any other blacklists specified in the command line prior. It seems intuitive for users that -fno-sanitize-blacklist means this, and that users then need to specify fsanitize-blacklist=... to add blacklists back.

Regardless of whether it is "dangerous" to use CFI without a blacklist, I think we should make -fno-sanitize-blacklist consistent in this case, since the flag is explicitly asking not to use the blacklist. Note that the diagnostic was originally added for a slightly different scenario: user did not specify -fno-sanitize-blacklist but the system blacklist file was missing. I think it makes sense for -fno-sanitize-blacklist to override this diagnostic.

About -fno-system-sanitize-blacklist, I think it is unnecessary since its functionality is already achieved through -fno-sanitize-blacklist, and I prefer not to add superfluous flags, and because it has no antonym flag that can be used from the clang driver to undo its effects.

saudi updated this revision to Diff 261249.Apr 30 2020, 8:55 AM

Removed -fno-system-blacklist argument, as suggested in comment.

This revision is now accepted and ready to land.Apr 30 2020, 9:17 AM
This revision was automatically updated to reflect the committed changes.

This patch is relatively recent. I'd like to revert this for D96203
Can we user just pointo to /dev/null if file is not needed?

This revision is now accepted and ready to land.Feb 12 2021, 11:50 AM

This patch is relatively recent. I'd like to revert this for D96203
Can we user just pointo to /dev/null if file is not needed?

You mean just use -fno-sanitize-file=/dev/null as you're proposing in https://reviews.llvm.org/D96203#2560711 ? As long as there's a way to prevent searching for default blocklists on disk, to cover the case in https://reviews.llvm.org/D79043#2011084 fine with me, you can revert.

This patch is relatively recent. I'd like to revert this for D96203
Can we user just pointo to /dev/null if file is not needed?

You mean just use -fno-sanitize-file=/dev/null as you're proposing in https://reviews.llvm.org/D96203#2560711 ? As long as there's a way to prevent searching for default blocklists on disk, to cover the case in https://reviews.llvm.org/D79043#2011084 fine with me, you can revert.

Yes, with some new test for such behavior.