llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 Author: Benedek Kaibas (benedekaibas) <details> <summary>Changes</summary> I have implemented the `checkPostCall` function to understand the `lifetimebound` annotations when a parameter is annotated with it. I have generated the ExplodedGraph as well and the annotation is present. This PR is just the very base of the whole implementation of making the annotations present in the program state and I do not cover all the cases yet, but I just want to see if you have any suggestions for me. Here is the `Compiler Explorer` link to the code I have used as a test: https://godbolt.org/z/K896aceMf --- Full diff: https://github.com/llvm/llvm-project/pull/200143.diff 7 Files Affected: - (modified) clang/include/clang/StaticAnalyzer/Checkers/Checkers.td (+4) - (modified) clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt (+1) - (added) clang/lib/StaticAnalyzer/Checkers/LifetimeAnnotations.cpp (+73) - (modified) clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp (+111-4) - (modified) clang/test/Analysis/Inputs/system-header-simulator-cxx.h (+24) - (added) clang/test/Analysis/use-after-move-iterator.cpp (+29) - (modified) clang/test/Analysis/use-after-move.cpp (+19) ``````````diff diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index eca2afbe340a9..85d59fc139728 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -788,6 +788,10 @@ def SmartPtrChecker: Checker<"SmartPtr">, Dependencies<[SmartPtrModeling]>, Documentation<HasDocumentation>; +def LifetimeAnnotations : Checker<"LifetimeAnnotations">, + HelpText<"Check for lifetime violations using lifetime annotations">, + Documentation<NotDocumented>; + } // end: "alpha.cplusplus" //===----------------------------------------------------------------------===// diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index 8a0621077b977..3f426186189fa 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -55,6 +55,7 @@ add_clang_library(clangStaticAnalyzerCheckers IteratorModeling.cpp IteratorRangeChecker.cpp IvarInvalidationChecker.cpp + LifetimeAnnotations.cpp LLVMConventionsChecker.cpp LocalizationChecker.cpp MacOSKeychainAPIChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/LifetimeAnnotations.cpp b/clang/lib/StaticAnalyzer/Checkers/LifetimeAnnotations.cpp new file mode 100644 index 0000000000000..54e98b945c7b3 --- /dev/null +++ b/clang/lib/StaticAnalyzer/Checkers/LifetimeAnnotations.cpp @@ -0,0 +1,73 @@ +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" +#include <AllocationState.h> + +using namespace clang; +using namespace ento; + +REGISTER_MAP_WITH_PROGRAMSTATE(LifetimeBoundMap, const MemRegion *, + const MemRegion *); + +class LifetimeAnnotations : public Checker<check::PostCall> { +public: + void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + void printState(raw_ostream &Out, ProgramStateRef State, const char *NL, + const char *Sep) const override; +}; + +void LifetimeAnnotations::checkPostCall(const CallEvent &Call, + CheckerContext &C) const { + ProgramStateRef State = C.getState(); + + const auto *MethodDecl = dyn_cast_if_present<CXXMethodDecl>(Call.getDecl()); + + if (!MethodDecl) + return; + + unsigned LBParamIdx = MethodDecl->getNumParams(); + for (unsigned i = 0; i < MethodDecl->getNumParams(); i++) { + if (MethodDecl->getParamDecl(i)->hasAttr<LifetimeBoundAttr>()) { + LBParamIdx = i; + break; + } + } + if (LBParamIdx == MethodDecl->getNumParams()) + return; + + SVal RetVal = Call.getReturnValue(); + const MemRegion *RetValRegion = RetVal.getAsRegion(); + if (!RetValRegion) + return; + + SVal ArgVal = Call.getArgSVal(LBParamIdx); + const MemRegion *ArgValRegion = ArgVal.getAsRegion(); + if (!ArgValRegion) + return; + + State = State->set<LifetimeBoundMap>(RetValRegion, ArgValRegion); + C.addTransition(State); +} + +void LifetimeAnnotations::printState(raw_ostream &Out, ProgramStateRef State, + const char *NL, const char *Sep) const { + auto LBTy = State->get<LifetimeBoundMap>(); + + if (!LBTy.isEmpty()) { + Out << Sep << "LifetimeBound objects: "; + + for (auto I : LBTy) { + Out << I.first << " bound to " << I.second << NL; + } + } +} + +void ento::registerLifetimeAnnotations(CheckerManager &mgr) { + mgr.registerChecker<LifetimeAnnotations>(); +} + +bool ento::shouldRegisterLifetimeAnnotations(const CheckerManager &mgr) { + return true; +} diff --git a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp index ba8281b186c5d..47e9c585054ae 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp @@ -12,19 +12,23 @@ // //===----------------------------------------------------------------------===// +#include "Iterator.h" #include "Move.h" #include "clang/AST/Attr.h" #include "clang/AST/ExprCXX.h" +#include "clang/Basic/OperatorKinds.h" #include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" #include "clang/StaticAnalyzer/Core/Checker.h" #include "clang/StaticAnalyzer/Core/CheckerManager.h" +#include "clang/StaticAnalyzer/Core/PathSensitive/CallDescription.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h" #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h" #include "llvm/ADT/StringSet.h" using namespace clang; using namespace ento; +using namespace iterator; namespace { struct RegionState { @@ -47,11 +51,12 @@ struct RegionState { namespace { class MoveChecker : public Checker<check::PreCall, check::PostCall, - check::DeadSymbols, check::RegionChanges> { + check::DeadSymbols, check::RegionChanges, eval::Call> { public: void checkPreCall(const CallEvent &MC, CheckerContext &C) const; void checkPostCall(const CallEvent &MC, CheckerContext &C) const; void checkDeadSymbols(SymbolReaper &SR, CheckerContext &C) const; + bool evalCall(const CallEvent &Call, CheckerContext &C) const; ProgramStateRef checkRegionChanges(ProgramStateRef State, const InvalidatedSymbols *Invalidated, @@ -205,6 +210,9 @@ class MoveChecker private: BugType BT{this, "Use-after-move", categories::CXXMoveSemantics}; + // Modelling the 3 argument std::move calls + const CallDescription StdMoveCall{CDM::SimpleFunc, {"std", "move"}, 3}; + // Check if the given form of potential misuse of a given object // should be reported. If so, get it reported. The callback from which // this function was called should immediately return after the call @@ -230,6 +238,10 @@ class MoveChecker REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, RegionState) +// Custom map designed to track containers whose contents were moved by 3-arg +// std::move +REGISTER_MAP_WITH_PROGRAMSTATE(TrackedContentsMap, const MemRegion *, RegionState) + // Define the inter-checker API. namespace clang { namespace ento { @@ -495,6 +507,78 @@ void MoveChecker::checkPostCall(const CallEvent &Call, assert(!C.isDifferent() && "Should not have made transitions on this path!"); } +bool MoveChecker::evalCall(const CallEvent &Call, CheckerContext &C) const { + + const auto *CE = dyn_cast_if_present<CallExpr>(Call.getOriginExpr()); + if (!CE) + return false; + + ProgramStateRef State = C.getState(); + + if (!StdMoveCall.matches(Call)) + return false; + + const auto *BeginCall = + dyn_cast<CXXMemberCallExpr>(CE->getArg(0)->IgnoreImpCasts()); + if (!BeginCall) + return false; + + const Expr *ContainerExpr = BeginCall->getImplicitObjectArgument(); + const auto *DRE = dyn_cast<DeclRefExpr>(ContainerExpr->IgnoreImpCasts()); + if (!DRE) + return false; + + const auto *VD = dyn_cast<VarDecl>(DRE->getDecl()); + if (!VD) + return false; + + const MemRegion *Region = + State->getLValue(VD, C.getLocationContext()).getAsRegion(); + if (!Region) + return false; + + const CXXRecordDecl *RD = ContainerExpr->getType()->getAsCXXRecordDecl(); + if (!RD) + return false; + + ObjectKind OK = classifyObject(State, Region, RD); + + const auto *BackInsCall = dyn_cast<CallExpr>(CE->getArg(2)->IgnoreImpCasts()); + if (!BackInsCall) + return false; + + const Expr *DestExpr = BackInsCall->getArg(0)->IgnoreImpCasts(); + if (!DestExpr) + return false; + + const auto *DestDRE = dyn_cast<DeclRefExpr>(DestExpr); + if (!DestDRE) + return false; + + const auto *DestVD = dyn_cast<VarDecl>(DestDRE->getDecl()); + if (!DestVD) + return false; + + const MemRegion *DestRegion = + State->getLValue(DestVD, C.getLocationContext()).getAsRegion(); + if (!DestRegion) + return false; + + SValBuilder &SVB = State->getStateManager().getSValBuilder(); + SVal ReturnVal = SVB.conjureSymbolVal(Call, C.blockCount()); + State = State->BindExpr(CE, C.getLocationContext(), ReturnVal); + + State = State->invalidateRegions({DestRegion}, Call.getCFGElementRef(), + C.blockCount(), C.getLocationContext(), + /*CausesPointerEscape=*/false); + + if (shouldBeTracked(OK)) + State = State->set<TrackedContentsMap>(Region, RegionState::getMoved()); + + C.addTransition(State); + return true; +} + bool MoveChecker::isMoveSafeMethod(const CXXMethodDecl *MethodDec) const { // We abandon the cases where bool/void/void* conversion happens. if (const auto *ConversionDec = @@ -643,6 +727,32 @@ void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { // class in which the encountered method defined. ThisRegion = ThisRegion->getMostDerivedObjectRegion(); + // Store class declaration as well, for bug reporting purposes. + const CXXRecordDecl *RD = MethodDecl->getParent(); + + if (MethodDecl->getOverloadedOperator() == OO_Star || + MethodDecl->getOverloadedOperator() == OO_Arrow) { + SVal Val = IC->getCXXThisVal(); + + if (const auto *POS = getIteratorPosition(State, Val)) { + const MemRegion *ContainerRegion = POS->getContainer(); + + const auto *TypedRegion = cast<TypedValueRegion>(ContainerRegion); + QualType ObjTy = TypedRegion->getValueType(); + const auto *R = ObjTy->getAsCXXRecordDecl(); + if (State->get<TrackedContentsMap>(ContainerRegion)) { + ExplodedNode *N = tryToReportBug(ContainerRegion, R, C, MK_FunCall); + if (!N || N->isSink()) + return; + + State = State->set<TrackedContentsMap>(ContainerRegion, + RegionState::getReported()); + C.addTransition(State, N); + return; + } + } + } + if (isStateResetMethod(MethodDecl)) { State = removeFromState(State, ThisRegion); C.addTransition(State); @@ -652,9 +762,6 @@ void MoveChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { if (isMoveSafeMethod(MethodDecl)) return; - // Store class declaration as well, for bug reporting purposes. - const CXXRecordDecl *RD = MethodDecl->getParent(); - if (MethodDecl->isOverloadedOperator()) { OverloadedOperatorKind OOK = MethodDecl->getOverloadedOperator(); diff --git a/clang/test/Analysis/Inputs/system-header-simulator-cxx.h b/clang/test/Analysis/Inputs/system-header-simulator-cxx.h index c5aeb0af9d578..78fe4994b8d44 100644 --- a/clang/test/Analysis/Inputs/system-header-simulator-cxx.h +++ b/clang/test/Analysis/Inputs/system-header-simulator-cxx.h @@ -386,6 +386,8 @@ namespace std { void assign(std::initializer_list<T> ilist); void clear(); + size_t size() const; + bool empty() const; void push_back(const T &value); void push_back(T &&value); @@ -1002,6 +1004,28 @@ next(ForwardIterator it, OutputIterator copy(InputIterator first, InputIterator last, OutputIterator result); + template<class InputIterator, class OutputIterator> + OutputIterator move(InputIterator first, InputIterator last, + OutputIterator result); + + template <typename Container> struct back_insert_iterator { + Container *item; + + back_insert_iterator(Container& container) : item(&container) {} + + back_insert_iterator& operator=(const typename Container::value_type& value) { + item->push_back(value); + return *this; + } + back_insert_iterator& operator*() {return *this;} + back_insert_iterator& operator++() {return *this;} + back_insert_iterator operator++(int) {return *this;} + }; + + template <typename Container> + back_insert_iterator<Container> back_inserter(Container& c) { + return back_insert_iterator<Container>(c); + } } #if __cplusplus >= 201103L diff --git a/clang/test/Analysis/use-after-move-iterator.cpp b/clang/test/Analysis/use-after-move-iterator.cpp new file mode 100644 index 0000000000000..50dd7e57b42e4 --- /dev/null +++ b/clang/test/Analysis/use-after-move-iterator.cpp @@ -0,0 +1,29 @@ +// RUN: %clang_analyze_cc1 -std=c++17 -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.IteratorModeling -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false %s -verify -analyzer-config display-checker-name=false + +#include "Inputs/system-header-simulator-cxx.h" + + +//===----------------------------------------------------------------------===// +// Test suite for test functions that require both MoveChecker.cpp and +// IteratorModeling.cpp to be enabled. +// NOTE: Currently the iterator dereference detection is only working when +// IteratorModeling is enabled. +//===----------------------------------------------------------------------===// + +void iteratorDerefSource() { + std::list<std::string> l1; + l1.push_back("l1"); + std::list<std::string> l2; + + std::move(l1.begin(), l1.end(), std::back_inserter(l2)); + *l1.cbegin(); // expected-warning {{Method called on moved-from object 'l1'}} +} + +void iteratorDerefDest() { + std::list<std::string> l1; + l1.push_back("l1"); + std::list<std::string> l2; + + std::move(l1.begin(), l1.end(), std::back_inserter(l2)); + *l2.cbegin(); // no-warning +} diff --git a/clang/test/Analysis/use-after-move.cpp b/clang/test/Analysis/use-after-move.cpp index 24d5dd8a8b3d2..b26f0320f014b 100644 --- a/clang/test/Analysis/use-after-move.cpp +++ b/clang/test/Analysis/use-after-move.cpp @@ -1015,3 +1015,22 @@ struct OtherMoveSafeClasses { // aggressive-note@-2 {{Moved-from object 'Task' is moved}} } }; + +void safeOperatorAfterMove() { + std::list<std::string> l1; + l1.push_back("l1"); + std::list<std::string> l2; + + std::move(l1.begin(), l1.end(), std::back_inserter(l2)); + *l2.cbegin(); // no-warning +} + +void sizeAfterMove() { + std::list<std::string> l1; + l1.push_back("l1"); + l1.push_back("l2"); + std::list<std::string> l2; + + std::move(l1.begin(), l1.end(), std::back_inserter(l2)); + l1.size(); // no-warning +} `````````` </details> https://github.com/llvm/llvm-project/pull/200143 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
