[clang] [WIP] Refactor MallocChecker to use `BindExpr` in `evalCall` (PR #106081)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Aug 26 07:29:27 PDT 2024
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
More information about the cfe-commits
mailing list