[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