Thread Safety Analysis: Fix recursive capability alias resolution (#159921)

Fix a false positive in thread safety alias analysis caused by incorrect
late resolution of aliases. The analysis previously failed to
distinguish between an alias and its defining expression; reassigning a
variable within that expression (e.g., `ptr` in `alias = ptr->field`)
would incorrectly change the dependent alias as well.

The fix is to properly use LocalVariableMap::lookupExpr's updated
context in a recursive lookup.

Reported-by: Christoph Hellwig <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]

Author: melver

clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h

@@ -543,10 +543,14 @@ class SExprBuilder {
   til::BasicBlock *CurrentBB = nullptr;
   BlockInfo *CurrentBlockInfo = nullptr;
 
+  // The closure that captures state required for the lookup; this may be
+  // mutable, so we have to save/restore before/after recursive lookups.
+  using LookupLocalVarExprClosure =
+      std::function<const Expr *(const NamedDecl *)>;
   // Recursion guard.
   llvm::DenseSet<const ValueDecl *> VarsBeingTranslated;
   // Context-dependent lookup of currently valid definitions of local variables.
-  std::function<const Expr *(const NamedDecl *)> LookupLocalVarExpr;
+  LookupLocalVarExprClosure LookupLocalVarExpr;
 };
 
 #ifndef NDEBUG

clang/lib/Analysis/ThreadSafety.cpp

@@ -1668,13 +1668,13 @@ void ThreadSafetyAnalyzer::getEdgeLockset(FactSet& Result,
   const CFGBlockInfo *PredBlockInfo = &BlockInfo[PredBlock->getBlockID()];
   const LocalVarContext &LVarCtx = PredBlockInfo->ExitContext;
 
-  // Temporarily set the lookup context for SExprBuilder.
-  SxBuilder.setLookupLocalVarExpr([&](const NamedDecl *D) -> const Expr * {
-    if (!Handler.issueBetaWarnings())
-      return nullptr;
-    auto Ctx = LVarCtx;
-    return LocalVarMap.lookupExpr(D, Ctx);
-  });
+  if (Handler.issueBetaWarnings()) {
+    // Temporarily set the lookup context for SExprBuilder.
+    SxBuilder.setLookupLocalVarExpr(
+        [this, Ctx = LVarCtx](const NamedDecl *D) mutable -> const Expr * {
+          return LocalVarMap.lookupExpr(D, Ctx);
+        });
+  }
   auto Cleanup = llvm::make_scope_exit(
       [this] { SxBuilder.setLookupLocalVarExpr(nullptr); });
 
@@ -1722,6 +1722,19 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
   LocalVariableMap::Context LVarCtx;
   unsigned CtxIndex;
 
+  // To update and adjust the context.
+  void updateLocalVarMapCtx(const Stmt *S) {
+    if (S)
+      LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx);
+    if (!Analyzer->Handler.issueBetaWarnings())
+      return;
+    // The lookup closure needs to be reconstructed with the refreshed LVarCtx.
+    Analyzer->SxBuilder.setLookupLocalVarExpr(
+        [this, Ctx = LVarCtx](const NamedDecl *D) mutable -> const Expr * {
+          return Analyzer->LocalVarMap.lookupExpr(D, Ctx);
+        });
+  }
+
   // helper functions
 
   void checkAccess(const Expr *Exp, AccessKind AK,
@@ -1747,13 +1760,7 @@ class BuildLockset : public ConstStmtVisitor<BuildLockset> {
       : ConstStmtVisitor<BuildLockset>(), Analyzer(Anlzr), FSet(Info.EntrySet),
         FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext),
         CtxIndex(Info.EntryIndex) {
-    Analyzer->SxBuilder.setLookupLocalVarExpr(
-        [this](const NamedDecl *D) -> const Expr * {
-          if (!Analyzer->Handler.issueBetaWarnings())
-            return nullptr;
-          auto Ctx = LVarCtx;
-          return Analyzer->LocalVarMap.lookupExpr(D, Ctx);
-        });
+    updateLocalVarMapCtx(nullptr);
   }
 
   ~BuildLockset() { Analyzer->SxBuilder.setLookupLocalVarExpr(nullptr); }
@@ -2259,9 +2266,7 @@ void BuildLockset::VisitBinaryOperator(const BinaryOperator *BO) {
   if (!BO->isAssignmentOp())
     return;
 
-  // adjust the context
-  LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, BO, LVarCtx);
-
+  updateLocalVarMapCtx(BO);
   checkAccess(BO->getLHS(), AK_Written);
 }
 
@@ -2307,8 +2312,7 @@ void BuildLockset::examineArguments(const FunctionDecl *FD,
 }
 
 void BuildLockset::VisitCallExpr(const CallExpr *Exp) {
-  // adjust the context
-  LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, Exp, LVarCtx);
+  updateLocalVarMapCtx(Exp);
 
   if (const auto *CE = dyn_cast<CXXMemberCallExpr>(Exp)) {
     const auto *ME = dyn_cast<MemberExpr>(CE->getCallee());
@@ -2404,8 +2408,7 @@ static const Expr *UnpackConstruction(const Expr *E) {
 }
 
 void BuildLockset::VisitDeclStmt(const DeclStmt *S) {
-  // adjust the context
-  LVarCtx = Analyzer->LocalVarMap.getNextContext(CtxIndex, S, LVarCtx);
+  updateLocalVarMapCtx(S);
 
   for (auto *D : S->getDeclGroup()) {
     if (auto *VD = dyn_cast_or_null<VarDecl>(D)) {

clang/lib/Analysis/ThreadSafetyCommon.cpp

@@ -248,9 +248,17 @@ til::SExpr *SExprBuilder::translateVariable(const VarDecl *VD,
   // defining VD, use its pre-assignment value to break the cycle.
   if (VarsBeingTranslated.contains(VD->getCanonicalDecl()))
     return new (Arena) til::LiteralPtr(VD);
-  VarsBeingTranslated.insert(VD->getCanonicalDecl());
+
+  // The closure captures state that is updated to correctly translate chains of
+  // aliases. Restore it when we are done with recursive translation.
   auto Cleanup = llvm::make_scope_exit(
-      [&] { VarsBeingTranslated.erase(VD->getCanonicalDecl()); });
+      [&, RestoreClosure =
+              VarsBeingTranslated.empty() ? LookupLocalVarExpr : nullptr] {
+        VarsBeingTranslated.erase(VD->getCanonicalDecl());
+        if (VarsBeingTranslated.empty())
+          LookupLocalVarExpr = RestoreClosure;
+      });
+  VarsBeingTranslated.insert(VD->getCanonicalDecl());
 
   QualType Ty = VD->getType();
   if (!VD->isStaticLocal() && Ty->isPointerType()) {

clang/test/SemaCXX/warn-thread-safety-analysis.cpp

@@ -7463,6 +7463,7 @@ void testNestedAcquire(Container *c) EXCLUSIVE_LOCK_FUNCTION(&c->foo.mu) {
 
 struct ContainerOfPtr {
   Foo *foo_ptr;
+  ContainerOfPtr *next;
 };
 
 void testIndirectAccess(ContainerOfPtr *fc) {
@@ -7472,6 +7473,23 @@ void testIndirectAccess(ContainerOfPtr *fc) {
   ptr->mu.Unlock();
 }
 
+void testAliasChainUnrelatedReassignment1(ContainerOfPtr *list) {
+  Foo *eb = list->foo_ptr;
+  eb->mu.Lock();
+  list = list->next;
+  eb->data = 42;
+  eb->mu.Unlock();
+}
+
+void testAliasChainUnrelatedReassignment2(ContainerOfPtr *list) {
+  ContainerOfPtr *busyp = list;
+  Foo *eb = busyp->foo_ptr;
+  eb->mu.Lock();
+  busyp = busyp->next;
+  eb->data = 42;
+  eb->mu.Unlock();
+}
+
 void testControlFlowDoWhile(Foo *f, int x) {
   Foo *ptr = f;