llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Pavel Skripkin (pskrgag) <details> <summary>Changes</summary> PR refactors `MallocChecker` to not violate invariant of `BindExpr`, which should be called only during `evalCall` to avoid conflicts. To achieve this, most of `postCall` logic was moved to `evalCall` with addition return value binding in case of processing of allocation functions. Check functions prototypes was changed to use `State` with bound return value. `checkDelim` logic was left in `postCall` to avoid conflicts with `StreamChecker` which also evaluates `getline` and friends. PR also introduces breaking change: now checker does not try to inline allocation functions and assumes their initial semantics. Closes #<!-- -->73830 --- Patch is 38.57 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/106081.diff 8 Files Affected: - (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h (+1-1) - (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+227-128) - (modified) clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp (+1-2) - (modified) clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp (+1-1) - (modified) clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp (+1-2) - (modified) clang/test/Analysis/NewDelete-checker-test.cpp (-13) - (modified) clang/test/Analysis/NewDelete-intersections.mm (+2-4) - (modified) clang/test/Analysis/malloc-interprocedural.c (-35) ``````````diff diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h index 50d5d254415af3..1a9bef06b15a44 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicExtent.h @@ -36,7 +36,7 @@ DefinedOrUnknownSVal getDynamicElementCount(ProgramStateRef State, /// Set the dynamic extent \p Extent of the region \p MR. ProgramStateRef setDynamicExtent(ProgramStateRef State, const MemRegion *MR, - DefinedOrUnknownSVal Extent, SValBuilder &SVB); + DefinedOrUnknownSVal Extent); /// Get the dynamic extent for a symbolic value that represents a buffer. If /// there is an offsetting to the underlying buffer we consider that too. diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index 3ddcb7e94ae5d6..1f524481049fa4 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -315,13 +315,24 @@ struct ReallocPair { REGISTER_MAP_WITH_PROGRAMSTATE(ReallocPairs, SymbolRef, ReallocPair) -/// Tells if the callee is one of the builtin new/delete operators, including -/// placement operators and other standard overloads. -static bool isStandardNewDelete(const FunctionDecl *FD); -static bool isStandardNewDelete(const CallEvent &Call) { +static bool isStandardNew(const FunctionDecl *FD); +static bool isStandardNew(const CallEvent &Call) { + if (!Call.getDecl() || !isa<FunctionDecl>(Call.getDecl())) + return false; + return isStandardNew(cast<FunctionDecl>(Call.getDecl())); +} + +static bool isStandardDelete(const FunctionDecl *FD); +static bool isStandardDelete(const CallEvent &Call) { if (!Call.getDecl() || !isa<FunctionDecl>(Call.getDecl())) return false; - return isStandardNewDelete(cast<FunctionDecl>(Call.getDecl())); + return isStandardDelete(cast<FunctionDecl>(Call.getDecl())); +} + +/// Tells if the callee is one of the builtin new/delete operators, including +/// placement operators and other standard overloads. +template <typename T> static bool isStandardNewDelete(const T &FD) { + return isStandardDelete(FD) || isStandardNew(FD); } //===----------------------------------------------------------------------===// @@ -334,8 +345,9 @@ class MallocChecker : public Checker<check::DeadSymbols, check::PointerEscape, check::ConstPointerEscape, check::PreStmt<ReturnStmt>, check::EndFunction, check::PreCall, check::PostCall, - check::NewAllocator, check::PostStmt<BlockExpr>, - check::PostObjCMessage, check::Location, eval::Assume> { + eval::Call, check::NewAllocator, + check::PostStmt<BlockExpr>, check::PostObjCMessage, + check::Location, eval::Assume> { public: /// In pessimistic mode, the checker assumes that it does not know which /// functions might free the memory. @@ -367,6 +379,7 @@ class MallocChecker void checkPreCall(const CallEvent &Call, CheckerContext &C) const; void checkPostCall(const CallEvent &Call, CheckerContext &C) const; + bool evalCall(const CallEvent &Call, CheckerContext &C) const; void checkNewAllocator(const CXXAllocatorCall &Call, CheckerContext &C) const; void checkPostObjCMessage(const ObjCMethodCall &Call, CheckerContext &C) const; void checkPostStmt(const BlockExpr *BE, CheckerContext &C) const; @@ -403,7 +416,8 @@ class MallocChecker mutable std::unique_ptr<BugType> BT_TaintedAlloc; #define CHECK_FN(NAME) \ - void NAME(const CallEvent &Call, CheckerContext &C) const; + void NAME(ProgramStateRef State, const CallEvent &Call, CheckerContext &C) \ + const; CHECK_FN(checkFree) CHECK_FN(checkIfNameIndex) @@ -423,11 +437,12 @@ class MallocChecker CHECK_FN(checkReallocN) CHECK_FN(checkOwnershipAttr) - void checkRealloc(const CallEvent &Call, CheckerContext &C, - bool ShouldFreeOnFail) const; + void checkRealloc(ProgramStateRef State, const CallEvent &Call, + CheckerContext &C, bool ShouldFreeOnFail) const; - using CheckFn = std::function<void(const MallocChecker *, - const CallEvent &Call, CheckerContext &C)>; + using CheckFn = + std::function<void(const MallocChecker *, ProgramStateRef State, + const CallEvent &Call, CheckerContext &C)>; const CallDescriptionMap<CheckFn> PreFnMap{ // NOTE: the following CallDescription also matches the C++ standard @@ -436,6 +451,13 @@ class MallocChecker {{CDM::CLibrary, {"getdelim"}, 4}, &MallocChecker::preGetdelim}, }; + const CallDescriptionMap<CheckFn> PostFnMap{ + // NOTE: the following CallDescription also matches the C++ standard + // library function std::getline(); the callback will filter it out. + {{CDM::CLibrary, {"getline"}, 3}, &MallocChecker::checkGetdelim}, + {{CDM::CLibrary, {"getdelim"}, 4}, &MallocChecker::checkGetdelim}, + }; + const CallDescriptionMap<CheckFn> FreeingMemFnMap{ {{CDM::CLibrary, {"free"}, 1}, &MallocChecker::checkFree}, {{CDM::CLibrary, {"if_freenameindex"}, 1}, @@ -446,10 +468,13 @@ class MallocChecker bool isFreeingCall(const CallEvent &Call) const; static bool isFreeingOwnershipAttrCall(const FunctionDecl *Func); + static bool isFreeingOwnershipAttrCall(const CallEvent &Call); + static bool isAllocatingOwnershipAttrCall(const FunctionDecl *Func); + static bool isAllocatingOwnershipAttrCall(const CallEvent &Call); friend class NoMemOwnershipChangeVisitor; - CallDescriptionMap<CheckFn> AllocatingMemFnMap{ + CallDescriptionMap<CheckFn> AllocaMemFnMap{ {{CDM::CLibrary, {"alloca"}, 1}, &MallocChecker::checkAlloca}, {{CDM::CLibrary, {"_alloca"}, 1}, &MallocChecker::checkAlloca}, // The line for "alloca" also covers "__builtin_alloca", but the @@ -457,6 +482,9 @@ class MallocChecker // extra argument: {{CDM::CLibrary, {"__builtin_alloca_with_align"}, 2}, &MallocChecker::checkAlloca}, + }; + + CallDescriptionMap<CheckFn> AllocatingMemFnMap{ {{CDM::CLibrary, {"malloc"}, 1}, &MallocChecker::checkBasicAlloc}, {{CDM::CLibrary, {"malloc"}, 3}, &MallocChecker::checkKernelMalloc}, {{CDM::CLibrary, {"calloc"}, 2}, &MallocChecker::checkCalloc}, @@ -481,23 +509,20 @@ class MallocChecker CallDescriptionMap<CheckFn> ReallocatingMemFnMap{ {{CDM::CLibrary, {"realloc"}, 2}, - std::bind(&MallocChecker::checkRealloc, _1, _2, _3, false)}, + std::bind(&MallocChecker::checkRealloc, _1, _2, _3, _4, false)}, {{CDM::CLibrary, {"reallocf"}, 2}, - std::bind(&MallocChecker::checkRealloc, _1, _2, _3, true)}, + std::bind(&MallocChecker::checkRealloc, _1, _2, _3, _4, true)}, {{CDM::CLibrary, {"g_realloc"}, 2}, - std::bind(&MallocChecker::checkRealloc, _1, _2, _3, false)}, + std::bind(&MallocChecker::checkRealloc, _1, _2, _3, _4, false)}, {{CDM::CLibrary, {"g_try_realloc"}, 2}, - std::bind(&MallocChecker::checkRealloc, _1, _2, _3, false)}, + std::bind(&MallocChecker::checkRealloc, _1, _2, _3, _4, false)}, {{CDM::CLibrary, {"g_realloc_n"}, 3}, &MallocChecker::checkReallocN}, {{CDM::CLibrary, {"g_try_realloc_n"}, 3}, &MallocChecker::checkReallocN}, - - // NOTE: the following CallDescription also matches the C++ standard - // library function std::getline(); the callback will filter it out. - {{CDM::CLibrary, {"getline"}, 3}, &MallocChecker::checkGetdelim}, - {{CDM::CLibrary, {"getdelim"}, 4}, &MallocChecker::checkGetdelim}, }; bool isMemCall(const CallEvent &Call) const; + bool hasOwnershipReturns(const CallEvent &Call) const; + bool hasOwnershipTakesHolds(const CallEvent &Call) const; void reportTaintBug(StringRef Msg, ProgramStateRef State, CheckerContext &C, llvm::ArrayRef<SymbolRef> TaintedSyms, AllocationFamily Family) const; @@ -531,8 +556,8 @@ class MallocChecker /// \param [in] RetVal Specifies the newly allocated pointer value; /// if unspecified, the value of expression \p E is used. [[nodiscard]] static ProgramStateRef - ProcessZeroAllocCheck(const CallEvent &Call, const unsigned IndexOfSizeArg, - ProgramStateRef State, + ProcessZeroAllocCheck(CheckerContext &C, const CallEvent &Call, + const unsigned IndexOfSizeArg, ProgramStateRef State, std::optional<SVal> RetVal = std::nullopt); /// Model functions with the ownership_returns attribute. @@ -554,6 +579,17 @@ class MallocChecker [[nodiscard]] ProgramStateRef MallocMemReturnsAttr(CheckerContext &C, const CallEvent &Call, const OwnershipAttr *Att, ProgramStateRef State) const; + /// Models memory allocation. + /// + /// \param [in] C Checker context. + /// \param [in] Call The expression that allocates memory. + /// \param [in] State The \c ProgramState right before allocation. + /// \param [in] isAlloca Is the allocation function alloca-like + /// \returns The ProgramState with returnValue bindinded + [[nodiscard]] ProgramStateRef MallocBindRetval(CheckerContext &C, + const CallEvent &Call, + ProgramStateRef State, + bool isAlloca) const; /// Models memory allocation. /// @@ -1031,13 +1067,28 @@ class StopTrackingCallback final : public SymbolVisitor { }; } // end anonymous namespace -static bool isStandardNewDelete(const FunctionDecl *FD) { +static bool isStandardNew(const FunctionDecl *FD) { + if (!FD) + return false; + + OverloadedOperatorKind Kind = FD->getOverloadedOperator(); + if (Kind != OO_New && Kind != OO_Array_New) + return false; + + // This is standard if and only if it's not defined in a user file. + SourceLocation L = FD->getLocation(); + // If the header for operator delete is not included, it's still defined + // in an invalid source location. Check to make sure we don't crash. + return !L.isValid() || + FD->getASTContext().getSourceManager().isInSystemHeader(L); +} + +static bool isStandardDelete(const FunctionDecl *FD) { if (!FD) return false; OverloadedOperatorKind Kind = FD->getOverloadedOperator(); - if (Kind != OO_New && Kind != OO_Array_New && Kind != OO_Delete && - Kind != OO_Array_Delete) + if (Kind != OO_Delete && Kind != OO_Array_Delete) return false; // This is standard if and only if it's not defined in a user file. @@ -1052,6 +1103,12 @@ static bool isStandardNewDelete(const FunctionDecl *FD) { // Methods of MallocChecker and MallocBugVisitor. //===----------------------------------------------------------------------===// +bool MallocChecker::isFreeingOwnershipAttrCall(const CallEvent &Call) { + const auto *Func = dyn_cast_or_null<FunctionDecl>(Call.getDecl()); + + return Func ? isFreeingOwnershipAttrCall(Func) : false; +} + bool MallocChecker::isFreeingOwnershipAttrCall(const FunctionDecl *Func) { if (Func->hasAttrs()) { for (const auto *I : Func->specific_attrs<OwnershipAttr>()) { @@ -1067,15 +1124,27 @@ bool MallocChecker::isFreeingCall(const CallEvent &Call) const { if (FreeingMemFnMap.lookup(Call) || ReallocatingMemFnMap.lookup(Call)) return true; - if (const auto *Func = dyn_cast_or_null<FunctionDecl>(Call.getDecl())) - return isFreeingOwnershipAttrCall(Func); + return isFreeingOwnershipAttrCall(Call); +} + +bool MallocChecker::isAllocatingOwnershipAttrCall(const CallEvent &Call) { + const auto *Func = dyn_cast_or_null<FunctionDecl>(Call.getDecl()); + + return Func ? isAllocatingOwnershipAttrCall(Func) : false; +} + +bool MallocChecker::isAllocatingOwnershipAttrCall(const FunctionDecl *Func) { + for (const auto *I : Func->specific_attrs<OwnershipAttr>()) { + if (I->getOwnKind() == OwnershipAttr::Returns) + return true; + } return false; } bool MallocChecker::isMemCall(const CallEvent &Call) const { if (FreeingMemFnMap.lookup(Call) || AllocatingMemFnMap.lookup(Call) || - ReallocatingMemFnMap.lookup(Call)) + AllocaMemFnMap.lookup(Call) || ReallocatingMemFnMap.lookup(Call)) return true; if (!ShouldIncludeOwnershipAnnotatedFunctions) @@ -1182,18 +1251,18 @@ SVal MallocChecker::evalMulForBufferSize(CheckerContext &C, const Expr *Blocks, return TotalSize; } -void MallocChecker::checkBasicAlloc(const CallEvent &Call, +void MallocChecker::checkBasicAlloc(ProgramStateRef State, + const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); State = MallocMemAux(C, Call, Call.getArgExpr(0), UndefinedVal(), State, AllocationFamily(AF_Malloc)); - State = ProcessZeroAllocCheck(Call, 0, State); + State = ProcessZeroAllocCheck(C, Call, 0, State); C.addTransition(State); } -void MallocChecker::checkKernelMalloc(const CallEvent &Call, +void MallocChecker::checkKernelMalloc(ProgramStateRef State, + const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); std::optional<ProgramStateRef> MaybeState = performKernelMalloc(Call, C, State); if (MaybeState) @@ -1226,7 +1295,8 @@ static bool isGRealloc(const CallEvent &Call) { AC.UnsignedLongTy; } -void MallocChecker::checkRealloc(const CallEvent &Call, CheckerContext &C, +void MallocChecker::checkRealloc(ProgramStateRef State, const CallEvent &Call, + CheckerContext &C, bool ShouldFreeOnFail) const { // Ignore calls to functions whose type does not match the expected type of // either the standard realloc or g_realloc from GLib. @@ -1236,24 +1306,22 @@ void MallocChecker::checkRealloc(const CallEvent &Call, CheckerContext &C, if (!isStandardRealloc(Call) && !isGRealloc(Call)) return; - ProgramStateRef State = C.getState(); State = ReallocMemAux(C, Call, ShouldFreeOnFail, State, AllocationFamily(AF_Malloc)); - State = ProcessZeroAllocCheck(Call, 1, State); + State = ProcessZeroAllocCheck(C, Call, 1, State); C.addTransition(State); } -void MallocChecker::checkCalloc(const CallEvent &Call, +void MallocChecker::checkCalloc(ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); State = CallocMem(C, Call, State); - State = ProcessZeroAllocCheck(Call, 0, State); - State = ProcessZeroAllocCheck(Call, 1, State); + State = ProcessZeroAllocCheck(C, Call, 0, State); + State = ProcessZeroAllocCheck(C, Call, 1, State); C.addTransition(State); } -void MallocChecker::checkFree(const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); +void MallocChecker::checkFree(ProgramStateRef State, const CallEvent &Call, + CheckerContext &C) const { bool IsKnownToBeAllocatedMemory = false; if (suppressDeallocationsInSuspiciousContexts(Call, C)) return; @@ -1262,29 +1330,28 @@ void MallocChecker::checkFree(const CallEvent &Call, CheckerContext &C) const { C.addTransition(State); } -void MallocChecker::checkAlloca(const CallEvent &Call, +void MallocChecker::checkAlloca(ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); State = MallocMemAux(C, Call, Call.getArgExpr(0), UndefinedVal(), State, AllocationFamily(AF_Alloca)); - State = ProcessZeroAllocCheck(Call, 0, State); + State = ProcessZeroAllocCheck(C, Call, 0, State); C.addTransition(State); } -void MallocChecker::checkStrdup(const CallEvent &Call, +void MallocChecker::checkStrdup(ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); const auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr()); if (!CE) return; - State = MallocUpdateRefState(C, CE, State, AllocationFamily(AF_Malloc)); + State = MallocMemAux(C, Call, UnknownVal(), UnknownVal(), State, + AllocationFamily(AF_Malloc)); C.addTransition(State); } -void MallocChecker::checkIfNameIndex(const CallEvent &Call, +void MallocChecker::checkIfNameIndex(ProgramStateRef State, + const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); // Should we model this differently? We can allocate a fixed number of // elements with zeros in the last one. State = MallocMemAux(C, Call, UnknownVal(), UnknownVal(), State, @@ -1293,18 +1360,18 @@ void MallocChecker::checkIfNameIndex(const CallEvent &Call, C.addTransition(State); } -void MallocChecker::checkIfFreeNameIndex(const CallEvent &Call, +void MallocChecker::checkIfFreeNameIndex(ProgramStateRef State, + const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); bool IsKnownToBeAllocatedMemory = false; State = FreeMemAux(C, Call, State, 0, false, IsKnownToBeAllocatedMemory, AllocationFamily(AF_IfNameIndex)); C.addTransition(State); } -void MallocChecker::checkCXXNewOrCXXDelete(const CallEvent &Call, +void MallocChecker::checkCXXNewOrCXXDelete(ProgramStateRef State, + const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); bool IsKnownToBeAllocatedMemory = false; const auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr()); if (!CE) @@ -1321,12 +1388,12 @@ void MallocChecker::checkCXXNewOrCXXDelete(const CallEvent &Call, case OO_New: State = MallocMemAux(C, Call, CE->getArg(0), UndefinedVal(), State, AllocationFamily(AF_CXXNew)); - State = ProcessZeroAllocCheck(Call, 0, State); + State = ProcessZeroAllocCheck(C, Call, 0, State); break; case OO_Array_New: State = MallocMemAux(C, Call, CE->getArg(0), UndefinedVal(), State, AllocationFamily(AF_CXXNewArray)); - State = ProcessZeroAllocCheck(Call, 0, State); + State = ProcessZeroAllocCheck(C, Call, 0, State); break; case OO_Delete: State = FreeMemAux(C, Call, State, 0, false, IsKnownToBeAllocatedMemory, @@ -1344,48 +1411,44 @@ void MallocChecker::checkCXXNewOrCXXDelete(const CallEvent &Call, C.addTransition(State); } -void MallocChecker::checkGMalloc0(const CallEvent &Call, +void MallocChecker::checkGMalloc0(ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); SValBuilder &svalBuilder = C.getSValBuilder(); SVal zeroVal = svalBuilder.makeZeroVal(svalBuilder.getContext().CharTy); State = MallocMemAux(C, Call, Call.getArgExpr(0), zeroVal, State, AllocationFamily(AF_Malloc)); - State = ProcessZeroAllocCheck(Call, 0, State); + State = ProcessZeroAllocCheck(C, Call, 0, State); C.addTransition(State); } -void MallocChecker::checkGMemdup(const CallEvent &Call, +void MallocChecker::checkGMemdup(ProgramStateRef State, const CallEvent &Call, CheckerContext &C) const { - ProgramStateRef State = C.getState(); State = MallocMemAux(C, Call, Call.getArgExpr(1), UnknownVal(), State, AllocationFamily(AF_Malloc)); - State = ProcessZeroAllocCheck(Call, 1, State); + State = ProcessZeroAllocCheck(C, Call, 1, State); C.addTransition(State); } -void MallocChecker::checkGMallocN(const CallEvent &Call, +void MallocChecker::checkGMallocN(ProgramStateRef State, const Cal... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/106081 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
