[clang] [Clang SA]: add support for mismatched ownership_returns+ownership_takes calls for custom allocation classes (PR #98941)

via cfe-commits cfe-commits at lists.llvm.org
Mon Jul 15 10:47:31 PDT 2024


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Pavel Skripkin (pskrgag)

<details>
<summary>Changes</summary>

Add support for checking mismatched ownership_returns/ownership_takes attributes.

Closes #<!-- -->76861

---
Full diff: https://github.com/llvm/llvm-project/pull/98941.diff


2 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+168-72) 
- (modified) clang/test/Analysis/MismatchedDeallocator-checker-test.mm (+42) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index fe202c79ed620..821e0c8da5d2d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -103,14 +103,46 @@ using namespace std::placeholders;
 namespace {
 
 // Used to check correspondence between allocators and deallocators.
-enum AllocationFamily {
+enum AllocationFamilyKind {
   AF_None,
   AF_Malloc,
   AF_CXXNew,
   AF_CXXNewArray,
   AF_IfNameIndex,
   AF_Alloca,
-  AF_InnerBuffer
+  AF_InnerBuffer,
+  AF_Custom,
+};
+
+class AllocationFamily {
+public:
+  AllocationFamily(AllocationFamilyKind kind,
+                   std::optional<StringRef> name = std::nullopt)
+      : _kind(kind), _customName(name) {
+    assert(kind != AF_Custom || name != std::nullopt);
+  }
+
+  bool operator==(const AllocationFamily &Other) const {
+    if (Other.kind() == this->kind()) {
+      return this->kind() == AF_Custom ? this->_customName == Other._customName
+                                       : true;
+    } else {
+      return false;
+    }
+  }
+
+  bool operator==(const AllocationFamilyKind &kind) {
+    return this->kind() == kind;
+  }
+
+  bool operator!=(const AllocationFamilyKind &kind) { return !(*this == kind); }
+
+  std::optional<StringRef> name() const { return _customName; }
+  AllocationFamilyKind kind() const { return _kind; }
+
+private:
+  AllocationFamilyKind _kind;
+  std::optional<StringRef> _customName;
 };
 
 } // end of anonymous namespace
@@ -194,7 +226,7 @@ class RefState {
   void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddInteger(K);
     ID.AddPointer(S);
-    ID.AddInteger(Family);
+    ID.AddInteger(Family.kind());
   }
 
   LLVM_DUMP_METHOD void dump(raw_ostream &OS) const {
@@ -1670,15 +1702,18 @@ MallocChecker::MallocMemReturnsAttr(CheckerContext &C, const CallEvent &Call,
   if (!State)
     return nullptr;
 
-  if (Att->getModule()->getName() != "malloc")
-    return nullptr;
+  // Preseve previous behavior when "malloc" class means AF_Malloc
+  auto attrClassName = Att->getModule()->getName();
+  auto AF = attrClassName == "malloc"
+                ? AF_Malloc
+                : AllocationFamily(AF_Custom, attrClassName);
 
   if (!Att->args().empty()) {
     return MallocMemAux(C, Call,
                         Call.getArgExpr(Att->args_begin()->getASTIndex()),
-                        UndefinedVal(), State, AF_Malloc);
+                        UndefinedVal(), State, AF);
   }
-  return MallocMemAux(C, Call, UnknownVal(), UndefinedVal(), State, AF_Malloc);
+  return MallocMemAux(C, Call, UnknownVal(), UndefinedVal(), State, AF);
 }
 
 ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
@@ -1830,16 +1865,18 @@ ProgramStateRef MallocChecker::FreeMemAttr(CheckerContext &C,
   if (!State)
     return nullptr;
 
-  if (Att->getModule()->getName() != "malloc")
-    return nullptr;
+  // Preseve previous behavior when "malloc" class means AF_Malloc
+  auto attrClassName = Att->getModule()->getName();
+  auto AF = attrClassName == "malloc"
+                ? AF_Malloc
+                : AllocationFamily(AF_Custom, attrClassName);
 
   bool IsKnownToBeAllocated = false;
 
   for (const auto &Arg : Att->args()) {
-    ProgramStateRef StateI =
-        FreeMemAux(C, Call, State, Arg.getASTIndex(),
-                   Att->getOwnKind() == OwnershipAttr::Holds,
-                   IsKnownToBeAllocated, AF_Malloc);
+    ProgramStateRef StateI = FreeMemAux(
+        C, Call, State, Arg.getASTIndex(),
+        Att->getOwnKind() == OwnershipAttr::Holds, IsKnownToBeAllocated, AF);
     if (StateI)
       State = StateI;
   }
@@ -1877,6 +1914,33 @@ static bool didPreviousFreeFail(ProgramStateRef State,
   return false;
 }
 
+static void printOwnershipTakesList(raw_ostream &os, CheckerContext &C,
+                                    const Expr *E) {
+  if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
+    const FunctionDecl *FD = CE->getDirectCallee();
+    if (!FD)
+      return;
+
+    if (FD->hasAttrs()) {
+      bool attrPrinted = false;
+
+      for (const auto *I : FD->specific_attrs<OwnershipAttr>()) {
+        OwnershipAttr::OwnershipKind OwnKind = I->getOwnKind();
+
+        if (OwnKind == OwnershipAttr::Takes) {
+          if (attrPrinted)
+            os << ", ";
+          else
+            os << ", which takes ownership of ";
+
+          os << '\'' << I->getModule()->getName() << "'";
+          attrPrinted = true;
+        }
+      }
+    }
+  }
+}
+
 static bool printMemFnName(raw_ostream &os, CheckerContext &C, const Expr *E) {
   if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
     // FIXME: This doesn't handle indirect calls.
@@ -1918,26 +1982,54 @@ static bool printMemFnName(raw_ostream &os, CheckerContext &C, const Expr *E) {
 
 static void printExpectedAllocName(raw_ostream &os, AllocationFamily Family) {
 
-  switch(Family) {
-    case AF_Malloc: os << "malloc()"; return;
-    case AF_CXXNew: os << "'new'"; return;
-    case AF_CXXNewArray: os << "'new[]'"; return;
-    case AF_IfNameIndex: os << "'if_nameindex()'"; return;
-    case AF_InnerBuffer: os << "container-specific allocator"; return;
-    case AF_Alloca:
-    case AF_None: llvm_unreachable("not a deallocation expression");
+  switch (Family.kind()) {
+  case AF_Malloc:
+    os << "malloc()";
+    return;
+  case AF_CXXNew:
+    os << "'new'";
+    return;
+  case AF_CXXNewArray:
+    os << "'new[]'";
+    return;
+  case AF_IfNameIndex:
+    os << "'if_nameindex()'";
+    return;
+  case AF_InnerBuffer:
+    os << "container-specific allocator";
+    return;
+  case AF_Custom:
+    os << Family.name().value();
+    return;
+  case AF_Alloca:
+  case AF_None:
+    llvm_unreachable("not a deallocation expression");
   }
 }
 
 static void printExpectedDeallocName(raw_ostream &os, AllocationFamily Family) {
-  switch(Family) {
-    case AF_Malloc: os << "free()"; return;
-    case AF_CXXNew: os << "'delete'"; return;
-    case AF_CXXNewArray: os << "'delete[]'"; return;
-    case AF_IfNameIndex: os << "'if_freenameindex()'"; return;
-    case AF_InnerBuffer: os << "container-specific deallocator"; return;
-    case AF_Alloca:
-    case AF_None: llvm_unreachable("suspicious argument");
+  switch (Family.kind()) {
+  case AF_Malloc:
+    os << "free()";
+    return;
+  case AF_CXXNew:
+    os << "'delete'";
+    return;
+  case AF_CXXNewArray:
+    os << "'delete[]'";
+    return;
+  case AF_IfNameIndex:
+    os << "'if_freenameindex()'";
+    return;
+  case AF_InnerBuffer:
+    os << "container-specific deallocator";
+    return;
+  case AF_Custom:
+    os << "function that takes ownership of '" << Family.name().value() << "\'";
+    return;
+  case AF_Alloca:
+  case AF_None:
+    llvm_unreachable("suspicious argument");
   }
 }
 
@@ -2119,9 +2211,10 @@ MallocChecker::FreeMemAux(CheckerContext &C, const Expr *ArgExpr,
 std::optional<MallocChecker::CheckKind>
 MallocChecker::getCheckIfTracked(AllocationFamily Family,
                                  bool IsALeakCheck) const {
-  switch (Family) {
+  switch (Family.kind()) {
   case AF_Malloc:
   case AF_Alloca:
+  case AF_Custom:
   case AF_IfNameIndex: {
     if (ChecksEnabled[CK_MallocChecker])
       return CK_MallocChecker;
@@ -2373,6 +2466,8 @@ void MallocChecker::HandleMismatchedDealloc(CheckerContext &C,
 
         if (printMemFnName(DeallocOs, C, DeallocExpr))
           os << ", not " << DeallocOs.str();
+
+        printOwnershipTakesList(os, C, DeallocExpr);
     }
 
     auto R = std::make_unique<PathSensitiveBugReport>(*BT_MismatchedDealloc,
@@ -3483,53 +3578,54 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
           Sym, "Returned allocated memory");
     } else if (isReleased(RSCurr, RSPrev, S)) {
       const auto Family = RSCurr->getAllocationFamily();
-      switch (Family) {
-        case AF_Alloca:
-        case AF_Malloc:
-        case AF_CXXNew:
-        case AF_CXXNewArray:
-        case AF_IfNameIndex:
-          Msg = "Memory is released";
+      switch (Family.kind()) {
+      case AF_Alloca:
+      case AF_Malloc:
+      case AF_Custom:
+      case AF_CXXNew:
+      case AF_CXXNewArray:
+      case AF_IfNameIndex:
+        Msg = "Memory is released";
+        StackHint = std::make_unique<StackHintGeneratorForSymbol>(
+            Sym, "Returning; memory was released");
+        break;
+      case AF_InnerBuffer: {
+        const MemRegion *ObjRegion =
+            allocation_state::getContainerObjRegion(statePrev, Sym);
+        const auto *TypedRegion = cast<TypedValueRegion>(ObjRegion);
+        QualType ObjTy = TypedRegion->getValueType();
+        OS << "Inner buffer of '" << ObjTy << "' ";
+
+        if (N->getLocation().getKind() == ProgramPoint::PostImplicitCallKind) {
+          OS << "deallocated by call to destructor";
           StackHint = std::make_unique<StackHintGeneratorForSymbol>(
-              Sym, "Returning; memory was released");
-          break;
-        case AF_InnerBuffer: {
-          const MemRegion *ObjRegion =
-              allocation_state::getContainerObjRegion(statePrev, Sym);
-          const auto *TypedRegion = cast<TypedValueRegion>(ObjRegion);
-          QualType ObjTy = TypedRegion->getValueType();
-          OS << "Inner buffer of '" << ObjTy << "' ";
-
-          if (N->getLocation().getKind() == ProgramPoint::PostImplicitCallKind) {
-            OS << "deallocated by call to destructor";
-            StackHint = std::make_unique<StackHintGeneratorForSymbol>(
-                Sym, "Returning; inner buffer was deallocated");
-          } else {
-            OS << "reallocated by call to '";
-            const Stmt *S = RSCurr->getStmt();
-            if (const auto *MemCallE = dyn_cast<CXXMemberCallExpr>(S)) {
-              OS << MemCallE->getMethodDecl()->getDeclName();
-            } else if (const auto *OpCallE = dyn_cast<CXXOperatorCallExpr>(S)) {
-              OS << OpCallE->getDirectCallee()->getDeclName();
-            } else if (const auto *CallE = dyn_cast<CallExpr>(S)) {
-              auto &CEMgr = BRC.getStateManager().getCallEventManager();
-              CallEventRef<> Call =
-                  CEMgr.getSimpleCall(CallE, state, CurrentLC, {nullptr, 0});
-              if (const auto *D = dyn_cast_or_null<NamedDecl>(Call->getDecl()))
-                OS << D->getDeclName();
-              else
-                OS << "unknown";
-            }
-            OS << "'";
-            StackHint = std::make_unique<StackHintGeneratorForSymbol>(
-                Sym, "Returning; inner buffer was reallocated");
+              Sym, "Returning; inner buffer was deallocated");
+        } else {
+          OS << "reallocated by call to '";
+          const Stmt *S = RSCurr->getStmt();
+          if (const auto *MemCallE = dyn_cast<CXXMemberCallExpr>(S)) {
+            OS << MemCallE->getMethodDecl()->getDeclName();
+          } else if (const auto *OpCallE = dyn_cast<CXXOperatorCallExpr>(S)) {
+            OS << OpCallE->getDirectCallee()->getDeclName();
+          } else if (const auto *CallE = dyn_cast<CallExpr>(S)) {
+            auto &CEMgr = BRC.getStateManager().getCallEventManager();
+            CallEventRef<> Call =
+                CEMgr.getSimpleCall(CallE, state, CurrentLC, {nullptr, 0});
+            if (const auto *D = dyn_cast_or_null<NamedDecl>(Call->getDecl()))
+              OS << D->getDeclName();
+            else
+              OS << "unknown";
           }
-          Msg = OS.str();
-          break;
+          OS << "'";
+          StackHint = std::make_unique<StackHintGeneratorForSymbol>(
+              Sym, "Returning; inner buffer was reallocated");
         }
+        Msg = OS.str();
+        break;
+      }
         case AF_None:
           llvm_unreachable("Unhandled allocation family!");
-      }
+        }
 
       // See if we're releasing memory while inlining a destructor
       // (or one of its callees). This turns on various common
diff --git a/clang/test/Analysis/MismatchedDeallocator-checker-test.mm b/clang/test/Analysis/MismatchedDeallocator-checker-test.mm
index 013d677e515cf..ef87951c0131e 100644
--- a/clang/test/Analysis/MismatchedDeallocator-checker-test.mm
+++ b/clang/test/Analysis/MismatchedDeallocator-checker-test.mm
@@ -14,6 +14,13 @@
 void free(void *);
 void __attribute((ownership_takes(malloc, 1))) my_free(void *);
 
+void __attribute((ownership_returns(malloc1))) *my_malloc1(size_t);
+void __attribute((ownership_takes(malloc1, 1))) my_free1(void *);
+
+void __attribute((ownership_returns(malloc2))) *my_malloc2(size_t);
+void __attribute((ownership_returns(malloc2))) *my_malloc3(size_t);
+void __attribute((ownership_takes(malloc2, 1))) __attribute((ownership_takes(malloc3, 1))) my_free23(void *);
+
 //---------------------------------------------------------------
 // Test if an allocation function matches deallocation function
 //---------------------------------------------------------------
@@ -60,6 +67,41 @@ void testMalloc8() {
   operator delete[](p); // expected-warning{{Memory allocated by malloc() should be deallocated by free(), not operator delete[]}}
 }
 
+void testMalloc9() {
+  int *p = (int *)my_malloc(sizeof(int));
+  my_free(p); // no warning
+}
+
+void testMalloc10() {
+  int *p = (int *)my_malloc1(sizeof(int));
+  my_free1(p); // no warning
+}
+
+void testMalloc11() {
+  int *p = (int *)my_malloc2(sizeof(int));
+  my_free23(p); // no warning
+}
+
+void testMalloc12() {
+  int *p = (int *)my_malloc1(sizeof(int));
+  my_free(p); // expected-warning{{Memory allocated by my_malloc1() should be deallocated by function that takes ownership of 'malloc1', not my_free(), which takes ownership of 'malloc'}}
+}
+
+void testMalloc13() {
+  int *p = (int *)my_malloc2(sizeof(int));
+  my_free1(p); // expected-warning{{Memory allocated by my_malloc2() should be deallocated by function that takes ownership of 'malloc2', not my_free1(), which takes ownership of 'malloc1'}}
+}
+
+void testMalloc14() {
+  int *p = (int *)my_malloc1(sizeof(int));
+  my_free23(p); // expected-warning{{Memory allocated by my_malloc1() should be deallocated by function that takes ownership of 'malloc1', not my_free23(), which takes ownership of 'malloc2', 'malloc3'}}
+}
+
+void testMalloc15() {
+  int *p = (int *)my_malloc1(sizeof(int));
+  free(p); // expected-warning{{Memory allocated by my_malloc1() should be deallocated by function that takes ownership of 'malloc1', not free()}}
+}
+
 void testAlloca() {
   int *p = (int *)__builtin_alloca(sizeof(int));
   delete p; // expected-warning{{Memory allocated by alloca() should not be deallocated}}

``````````

</details>


https://github.com/llvm/llvm-project/pull/98941


More information about the cfe-commits mailing list