[CFI] Force LLVM to die if the implicit blacklist files cannot be found.

[cmtice]

Summary

Currently LLVM CFI tries to use an implicit blacklist file, currently in /usr/lib64/clang/<version>/share. If the file is not there, LLVM happily continues, which causes CFI to add checks to files/functions that are known to fail, generating binaries that fail. This CL causes LLVM to die (I hope) if it can't find these implicit blacklist files.

[lebedev.ri]

Test?

[lebedev.ri]

Also, please upload the correct patch, from the root of the repo, not from the directory of the file.

[vsk]

Toolchain vendors aren't currently required to provide default blacklists for every sanitizer clang supports. We don't ship a default ubsan blacklist on macOS, so this patch would break ubsan for all our users.

I think we need to find a different solution here. Have you considered making a blacklist mandatory just for CFI? Alternatively, if the blacklist is tiny and never changes, perhaps we could embed it within the compiler?

[cmtice]

Tried to upload the diff from the correct location.

[cmtice]

Ok, I'll work on making this CFI-specific and adding a test case.

[vsk]

In D46403#1086923, @cmtice wrote:

Ok, I'll work on making this CFI-specific and adding a test case.

Sounds good, thanks! I'd suggest modifying test/Driver/fsanitize-blacklist.c for your purposes. It should be possible to pair an empty resource directory with -fsanitize=cfi, and to check for a diagnostic.

[cmtice]

Updated the error to only occur for CFI blacklist, and added test case.

[vsk]

lib/Driver/SanitizerArgs.cpp

118

CFI can be enabled with other sanitizers, such as ubsan. I think you'll need 'Mask & CFI' here. It'd be great to have a test for this case, too (i.e -fsanitize=cfi,undefined -resource-dir=/dev/null should also give you this diagnostic).

test/Driver/fsanitize-blacklist.c

65

It'd be more helpful for readers if this comment were in the source, at the point where the diagnostic is emitted.

[vsk]

lib/Driver/SanitizerArgs.cpp

118

Sorry, I misread the surrounding code. Your check is fine as-is.

[vsk]

One problem with this direction is that clang doesn't ship a cfi blacklist in its default install, so, this leaves cfi users with stock toolchains to fend for themselves. I think it'd be a good idea to ship a basic cfi blacklist in clang's resource dir to avoid inadvertent breakage.

[cmtice]

Added comment to change in source code.

[cmtice]

vsk: Are you asking me to put together a cfi blacklist to ship in the resource directory in the default install as part of this code review? Or is that something you want to see in a different code reivew?

[pcc]

We should be installing compiler-rt/lib/cfi/cfi_blacklist.txt, no?

[vsk]

In D46403#1088759, @pcc wrote:

We should be installing compiler-rt/lib/cfi/cfi_blacklist.txt, no?

Oh, I see. This is already taken care of, then.

@cmtice This looks fine to me, but please wait for another +1 from someone more familiar with cfi.

[pcc]

LGTM

[cmtice]

Fix test failure that my previous changes caused.

[vsk]

Do you have commit access? If not I'd be happy to land this for you.

test/Driver/Inputs/resource_dir_with_cfi_blacklist/cfi_blacklist.txt

19 ↗

(On Diff #145524)

Would the test work if this were an empty file? If so, I'd suggest doing that for simplicity's sake.

[cmtice]

I'm not sure if I have commit access or not; Peter was working with me on trying to commit the change.

[pcc]

test/Frontend/dependency-gen.c

24

Shouldn't this be %S/../Driver/Inputs/resource_dir_with_cfi_blacklist?

[cmtice]

Make -resource-dir point to correct directory, in test case; move cfi_blacklist.txt file to 'share' subdirectory in test resource dir.

[cmtice]

Make cfi_blacklist.txt be an empty file.

[pcc]

LGTM

[filcab]

Sorry to ressurect this review, but I have a few questions:

• What kind of functions fail?
• Are there bugzillas to track these?
• How can a compiler expect to have blacklists for "all" its CFI clients? (I'm ok with having a default for "most-used, well-known, problematic functions", e.g: functions in libstdc++ or libc++)
• clang/compiler-rt don't seem to have a default blacklist, what should the contents be? This patch seems to be saying "There are some well-known functions that should never be instrumented with CFI, I'll provide a list of names", which doesn't really seem possible to do in general, for "most" CFI-toolchain clients. As I see it, each client will need to have their own blacklist, and provide it as an argument if needed. Which brings us to:
• If I pass -fsanitize-blacklist=my_blacklist.txt I still get an error. This is not ideal, as I might be working on the blacklist to include in my toolchain/program.

I don't think we should have an error that is default enabled for this issue. At most a warning (+ -Werror) if there was no blacklist passed in nor found in the toolchain resource directory.

Thank you,
Filipe

P.S: Sorry for only noticing this so much later.

[pcc]

In D46403#1291697, @filcab wrote:

Sorry to ressurect this review, but I have a few questions:

• What kind of functions fail?
• Are there bugzillas to track these?
• How can a compiler expect to have blacklists for "all" its CFI clients? (I'm ok with having a default for "most-used, well-known, problematic functions", e.g: functions in libstdc++ or libc++)

The default blacklist should contain problematic functions in the stdlibs that we don't control (i.e. libstdc++ and the MSVC stdlib). In libc++, we blacklist functions directly in the source code using the no_sanitize attribute. In general the blacklist entries for the stdlib are necessary because the standard requires that functions have a particular signature which requires the implementation to make what is considered to be a bad cast by CFI, so there isn't a bug as such.

• clang/compiler-rt don't seem to have a default blacklist, what should the contents be?

There is a default blacklist, see http://llvm-cs.pcc.me.uk/projects/compiler-rt/lib/cfi/cfi_blacklist.txt
Its contents are as mentioned above.

It should be installed by default, is that not happening for you?

This patch seems to be saying "There are some well-known functions that should never be instrumented with CFI, I'll provide a list of names", which doesn't really seem possible to do in general, for "most" CFI-toolchain clients. As I see it, each client will need to have their own blacklist, and provide it as an argument if needed.

Right, if the client needs a blacklist it should be passed on the command line. More importantly, it should be applied in addition to the standard one so that the user doesn't have to repeat the stdlib blacklist in their blacklist (which was the situation before we added support for multiple blacklists).

Which brings us to:

• If I pass -fsanitize-blacklist=my_blacklist.txt I still get an error. This is not ideal, as I might be working on the blacklist to include in my toolchain/program.

I don't think we should have an error that is default enabled for this issue. At most a warning (+ -Werror) if there was no blacklist passed in nor found in the toolchain resource directory.

Clang prints this error if the blacklist is missing from the resource directory, which means that Clang was not installed properly. This is intentional as it prevents us from silently not applying the standard blacklist in that case.

If you are working on the toolchain blacklist, that is a very special case and there are plenty of ways around this issue. For example you can edit lib/cfi/cfi_blacklist.txt in place and use ninja to install it.

lib/Driver/SanitizerArgs.cpp

static void addDefaultBlacklists(const Driver &D, SanitizerMask Kinds,
   for (auto BL : Blacklists) {
     if (!(Kinds & BL.Mask))
       continue;
 
     clang::SmallString<64> Path(D.ResourceDir);
     llvm::sys::path::append(Path, "share", BL.File);
     if (llvm::sys::fs::exists(Path))
       BlacklistFiles.push_back(Path.str());
+    else if (BL.Mask == 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;
   }
 }
 
 /// 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)                                       \

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
 
+// 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
+// CHECK-MISSING-CFI-BLACKLIST: error: no such file or directory: '{{.*}}/share/cfi_blacklist.txt'
+
 // DELIMITERS: {{^ *"}}

test/Frontend/dependency-gen.c

 // RUN: %clang -MD -MF - %s -fsyntax-only -I .//./a/b/ | FileCheck -check-prefix=CHECK-FOUR %s
 // CHECK-FOUR: {{ }}a/b{{[/\\]}}x.h
 // RUN: %clang -MD -MF - %s -fsyntax-only -I a/b/. | FileCheck -check-prefix=CHECK-FIVE %s
 // CHECK-FIVE: {{ }}a/b/.{{[/\\]}}x.h
 // RUN: cd a/b
 // RUN: %clang -MD -MF - %s -fsyntax-only -I ./ | FileCheck -check-prefix=CHECK-SIX %s
 // CHECK-SIX: {{ }}x.h
 // RUN: echo "fun:foo" > %t.blacklist
-// RUN: %clang -MD -MF - %s -fsyntax-only -fsanitize=cfi-vcall -flto -fvisibility=hidden -fsanitize-blacklist=%t.blacklist -I ./ | FileCheck -check-prefix=CHECK-SEVEN %s
+// RUN: %clang -MD -MF - %s -fsyntax-only -resource-dir=%S/../Driver/Inputs/resource_dir_with_cfi_blacklist -fsanitize=cfi-vcall -flto -fvisibility=hidden -fsanitize-blacklist=%t.blacklist -I ./ | FileCheck -check-prefix=CHECK-SEVEN %s
 // CHECK-SEVEN: .blacklist
 // CHECK-SEVEN: {{ }}x.h
 #ifndef INCLUDE_FLAG_TEST
 #include <x.h>
 #endif