sanmd: improve precision of UAR analysis

dvyukov · dvyukov · commit 5addb736a9a8 · 2022-12-12T11:41:59.000+01:00
Only mark functions that have address-taken locals as requiring UAR checking. On a large internal app this reduces number of marked functions from 78441 to 66618. Mostly small, trivial getter/setter-type functions are unmarked, but also some amount of larger number-crunching-type functions are unmarked as well. Reviewed By: melver Differential Revision: https://reviews.llvm.org/D139811
diff --git a/compiler-rt/test/metadata/covered.cpp b/compiler-rt/test/metadata/covered.cpp
@@ -7,6 +7,11 @@
 // RUN: %clangxx %s -o %t -fexperimental-sanitize-metadata=atomics,uar && %t | FileCheck -check-prefix=CHECK-AU %s
 // RUN: %clangxx %s -o %t -fexperimental-sanitize-metadata=covered,atomics,uar && %t | FileCheck -check-prefix=CHECK-CAU %s
 
+__attribute__((noinline, not_tail_called)) void escape(const volatile void *p) {
+  static const volatile void *sink;
+  sink = p;
+}
+
 // CHECK-NOT: metadata add
 // CHECK: main
 // CHECK-NOT: metadata del
@@ -28,19 +33,31 @@ void empty() {}
 // CHECK-AU: normal: features=3
 // CHECK-CAU:normal: features=3
 void normal() {
-  volatile int x;
-  x = 0;
+  int x;
+  escape(&x);
 }
 
-// CHECK-C:   with_atomic: features=0
-// CHECK-A:   with_atomic: features=1
-// CHECK-U:   with_atomic: features=2
-// CHECK-CA:  with_atomic: features=1
-// CHECK-CU:  with_atomic: features=2
-// CHECK-AU:  with_atomic: features=3
-// CHECK-CAU: with_atomic: features=3
+// CHECK-C:     with_atomic: features=0
+// CHECK-A:     with_atomic: features=1
+// CHECK-U-NOT: with_atomic:
+// CHECK-CA:    with_atomic: features=1
+// CHECK-CU:    with_atomic: features=0
+// CHECK-AU:    with_atomic: features=1
+// CHECK-CAU:   with_atomic: features=1
 int with_atomic(int *p) { return __atomic_load_n(p, __ATOMIC_RELAXED); }
 
+// CHECK-C:   with_atomic_escape: features=0
+// CHECK-A:   with_atomic_escape: features=1
+// CHECK-U:   with_atomic_escape: features=2
+// CHECK-CA:  with_atomic_escape: features=1
+// CHECK-CU:  with_atomic_escape: features=2
+// CHECK-AU:  with_atomic_escape: features=3
+// CHECK-CAU: with_atomic_escape: features=3
+int with_atomic_escape(int *p) {
+  escape(&p);
+  return __atomic_load_n(p, __ATOMIC_RELAXED);
+}
+
 // CHECK-C:     ellipsis: features=0
 // CHECK-A:     ellipsis: features=1
 // CHECK-U-NOT: ellipsis:
@@ -49,6 +66,7 @@ int with_atomic(int *p) { return __atomic_load_n(p, __ATOMIC_RELAXED); }
 // CHECK-AU:    ellipsis: features=1
 // CHECK-CAU:   ellipsis: features=1
 void ellipsis(int *p, ...) {
+  escape(&p);
   volatile int x;
   x = 0;
 }
@@ -61,13 +79,15 @@ void ellipsis(int *p, ...) {
 // CHECK-AU:    ellipsis_with_atomic: features=1
 // CHECK-CAU:   ellipsis_with_atomic: features=1
 int ellipsis_with_atomic(int *p, ...) {
+  escape(&p);
   return __atomic_load_n(p, __ATOMIC_RELAXED);
 }
 
 #define FUNCTIONS                                                              \
   FN(empty);                                                                   \
   FN(normal);                                                                  \
   FN(with_atomic);                                                             \
+  FN(with_atomic_escape);                                                      \
   FN(ellipsis);                                                                \
   FN(ellipsis_with_atomic);                                                    \
   /**/
diff --git a/compiler-rt/test/metadata/uar.cpp b/compiler-rt/test/metadata/uar.cpp
@@ -1,49 +1,82 @@
-// RUN: %clangxx %s -o %t -fexperimental-sanitize-metadata=covered,uar && %t | FileCheck %s
+// RUN: %clangxx %s -O1 -o %t -fexperimental-sanitize-metadata=covered,uar && %t | FileCheck %s
 
 // CHECK: metadata add version 1
 
+__attribute__((noinline, not_tail_called)) void escape(const volatile void *p) {
+  static const volatile void *sink;
+  sink = p;
+}
+
+__attribute__((noinline, not_tail_called)) void use(int x) {
+  static volatile int sink;
+  sink += x;
+}
+
 // CHECK: empty: features=0 stack_args=0
 void empty() {}
 
 // CHECK: ellipsis: features=0 stack_args=0
 void ellipsis(const char *fmt, ...) {
-  volatile int x;
-  x = 1;
+  int x;
+  escape(&x);
 }
 
 // CHECK: non_empty_function: features=2 stack_args=0
 void non_empty_function() {
-  // Completely empty functions don't get uar metadata.
-  volatile int x;
-  x = 1;
+  int x;
+  escape(&x);
 }
 
 // CHECK: no_stack_args: features=2 stack_args=0
 void no_stack_args(long a0, long a1, long a2, long a3, long a4, long a5) {
-  volatile int x;
-  x = 1;
+  int x;
+  escape(&x);
 }
 
 // CHECK: stack_args: features=2 stack_args=16
 void stack_args(long a0, long a1, long a2, long a3, long a4, long a5, long a6) {
-  volatile int x;
-  x = 1;
+  int x;
+  escape(&x);
 }
 
 // CHECK: more_stack_args: features=2 stack_args=32
 void more_stack_args(long a0, long a1, long a2, long a3, long a4, long a5,
                      long a6, long a7, long a8) {
-  volatile int x;
-  x = 1;
+  int x;
+  escape(&x);
 }
 
 // CHECK: struct_stack_args: features=2 stack_args=144
 struct large {
   char x[131];
 };
 void struct_stack_args(large a) {
-  volatile int x;
-  x = 1;
+  int x;
+  escape(&x);
+}
+
+__attribute__((noinline)) int tail_called(int x) { return x; }
+
+// CHECK: with_tail_call: features=2
+int with_tail_call(int x) { [[clang::musttail]] return tail_called(x); }
+
+// CHECK: local_array: features=0
+void local_array(int x) {
+  int data[10];
+  use(data[x]);
+}
+
+// CHECK: local_alloca: features=0
+void local_alloca(int size, int i, int j) {
+  volatile int *p = static_cast<int *>(__builtin_alloca(size));
+  p[i] = 0;
+  use(p[j]);
+}
+
+// CHECK: escaping_alloca: features=2
+void escaping_alloca(int size, int i) {
+  volatile int *p = static_cast<int *>(__builtin_alloca(size));
+  escape(&p[i]);
 }
 
 #define FUNCTIONS                                                              \
@@ -54,6 +87,10 @@ void struct_stack_args(large a) {
   FN(stack_args);                                                              \
   FN(more_stack_args);                                                         \
   FN(struct_stack_args);                                                       \
+  FN(with_tail_call);                                                          \
+  FN(local_array);                                                             \
+  FN(local_alloca);                                                            \
+  FN(escaping_alloca);                                                         \
   /**/
 
 #include "common.h"
diff --git a/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp b/llvm/lib/Transforms/Instrumentation/SanitizerBinaryMetadata.cpp
@@ -250,8 +250,10 @@ void SanitizerBinaryMetadata::runOn(Function &F, MetadataInfoSet &MIS) {
 
   if (F.isVarArg())
     FeatureMask &= ~kSanitizerBinaryMetadataUAR;
-  if (FeatureMask & kSanitizerBinaryMetadataUAR)
+  if (FeatureMask & kSanitizerBinaryMetadataUAR) {
+    RequiresCovered = true;
     NumMetadataUAR++;
+  }
 
   // Covered metadata is always emitted if explicitly requested, otherwise only
   // if some other metadata requires it to unambiguously interpret it for
@@ -269,23 +271,50 @@ void SanitizerBinaryMetadata::runOn(Function &F, MetadataInfoSet &MIS) {
   }
 }
 
+bool hasUseAfterReturnUnsafeUses(Value &V) {
+  for (User *U : V.users()) {
+    if (auto *I = dyn_cast<Instruction>(U)) {
+      if (I->isLifetimeStartOrEnd() || I->isDroppable())
+        continue;
+      if (isa<LoadInst>(U))
+        continue;
+      if (auto *SI = dyn_cast<StoreInst>(U)) {
+        // If storing TO the alloca, then the address isn't taken.
+        if (SI->getOperand(1) == &V)
+          continue;
+      }
+      if (auto *GEPI = dyn_cast<GetElementPtrInst>(U)) {
+        if (!hasUseAfterReturnUnsafeUses(*GEPI))
+          continue;
+      } else if (auto *BCI = dyn_cast<BitCastInst>(U)) {
+        if (!hasUseAfterReturnUnsafeUses(*BCI))
+          continue;
+      }
+    }
+    return true;
+  }
+  return false;
+}
+
+bool useAfterReturnUnsafe(Instruction &I) {
+  if (isa<AllocaInst>(I))
+    return hasUseAfterReturnUnsafeUses(I);
+  // Tail-called functions are not necessary intercepted
+  // at runtime because there is no call instruction.
+  // So conservatively mark the caller as requiring checking.
+  else if (auto *CI = dyn_cast<CallInst>(&I))
+    return CI->isTailCall();
+  return false;
+}
+
 bool SanitizerBinaryMetadata::runOn(Instruction &I, MetadataInfoSet &MIS,
                                     MDBuilder &MDB, uint32_t &FeatureMask) {
   SmallVector<const MetadataInfo *, 1> InstMetadata;
   bool RequiresCovered = false;
 
-  if (Options.UAR) {
-    for (unsigned i = 0; i < I.getNumOperands(); ++i) {
-      const Value *V = I.getOperand(i);
-      // TODO(dvyukov): check if V is an address of alloca/function arg.
-      // See isSafeAndProfitableToSinkLoad for addr-taken allocas
-      // and DeadArgumentEliminationPass::removeDeadStuffFromFunction
-      // for iteration over function args.
-      if (V) {
-        RequiresCovered = true;
-        FeatureMask |= kSanitizerBinaryMetadataUAR;
-      }
-    }
+  if (Options.UAR && !(FeatureMask & kSanitizerBinaryMetadataUAR)) {
+    if (useAfterReturnUnsafe(I))
+      FeatureMask |= kSanitizerBinaryMetadataUAR;
   }
 
   if (Options.Atomics && I.mayReadOrWriteMemory()) {
