[cfi] Fix handling of sanitize trap/recover flags in the cross-DSO CFI mode.

[eugenis]

Summary

In the cross-DSO CFI mode clang emits cfi_check_fail that handles
errors triggered from other modules with targets in the current
module. With this change, cfi_check_fail will handle errors for
CFI kinds that are not enabled in the current module as if they
have the trapping behaviour (-fsanitize-trap=...).

This fixes a bug where some combinations of -fsanitize* flags may
result in a link failure due to a missing sanitizer runtime library
for the diagnostic calls in __cfi_check_fail.

[pcc]

Why can't we make it so that a trap flag doesn't affect the behaviour if a sanitizer is disabled (this looks like what CodeGenModule::NeedAllVtablesBitSet is already doing?)

[eugenis]

This lets us support the following case:
module A checks vcalls and casts, with diagnostics
module B checks vcalls but not casts (but it still has bitsets for vtables), with diagnostics
then a cast check from module A with a target in module B should print diagnostics instead of trapping

It's definitely not critical functionality, but could be nice to have, especially as it does not cost us anything.

And yes, CodeGenModule::NeedAllVtablesBitSet needs to be fixed for this work.

WDYT?

[pcc]

What I meant was that it looks like a hack that this is being handled in the driver. The frontend shouldn't care what the value of a trap flag is if a sanitizer is disabled.

Why are we even emitting checks for disabled sanitizers in the target DSO anyway? Can we fail open if the target DSO does not have a particular CFI sanitizer enabled (same as we fail open for non-instrumented DSOs)? That would allow you to remove a little of the complication you added to EmitCheck in D15699.

[eugenis]

OK, done. Please take another look.
This is inferior to the original patch in terms of functionality, but the implementation is a lot simpler.

[eugenis]

ping

[pcc]

LGTM

lib/CodeGen/CGExpr.cpp

2484–2485 ↗

(On Diff #49115)

You can remove this comment now.

2677 ↗

(On Diff #49115)

I mentioned failing open, but yes, it's probably better to fail closed.

[eugenis]

r263180, thanks for the review!

[eugenis]

No, this is not committed.
I've run dcommit in the wrong checkout and landed D17900 instead.

[eugenis]

r263578, finally

lib/Driver/SanitizerArgs.cpp

bool SanitizerArgs::needsUbsanRt() const {
   return (Sanitizers.Mask & NeedsUbsanRt & ~TrapSanitizers.Mask) &&
          !Sanitizers.has(Address) &&
          !Sanitizers.has(Memory) &&
          !Sanitizers.has(Thread) &&
          !CfiCrossDso;
 }
 
 bool SanitizerArgs::needsCfiRt() const {
-  return !(Sanitizers.Mask & CFI & ~TrapSanitizers.Mask) && CfiCrossDso;
+  return !(CFI & ~TrapSanitizers.Mask) && CfiCrossDso;
 }
 
 bool SanitizerArgs::needsCfiDiagRt() const {
-  return (Sanitizers.Mask & CFI & ~TrapSanitizers.Mask) && CfiCrossDso;
+  return (CFI & ~TrapSanitizers.Mask) && CfiCrossDso;
 }
 
 bool SanitizerArgs::requiresPIE() const {
   return NeedPIE || (Sanitizers.Mask & RequiresPIE);
 }
 
 bool SanitizerArgs::needsUnwindTables() const {
   return Sanitizers.Mask & NeedsUnwindTables;
if (Arg->getOption().matches(options::OPT_fsanitize_recover)) {
       RecoverableKinds &= ~expandSanitizerGroups(parseArgValues(D, Arg, true));
       Arg->claim();
     }
     if (DeprecatedReplacement) {
       D.Diag(diag::warn_drv_deprecated_arg) << Arg->getAsString(Args)
                                             << DeprecatedReplacement;
     }
   }
-  RecoverableKinds &= Kinds;
-  RecoverableKinds &= ~Unrecoverable;
 
+  if (AllAddedKinds & CFI) {
+    CfiCrossDso = Args.hasFlag(options::OPT_fsanitize_cfi_cross_dso,
+                               options::OPT_fno_sanitize_cfi_cross_dso, false);
+    // Without PIE, external function address may resolve to a PLT record, which
+    // can not be verified by the target module.
+    NeedPIE |= CfiCrossDso;
+  }
+
+  // In the cross-DSO CFI mode a module may be asked to handle any type of CFI
+  // error, even if the module itself is not checked for that type of error.
+  // Therefore, trap/recover flags should be preserved all CFI checkers.
+  if (CfiCrossDso) {
+    TrappingKinds &= (Kinds | CFI);
+    RecoverableKinds &= (Kinds | CFI);
+  } else {
     TrappingKinds &= Kinds;
+    RecoverableKinds &= Kinds;
+  }
+
+  RecoverableKinds &= ~Unrecoverable;
 
   // Setup blacklist files.
   // Add default blacklist from resource directory.
   {
     std::string BLPath;
     if (getDefaultBlacklist(D, Kinds, BLPath) && llvm::sys::fs::exists(BLPath))
       BlacklistFiles.push_back(BLPath);
   }
if (Arg *A =
       }
     }
     MsanUseAfterDtor =
         Args.hasArg(options::OPT_fsanitize_memory_use_after_dtor);
     NeedPIE |= !(TC.getTriple().isOSLinux() &&
                  TC.getTriple().getArch() == llvm::Triple::x86_64);
   }
 
-  if (AllAddedKinds & CFI) {
-    CfiCrossDso = Args.hasFlag(options::OPT_fsanitize_cfi_cross_dso,
-                               options::OPT_fno_sanitize_cfi_cross_dso, false);
-    // Without PIE, external function address may resolve to a PLT record, which
-    // can not be verified by the target module.
-    NeedPIE |= CfiCrossDso;
-  }
-
   Stats = Args.hasFlag(options::OPT_fsanitize_stats,
                        options::OPT_fno_sanitize_stats, false);
 
   // Parse -f(no-)?sanitize-coverage flags if coverage is supported by the
   // enabled sanitizers.
   if (AllAddedKinds & SupportsCoverage) {
     for (const auto *Arg : Args) {
       if (Arg->getOption().matches(options::OPT_fsanitize_coverage)) {

test/Driver/fsanitize.c

 // RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi -flto -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-NO-CROSS-DSO
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi -fsanitize-cfi-cross-dso -fno-sanitize-cfi-cross-dso -flto -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-NO-CROSS-DSO
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi -fno-sanitize-cfi-cross-dso -fsanitize-cfi-cross-dso -flto -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-CROSS-DSO
 // CHECK-CFI-CROSS-DSO: -emit-llvm-bc
 // CHECK-CFI-CROSS-DSO: -fsanitize-cfi-cross-dso
 // CHECK-CFI-NO-CROSS-DSO: -emit-llvm-bc
 // CHECK-CFI-NO-CROSS-DSO-NOT: -fsanitize-cfi-cross-dso
 
+// In the non-cross-dso CFI mode, -fsanitize-trap only appears for enabled CFI checkers.
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-vcall -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-VCALL-TRAP
+// CHECK-CFI-VCALL-TRAP: "-fsanitize=cfi-vcall" "-fsanitize-trap=cfi-vcall"
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-vcall -fno-sanitize-trap=cfi-vcall -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-VCALL-NO-TRAP
+// CHECK-CFI-VCALL-NO-TRAP: "-fsanitize=cfi-vcall"
+// CHECK-CFI-VCALL-NO-TRAP-NOT: -fsanitize-trap=
+
+// In the cross-dso CFI mode, -fsanitize-trap appears for all CFI checkers.
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-vcall -fsanitize-cfi-cross-dso -flto -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-CROSS-DSO-VCALL-TRAP
+// CHECK-CFI-CROSS-DSO-VCALL-TRAP: "-fsanitize=cfi-vcall" "-fsanitize-trap=cfi-derived-cast,cfi-icall,cfi-unrelated-cast,cfi-nvcall,cfi-vcall"
+
+// RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi-vcall -fno-sanitize-trap=cfi-vcall -fsanitize-cfi-cross-dso -flto -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-CROSS-DSO-VCALL-NO-TRAP
+// CHECK-CFI-CROSS-DSO-VCALL-NO-TRAP: "-fsanitize=cfi-vcall" "-fsanitize-trap=cfi-derived-cast,cfi-icall,cfi-unrelated-cast,cfi-nvcall"
+
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=cfi -fsanitize-stats -flto -c %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-CFI-STATS
 // CHECK-CFI-STATS: -fsanitize-stats
 
 // RUN: %clang_cl -fsanitize=address -c -MDd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
 // RUN: %clang_cl -fsanitize=address -c -MTd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
 // RUN: %clang_cl -fsanitize=address -c -LDd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
 // RUN: %clang_cl -fsanitize=address -c -MD -MDd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL
 // RUN: %clang_cl -fsanitize=address -c -MT -MTd -### -- %s 2>&1 | FileCheck %s -check-prefix=CHECK-ASAN-DEBUGRTL