https://github.com/Szelethus updated https://github.com/llvm/llvm-project/pull/94357
From b6beb7098bb8e5148fe0467dc976506ff6691f15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krist=C3=B3f=20Umann?= <dkszelet...@gmail.com> Date: Tue, 4 Jun 2024 16:15:42 +0200 Subject: [PATCH 1/2] [analyzer] Factor out NoOwnershipChangeVisitor In preparation for adding essentially the same visitor to StreamChecker, this patch factors this visitor out to a common header. Its true that for the time being NoOwnershipChangeVisitor and NoStateChangeVisitor play a very similar role, so it'd be reasonable desire to merge these. My argument against that is that if we check for a property change that is not a resource this distinction could still be useful. Change-Id: I99d73ccd93a18dd145bbbc83afadbb432dd42b90 --- .../StaticAnalyzer/Checkers/CMakeLists.txt | 1 + .../StaticAnalyzer/Checkers/MallocChecker.cpp | 147 +++--------------- .../Checkers/NoOwnershipChangeVisitor.cpp | 116 ++++++++++++++ .../Checkers/NoOwnershipChangeVisitor.h | 78 ++++++++++ 4 files changed, 214 insertions(+), 128 deletions(-) create mode 100644 clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp create mode 100644 clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 4443ffd092938..2520119bffcdf 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -78,6 +78,7 @@ add_clang_library(clangStaticAnalyzerCheckers NoReturnFunctionChecker.cpp NonNullParamChecker.cpp NonnullGlobalConstantsChecker.cpp + NoOwnershipChangeVisitor.cpp NullabilityChecker.cpp NumberObjectConversionChecker.cpp ObjCAtSyncChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 79ab05f2c7866..41dd9cf3e6831 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -46,6 +46,7 @@ #include "AllocationState.h" #include "InterCheckerAPI.h" +#include "NoOwnershipChangeVisitor.h" #include "clang/AST/Attr.h" #include "clang/AST/DeclCXX.h" #include "clang/AST/DeclTemplate.h" @@ -78,13 +79,11 @@ #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/SetOperations.h" -#include "llvm/ADT/SmallString.h" #include "llvm/ADT/StringExtras.h" #include "llvm/Support/Casting.h" #include "llvm/Support/Compiler.h" #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/raw_ostream.h" -#include <climits> #include <functional> #include <optional> #include <utility> @@ -401,7 +400,7 @@ class MallocChecker bool isFreeingCall(const CallEvent &Call) const; static bool isFreeingOwnershipAttrCall(const FunctionDecl *Func); - friend class NoOwnershipChangeVisitor; + friend class NoMemOwnershipChangeVisitor; CallDescriptionMap<CheckFn> AllocatingMemFnMap{ {{{"alloca"}, 1}, &MallocChecker::checkAlloca}, @@ -730,60 +729,8 @@ class MallocChecker //===----------------------------------------------------------------------===// namespace { -class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor { - // The symbol whose (lack of) ownership change we are interested in. - SymbolRef Sym; - const MallocChecker &Checker; - using OwnerSet = llvm::SmallPtrSet<const MemRegion *, 8>; - - // Collect which entities point to the allocated memory, and could be - // responsible for deallocating it. - class OwnershipBindingsHandler : public StoreManager::BindingsHandler { - SymbolRef Sym; - OwnerSet &Owners; - - public: - OwnershipBindingsHandler(SymbolRef Sym, OwnerSet &Owners) - : Sym(Sym), Owners(Owners) {} - - bool HandleBinding(StoreManager &SMgr, Store Store, const MemRegion *Region, - SVal Val) override { - if (Val.getAsSymbol() == Sym) - Owners.insert(Region); - return true; - } - - LLVM_DUMP_METHOD void dump() const { dumpToStream(llvm::errs()); } - LLVM_DUMP_METHOD void dumpToStream(llvm::raw_ostream &out) const { - out << "Owners: {\n"; - for (const MemRegion *Owner : Owners) { - out << " "; - Owner->dumpToStream(out); - out << ",\n"; - } - out << "}\n"; - } - }; - +class NoMemOwnershipChangeVisitor final : public NoOwnershipChangeVisitor { protected: - OwnerSet getOwnersAtNode(const ExplodedNode *N) { - OwnerSet Ret; - - ProgramStateRef State = N->getState(); - OwnershipBindingsHandler Handler{Sym, Ret}; - State->getStateManager().getStoreManager().iterBindings(State->getStore(), - Handler); - return Ret; - } - - LLVM_DUMP_METHOD static std::string - getFunctionName(const ExplodedNode *CallEnterN) { - if (const CallExpr *CE = llvm::dyn_cast_or_null<CallExpr>( - CallEnterN->getLocationAs<CallEnter>()->getCallExpr())) - if (const FunctionDecl *FD = CE->getDirectCallee()) - return FD->getQualifiedNameAsString(); - return ""; - } /// Syntactically checks whether the callee is a deallocating function. Since /// we have no path-sensitive information on this call (we would need a @@ -793,8 +740,9 @@ class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor { /// See namespace `memory_passed_to_fn_call_free_through_fn_ptr` in /// clang/test/Analysis/NewDeleteLeaks.cpp. bool isFreeingCallAsWritten(const CallExpr &Call) const { - if (Checker.FreeingMemFnMap.lookupAsWritten(Call) || - Checker.ReallocatingMemFnMap.lookupAsWritten(Call)) + const auto *MallocChk = static_cast<const MallocChecker *>(&Checker); + if (MallocChk->FreeingMemFnMap.lookupAsWritten(Call) || + MallocChk->ReallocatingMemFnMap.lookupAsWritten(Call)) return true; if (const auto *Func = @@ -804,23 +752,21 @@ class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor { return false; } + bool hasResourceStateChanged(ProgramStateRef CallEnterState, + ProgramStateRef CallExitEndState) final { + return CallEnterState->get<RegionState>(Sym) != + CallExitEndState->get<RegionState>(Sym); + } + /// Heuristically guess whether the callee intended to free memory. This is /// done syntactically, because we are trying to argue about alternative /// paths of execution, and as a consequence we don't have path-sensitive /// information. - bool doesFnIntendToHandleOwnership(const Decl *Callee, ASTContext &ACtx) { + bool doesFnIntendToHandleOwnership(const Decl *Callee, + ASTContext &ACtx) final { using namespace clang::ast_matchers; const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee); - // Given that the stack frame was entered, the body should always be - // theoretically obtainable. In case of body farms, the synthesized body - // is not attached to declaration, thus triggering the '!FD->hasBody()' - // branch. That said, would a synthesized body ever intend to handle - // ownership? As of today they don't. And if they did, how would we - // put notes inside it, given that it doesn't match any source locations? - if (!FD || !FD->hasBody()) - return false; - auto Matches = match(findAll(stmt(anyOf(cxxDeleteExpr().bind("delete"), callExpr().bind("call")))), *FD->getBody(), ACtx); @@ -838,30 +784,7 @@ class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor { return false; } - bool wasModifiedInFunction(const ExplodedNode *CallEnterN, - const ExplodedNode *CallExitEndN) override { - if (!doesFnIntendToHandleOwnership( - CallExitEndN->getFirstPred()->getLocationContext()->getDecl(), - CallExitEndN->getState()->getAnalysisManager().getASTContext())) - return true; - - if (CallEnterN->getState()->get<RegionState>(Sym) != - CallExitEndN->getState()->get<RegionState>(Sym)) - return true; - - OwnerSet CurrOwners = getOwnersAtNode(CallEnterN); - OwnerSet ExitOwners = getOwnersAtNode(CallExitEndN); - - // Owners in the current set may be purged from the analyzer later on. - // If a variable is dead (is not referenced directly or indirectly after - // some point), it will be removed from the Store before the end of its - // actual lifetime. - // This means that if the ownership status didn't change, CurrOwners - // must be a superset of, but not necessarily equal to ExitOwners. - return !llvm::set_is_subset(ExitOwners, CurrOwners); - } - - static PathDiagnosticPieceRef emitNote(const ExplodedNode *N) { + PathDiagnosticPieceRef emitNote(const ExplodedNode *N) final { PathDiagnosticLocation L = PathDiagnosticLocation::create( N->getLocation(), N->getState()->getStateManager().getContext().getSourceManager()); @@ -870,42 +793,10 @@ class NoOwnershipChangeVisitor final : public NoStateChangeFuncVisitor { "later deallocation"); } - PathDiagnosticPieceRef - maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R, - const ObjCMethodCall &Call, - const ExplodedNode *N) override { - // TODO: Implement. - return nullptr; - } - - PathDiagnosticPieceRef - maybeEmitNoteForCXXThis(PathSensitiveBugReport &R, - const CXXConstructorCall &Call, - const ExplodedNode *N) override { - // TODO: Implement. - return nullptr; - } - - PathDiagnosticPieceRef - maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call, - const ExplodedNode *N) override { - // TODO: Factor the logic of "what constitutes as an entity being passed - // into a function call" out by reusing the code in - // NoStoreFuncVisitor::maybeEmitNoteForParameters, maybe by incorporating - // the printing technology in UninitializedObject's FieldChainInfo. - ArrayRef<ParmVarDecl *> Parameters = Call.parameters(); - for (unsigned I = 0; I < Call.getNumArgs() && I < Parameters.size(); ++I) { - SVal V = Call.getArgSVal(I); - if (V.getAsSymbol() == Sym) - return emitNote(N); - } - return nullptr; - } - public: - NoOwnershipChangeVisitor(SymbolRef Sym, const MallocChecker *Checker) - : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough), Sym(Sym), - Checker(*Checker) {} + NoMemOwnershipChangeVisitor(SymbolRef Sym, const MallocChecker *Checker) + : NoOwnershipChangeVisitor(Sym, Checker) {} + void Profile(llvm::FoldingSetNodeID &ID) const override { static int Tag = 0; @@ -2790,7 +2681,7 @@ void MallocChecker::HandleLeak(SymbolRef Sym, ExplodedNode *N, R->markInteresting(Sym); R->addVisitor<MallocBugVisitor>(Sym, true); if (ShouldRegisterNoOwnershipChangeVisitor) - R->addVisitor<NoOwnershipChangeVisitor>(Sym, this); + R->addVisitor<NoMemOwnershipChangeVisitor>(Sym, this); C.emitReport(std::move(R)); } diff --git a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp new file mode 100644 index 0000000000000..2ff76679b5ebf --- /dev/null +++ b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.cpp @@ -0,0 +1,116 @@ +//===--------------------------------------------------------------*- C++ -*--// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "NoOwnershipChangeVisitor.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h" +#include "llvm/ADT/SetOperations.h" + +using namespace clang; +using namespace ento; +using OwnerSet = NoOwnershipChangeVisitor::OwnerSet; + +// Collect which entities point to the allocated memory, and could be +// responsible for deallocating it. +class OwnershipBindingsHandler : public StoreManager::BindingsHandler { + SymbolRef Sym; + OwnerSet &Owners; + +public: + OwnershipBindingsHandler(SymbolRef Sym, OwnerSet &Owners) + : Sym(Sym), Owners(Owners) {} + + bool HandleBinding(StoreManager &SMgr, Store Store, const MemRegion *Region, + SVal Val) override { + if (Val.getAsSymbol() == Sym) + Owners.insert(Region); + return true; + } + + LLVM_DUMP_METHOD void dump() const { dumpToStream(llvm::errs()); } + LLVM_DUMP_METHOD void dumpToStream(llvm::raw_ostream &out) const { + out << "Owners: {\n"; + for (const MemRegion *Owner : Owners) { + out << " "; + Owner->dumpToStream(out); + out << ",\n"; + } + out << "}\n"; + } +}; + +OwnerSet NoOwnershipChangeVisitor::getOwnersAtNode(const ExplodedNode *N) { + OwnerSet Ret; + + ProgramStateRef State = N->getState(); + OwnershipBindingsHandler Handler{Sym, Ret}; + State->getStateManager().getStoreManager().iterBindings(State->getStore(), + Handler); + return Ret; +} + +LLVM_DUMP_METHOD std::string +NoOwnershipChangeVisitor::getFunctionName(const ExplodedNode *CallEnterN) { + if (const CallExpr *CE = llvm::dyn_cast_or_null<CallExpr>( + CallEnterN->getLocationAs<CallEnter>()->getCallExpr())) + if (const FunctionDecl *FD = CE->getDirectCallee()) + return FD->getQualifiedNameAsString(); + return ""; +} + +bool NoOwnershipChangeVisitor::wasModifiedInFunction( + const ExplodedNode *CallEnterN, const ExplodedNode *CallExitEndN) { + const Decl *Callee = + CallExitEndN->getFirstPred()->getLocationContext()->getDecl(); + const FunctionDecl *FD = dyn_cast<FunctionDecl>(Callee); + + // Given that the stack frame was entered, the body should always be + // theoretically obtainable. In case of body farms, the synthesized body + // is not attached to declaration, thus triggering the '!FD->hasBody()' + // branch. That said, would a synthesized body ever intend to handle + // ownership? As of today they don't. And if they did, how would we + // put notes inside it, given that it doesn't match any source locations? + if (!FD || !FD->hasBody()) + return false; + if (!doesFnIntendToHandleOwnership( + Callee, + CallExitEndN->getState()->getAnalysisManager().getASTContext())) + return true; + + if (hasResourceStateChanged(CallEnterN->getState(), CallExitEndN->getState())) + return true; + + OwnerSet CurrOwners = getOwnersAtNode(CallEnterN); + OwnerSet ExitOwners = getOwnersAtNode(CallExitEndN); + + // Owners in the current set may be purged from the analyzer later on. + // If a variable is dead (is not referenced directly or indirectly after + // some point), it will be removed from the Store before the end of its + // actual lifetime. + // This means that if the ownership status didn't change, CurrOwners + // must be a superset of, but not necessarily equal to ExitOwners. + return !llvm::set_is_subset(ExitOwners, CurrOwners); +} + +PathDiagnosticPieceRef NoOwnershipChangeVisitor::maybeEmitNoteForParameters( + PathSensitiveBugReport &R, const CallEvent &Call, const ExplodedNode *N) { + // TODO: Factor the logic of "what constitutes as an entity being passed + // into a function call" out by reusing the code in + // NoStoreFuncVisitor::maybeEmitNoteForParameters, maybe by incorporating + // the printing technology in UninitializedObject's FieldChainInfo. + ArrayRef<ParmVarDecl *> Parameters = Call.parameters(); + for (unsigned I = 0; I < Call.getNumArgs() && I < Parameters.size(); ++I) { + SVal V = Call.getArgSVal(I); + if (V.getAsSymbol() == Sym) + return emitNote(N); + } + return nullptr; +} diff --git a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h new file mode 100644 index 0000000000000..04c1cd80ffc15 --- /dev/null +++ b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h @@ -0,0 +1,78 @@ +//===--------------------------------------------------------------*- C++ -*--// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h" + +namespace clang { +namespace ento { + +class NoOwnershipChangeVisitor : public NoStateChangeFuncVisitor { +protected: + // The symbol whose (lack of) ownership change we are interested in. + SymbolRef Sym; + const CheckerBase &Checker; + + LLVM_DUMP_METHOD static std::string + getFunctionName(const ExplodedNode *CallEnterN); + + /// Heuristically guess whether the callee intended to free the resource. This + /// is done syntactically, because we are trying to argue about alternative + /// paths of execution, and as a consequence we don't have path-sensitive + /// information. + virtual bool doesFnIntendToHandleOwnership(const Decl *Callee, + ASTContext &ACtx) = 0; + + virtual bool hasResourceStateChanged(ProgramStateRef CallEnterState, + ProgramStateRef CallExitEndState) = 0; + + bool wasModifiedInFunction(const ExplodedNode *CallEnterN, + const ExplodedNode *CallExitEndN) final; + + virtual PathDiagnosticPieceRef emitNote(const ExplodedNode *N) = 0; + + PathDiagnosticPieceRef + maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R, + const ObjCMethodCall &Call, + const ExplodedNode *N) final { + // TODO: Implement. + return nullptr; + } + + PathDiagnosticPieceRef + maybeEmitNoteForCXXThis(PathSensitiveBugReport &R, + const CXXConstructorCall &Call, + const ExplodedNode *N) final { + // TODO: Implement. + return nullptr; + } + + // Set this to final, effectively dispatch to emitNote. + PathDiagnosticPieceRef + maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call, + const ExplodedNode *N) final; +public: + using OwnerSet = llvm::SmallPtrSet<const MemRegion *, 8>; + +private: + OwnerSet getOwnersAtNode(const ExplodedNode *N); + +public: + NoOwnershipChangeVisitor(SymbolRef Sym, const CheckerBase *Checker) + : NoStateChangeFuncVisitor(bugreporter::TrackingKind::Thorough), Sym(Sym), + Checker(*Checker) {} + + void Profile(llvm::FoldingSetNodeID &ID) const override { + static int Tag = 0; + ID.AddPointer(&Tag); + ID.AddPointer(Sym); + } +}; +} // namespace ento +} // namespace clang From 07f6daf2cf0f5d5bd4fc9950f2585a3f52b4ad2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krist=C3=B3f=20Umann?= <dkszelet...@gmail.com> Date: Thu, 6 Jun 2024 17:03:44 +0200 Subject: [PATCH 2/2] Make clang-format happy Change-Id: I6caa777e6168d0c291b38b0227b29e66addf7796 --- .../StaticAnalyzer/Checkers/MallocChecker.cpp | 2 -- .../Checkers/NoOwnershipChangeVisitor.h | 17 ++++++++--------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 41dd9cf3e6831..4e7e88b93ca99 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -731,7 +731,6 @@ class MallocChecker namespace { class NoMemOwnershipChangeVisitor final : public NoOwnershipChangeVisitor { protected: - /// Syntactically checks whether the callee is a deallocating function. Since /// we have no path-sensitive information on this call (we would need a /// CallEvent instead of a CallExpr for that), its possible that a @@ -797,7 +796,6 @@ class NoMemOwnershipChangeVisitor final : public NoOwnershipChangeVisitor { NoMemOwnershipChangeVisitor(SymbolRef Sym, const MallocChecker *Checker) : NoOwnershipChangeVisitor(Sym, Checker) {} - void Profile(llvm::FoldingSetNodeID &ID) const override { static int Tag = 0; ID.AddPointer(&Tag); diff --git a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h index 04c1cd80ffc15..027f1a156a7c0 100644 --- a/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h +++ b/clang/lib/StaticAnalyzer/Checkers/NoOwnershipChangeVisitor.h @@ -6,8 +6,8 @@ // //===----------------------------------------------------------------------===// -#include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h" +#include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h" namespace clang { @@ -37,18 +37,16 @@ class NoOwnershipChangeVisitor : public NoStateChangeFuncVisitor { virtual PathDiagnosticPieceRef emitNote(const ExplodedNode *N) = 0; - PathDiagnosticPieceRef - maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R, - const ObjCMethodCall &Call, - const ExplodedNode *N) final { + PathDiagnosticPieceRef maybeEmitNoteForObjCSelf(PathSensitiveBugReport &R, + const ObjCMethodCall &Call, + const ExplodedNode *N) final { // TODO: Implement. return nullptr; } - PathDiagnosticPieceRef - maybeEmitNoteForCXXThis(PathSensitiveBugReport &R, - const CXXConstructorCall &Call, - const ExplodedNode *N) final { + PathDiagnosticPieceRef maybeEmitNoteForCXXThis(PathSensitiveBugReport &R, + const CXXConstructorCall &Call, + const ExplodedNode *N) final { // TODO: Implement. return nullptr; } @@ -57,6 +55,7 @@ class NoOwnershipChangeVisitor : public NoStateChangeFuncVisitor { PathDiagnosticPieceRef maybeEmitNoteForParameters(PathSensitiveBugReport &R, const CallEvent &Call, const ExplodedNode *N) final; + public: using OwnerSet = llvm::SmallPtrSet<const MemRegion *, 8>; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits