Asan use-after-scope: don't poison allocas if there were untraced lifetime intrinsics in the function (PR41481)

[hans]

Summary

If there are any intrinsics that cannot be traced back to an alloca, we might have missed the start of a variable's scope, leading to false error reports if the variable is poisoned at function entry. Instead, if there are some intrinsics that can't be traced, fail safe and don't poison the variables in that function.

[eugenis]

LGTM
Consider extending the test with another alloca which does not participate in any untraceable lifetimes, but has to be poisoned in entry block anyway.

llvm/test/Instrumentation/AddressSanitizer/stack-poisoning-and-lifetime.ll

223 ↗

(On Diff #195133)

did you mean "should poison"?

[vitalybuka]

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

922 ↗

(On Diff #195133)

How often does this happen?
If often then maybe we should try to limit only that to allocas involved into such conflicts?

[hans]

In D60686#1467406, @eugenis wrote:

LGTM
Consider extending the test with another alloca which does not participate in any untraceable lifetimes, but has to be poisoned in entry block anyway.

But if there are untraceable lifetimes, we don't know what allocas might potentially be involved. At least not as the code currently works, where it doesn't trace selects and phis with different values.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

922 ↗

(On Diff #195133)

I think it's very rare, but it happens. We didn't run into it in any Chromium tests, but one test in V8 did. I think just bailing out for such functions is okay.

llvm/test/Instrumentation/AddressSanitizer/stack-poisoning-and-lifetime.ll

223 ↗

(On Diff #195133)

No, I mean shouldn't poison. Since we don't know the lifetimes we can't poison it. Or am I missing something?

[eugenis]

llvm/test/Instrumentation/AddressSanitizer/stack-poisoning-and-lifetime.ll

223 ↗

(On Diff #195133)

Well, we shouldn't poison them at lifetime intrinsic.
We should poison them at function entry (and only there).

[eugenis]

llvm/test/Instrumentation/AddressSanitizer/stack-poisoning-and-lifetime.ll

223 ↗

(On Diff #195133)

Ah, I get what you mean. We should not poison alloca contents, only their redzones.

llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp

struct FunctionStackPoisoner : public InstVisitor<FunctionStackPoisoner> {
   struct AllocaPoisonCall {
     IntrinsicInst *InsBefore;
     AllocaInst *AI;
     uint64_t Size;
     bool DoPoison;
   };
   SmallVector<AllocaPoisonCall, 8> DynamicAllocaPoisonCallVec;
   SmallVector<AllocaPoisonCall, 8> StaticAllocaPoisonCallVec;
+  bool HasUntracedLifetimeIntrinsic = false;
 
   SmallVector<AllocaInst *, 1> DynamicAllocaVec;
   SmallVector<IntrinsicInst *, 1> StackRestoreVec;
   AllocaInst *DynamicAllocaLayout = nullptr;
   IntrinsicInst *LocalEscapeCall = nullptr;
 
   // Maps Value to an AllocaInst from which the Value is originated.
   using AllocaForValueMapTy = DenseMap<Value *, AllocaInst *>;
bool runOnFunction() {
 
     // Collect alloca, ret, lifetime instructions etc.
     for (BasicBlock *BB : depth_first(&F.getEntryBlock())) visit(*BB);
 
     if (AllocaVec.empty() && DynamicAllocaVec.empty()) return false;
 
     initializeCallbacks(*F.getParent());
 
+    if (HasUntracedLifetimeIntrinsic) {
+      // If there are lifetime intrinsics which couldn't be traced back to an
+      // alloca, we may not know exactly when a variable enters scope, and
+      // therefore should "fail safe" by not poisoning them.
+      StaticAllocaPoisonCallVec.clear();
+      DynamicAllocaPoisonCallVec.clear();
+    }
+
     processDynamicAllocas();
     processStaticAllocas();
 
     if (ClDebugStack) {
       LLVM_DEBUG(dbgs() << F);
     }
     return true;
   }
void visitIntrinsicInst(IntrinsicInst &II) {
     // be stored in IntptrTy.
     const uint64_t SizeValue = Size->getValue().getLimitedValue();
     if (SizeValue == ~0ULL ||
         !ConstantInt::isValueValidForType(IntptrTy, SizeValue))
       return;
     // Find alloca instruction that corresponds to llvm.lifetime argument.
     AllocaInst *AI =
         llvm::findAllocaForValue(II.getArgOperand(1), AllocaForValue);
+    if (!AI) {
+      HasUntracedLifetimeIntrinsic = true;
+      return;
+    }
     // We're interested only in allocas we can handle.
-    if (!AI || !ASan.isInterestingAlloca(*AI))
+    if (!ASan.isInterestingAlloca(*AI))
       return;
     bool DoPoison = (ID == Intrinsic::lifetime_end);
     AllocaPoisonCall APC = {&II, AI, SizeValue, DoPoison};
     if (AI->isStaticAlloca())
       StaticAllocaPoisonCallVec.push_back(APC);
     else if (ClInstrumentDynamicAllocas)
       DynamicAllocaPoisonCallVec.push_back(APC);
   }

llvm/trunk/test/Instrumentation/AddressSanitizer/stack-poisoning-and-lifetime.ll

entry:
 
   ; CHECK-NOT: add i64 [[SHADOW_BASE]]
 
   ret void
   ; CHECK: {{^[0-9]+}}:
   ; CHECK: ret void
 }
 
+declare void @foo(i32*)
+define void @PR41481(i1 %b) sanitize_address {
+; CHECK-LABEL: @PR41481
+entry:
+  %p1 = alloca i32
+  %p2 = alloca i32
+  %q1 = bitcast i32* %p1 to i8*
+  %q2 = bitcast i32* %p2 to i8*
+  br label %bb1
+
+  ; Since we cannot account for all lifetime intrinsics in this function, we
+  ; might have missed a lifetime.start one and therefore shouldn't poison the
+  ; allocas at function entry.
+  ; ENTRY: store i64 -935356719533264399
+  ; ENTRY-UAS: store i64 -935356719533264399
+
+bb1:
+  %p = select i1 %b, i32* %p1, i32* %p2
+  %q = select i1 %b, i8*  %q1, i8*  %q2
+  call void @llvm.lifetime.start.p0i8(i64 4, i8* %q)
+  call void @foo(i32* %p)
+  br i1 %b, label %bb2, label %bb3
+
+bb2:
+  call void @llvm.lifetime.end.p0i8(i64 4, i8* %q1)
+  br label %end
+
+bb3:
+  call void @llvm.lifetime.end.p0i8(i64 4, i8* %q2)
+  br label %end
+
+end:
+  ret void
+}
+
+
 declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
 declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
 
 ; CHECK-ON: declare void @__asan_set_shadow_00(i64, i64)
 ; CHECK-ON: declare void @__asan_set_shadow_f1(i64, i64)
 ; CHECK-ON: declare void @__asan_set_shadow_f2(i64, i64)
 ; CHECK-ON: declare void @__asan_set_shadow_f3(i64, i64)
 ; CHECK-ON: declare void @__asan_set_shadow_f5(i64, i64)
 ; CHECK-ON: declare void @__asan_set_shadow_f8(i64, i64)
 
 ; CHECK-OFF-NOT: declare void @__asan_set_shadow_