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

[saudi]

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.

[jkorous]

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

73

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.

[jkorous]

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?

[aganea]

In D79043#2009225, @jkorous wrote:

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]

In D79043#2009225, @jkorous wrote:

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]

Update for suggestions in comments:

• added -fno-system-sanitize-blacklist driver argument
• added test for system blacklist files deactivation in the cc1 command line

[morehouse]

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]

In D79043#2010908, @morehouse wrote:

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]

clang/include/clang/Driver/Options.td

1018 ↗

(On Diff #260942)

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

[saudi]

clang/include/clang/Driver/Options.td

1018 ↗

(On Diff #260942)

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]

clang/include/clang/Driver/Options.td

1018 ↗

(On Diff #260942)

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]

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

[morehouse]

LGTM

[vitalybuka]

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?

[vitalybuka]

Reopen to attract attention

[aganea]

In D79043#2560710, @vitalybuka wrote:

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.

[vitalybuka]

In D79043#2560867, @aganea wrote:

In D79043#2560710, @vitalybuka wrote:

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.

clang/lib/Driver/SanitizerArgs.cpp

 /// "-fsanitize=alignment".
 static std::string describeSanitizeArg(const llvm::opt::Arg *A,
                                        SanitizerMask Mask);
 
 /// Produce a string containing comma-separated names of sanitizers in \p
 /// Sanitizers set.
 static std::string toString(const clang::SanitizerSet &Sanitizers);
 
+static void validateSpecialCaseListFormat(const Driver &D,
+                                          std::vector<std::string> &SCLFiles,
+                                          unsigned MalformedSCLErrorDiagID) {
+  if (SCLFiles.empty())
+    return;
+
+  std::string BLError;
+  std::unique_ptr<llvm::SpecialCaseList> SCL(
+      llvm::SpecialCaseList::create(SCLFiles, D.getVFS(), BLError));
+  if (!SCL.get())
+    D.Diag(MalformedSCLErrorDiagID) << BLError;
+}
+
 static void addDefaultBlacklists(const Driver &D, SanitizerMask Kinds,
                                  std::vector<std::string> &BlacklistFiles) {
   struct Blacklist {
     const char *File;
     SanitizerMask Mask;
   } Blacklists[] = {{"asan_blacklist.txt", SanitizerKind::Address},
                     {"hwasan_blacklist.txt", SanitizerKind::HWAddress},
                     {"memtag_blacklist.txt", SanitizerKind::MemTag},
for (auto BL : Blacklists) {
     llvm::sys::path::append(Path, "share", BL.File);
     if (D.getVFS().exists(Path))
       BlacklistFiles.push_back(std::string(Path.str()));
     else if (BL.Mask == SanitizerKind::CFI)
       // If cfi_blacklist.txt cannot be found in the resource dir, driver
       // should fail.
       D.Diag(clang::diag::err_drv_no_such_file) << Path;
   }
+  validateSpecialCaseListFormat(
+      D, BlacklistFiles, clang::diag::err_drv_malformed_sanitizer_blacklist);
 }
 
 /// Parse -f(no-)?sanitize-(coverage-)?(white|black)list argument's values,
 /// diagnosing any invalid file paths and validating special case list format.
 static void parseSpecialCaseListArg(const Driver &D,
                                     const llvm::opt::ArgList &Args,
                                     std::vector<std::string> &SCLFiles,
                                     llvm::opt::OptSpecifier SCLOptionID,
if (Arg->getOption().matches(SCLOptionID)) {
         D.Diag(clang::diag::err_drv_no_such_file) << SCLPath;
       }
       // Match -fno-sanitize-blacklist.
     } else if (Arg->getOption().matches(NoSCLOptionID)) {
       Arg->claim();
       SCLFiles.clear();
     }
   }
-  // Validate special case list format.
-  {
-    std::string BLError;
-    std::unique_ptr<llvm::SpecialCaseList> SCL(
-        llvm::SpecialCaseList::create(SCLFiles, D.getVFS(), BLError));
-    if (!SCL.get())
-      D.Diag(MalformedSCLErrorDiagID) << BLError;
-  }
+  validateSpecialCaseListFormat(D, SCLFiles, MalformedSCLErrorDiagID);
 }
 
 /// Sets group bits for every group that has at least one representative already
 /// enabled in \p Kinds.
 static SanitizerMask setGroupBits(SanitizerMask Kinds) {
 #define SANITIZER(NAME, ID)
 #define SANITIZER_GROUP(NAME, ID, ALIAS)                                       \
   if (Kinds & SanitizerKind::ID)                                               \
SanitizerArgs::SanitizerArgs(const ToolChain &TC,
   }
   RecoverableKinds &= Kinds;
   RecoverableKinds &= ~Unrecoverable;
 
   TrappingKinds &= Kinds;
   RecoverableKinds &= ~TrappingKinds;
 
   // Setup blacklist files.
-  // Add default blacklist from resource directory.
+  // Add default blacklist from resource directory for activated sanitizers, and
+  // validate special case lists format.
+  if (!Args.hasArgNoClaim(options::OPT_fno_sanitize_blacklist))
     addDefaultBlacklists(D, Kinds, SystemBlacklistFiles);
 
   // Parse -f(no-)?sanitize-blacklist options.
   // This also validates special case lists format.
-  // Here, OptSpecifier() acts as a never-matching command-line argument.
-  // So, there is no way to append to system blacklist but it can be cleared.
-  parseSpecialCaseListArg(D, Args, SystemBlacklistFiles, OptSpecifier(),
-                          options::OPT_fno_sanitize_blacklist,
-                          clang::diag::err_drv_malformed_sanitizer_blacklist);
   parseSpecialCaseListArg(D, Args, UserBlacklistFiles,
                           options::OPT_fsanitize_blacklist,
                           options::OPT_fno_sanitize_blacklist,
                           clang::diag::err_drv_malformed_sanitizer_blacklist);
 
   // Parse -f[no-]sanitize-memory-track-origins[=level] options.
   if (AllAddedKinds & SanitizerKind::Memory) {
     if (Arg *A =

clang/test/Driver/fsanitize-blacklist.c

 // CHECK-BAD-BLACKLIST: error: malformed sanitizer blacklist: 'error parsing file '{{.*}}.bad': malformed line 1: 'badline''
 
 // -fno-sanitize-blacklist disables all blacklists specified earlier.
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fsanitize-blacklist=%t.good -fno-sanitize-blacklist -fsanitize-blacklist=%t.second %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-ONLY-FIRST-DISABLED --implicit-check-not=-fsanitize-blacklist=
 // CHECK-ONLY_FIRST-DISABLED-NOT: good
 // CHECK-ONLY-FIRST-DISABLED: -fsanitize-blacklist={{.*}}.second
 // CHECK-ONLY_FIRST-DISABLED-NOT: good
 
+// -fno-sanitize-blacklist disables the system blacklists.
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=address -fno-sanitize-blacklist %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-DISABLED-SYSTEM --check-prefix=DELIMITERS
+// CHECK-DISABLED-SYSTEM-NOT: -fsanitize-system-blacklist
+
 // If cfi_blacklist.txt cannot be found in the resource dir, driver should fail.
-// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi -resource-dir=/dev/null %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MISSING-CFI-BLACKLIST
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi -flto -fvisibility=default -resource-dir=/dev/null %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MISSING-CFI-BLACKLIST
 // CHECK-MISSING-CFI-BLACKLIST: error: no such file or directory: '{{.*}}cfi_blacklist.txt'
 
+// -fno-sanitize-blacklist disables checking for cfi_blacklist.txt in the resource dir.
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi -flto -fvisibility=default -fno-sanitize-blacklist -resource-dir=/dev/null %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-MISSING-CFI-NO-BLACKLIST
+// CHECK-MISSING-CFI-NO-BLACKLIST-NOT: error: no such file or directory: '{{.*}}cfi_blacklist.txt'
+
 // DELIMITERS: {{^ *"}}