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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
