[analyzer] Refactor MallocChecker to use BindExpr in evalCall
#106081
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Pavel Skripkin (pskrgag) ChangesPR refactors To achieve this, most of
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:
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]
|
|
So I just wanted to share progress on this. I have only 2 failing tests now: The problem with those is that now CSA reports read of undefined value obtained from |
pskrgag
changed the title
BindExpr in evalCallBindExpr in evalCall
There was a problem hiding this comment.
The commit looks very promising 😄 I added several minor remarks, but overall I'm very satisfied with the quality of your changes and I'm relieved that I won't have to perform this refactoring.
I have only 2 failing tests now:
Clang :: Analysis/NewDelete+MismatchedDeallocator_intersections.cpp
Clang :: Analysis/NewDelete-intersections.mThe problem with those is that now CSA reports read of undefined value obtained from malloc. I am not sure how to handle it and why it did work before this. Maybe it was related to wrong usage of BindExpr?
"Why it did work before this?" is always an excellent question 🙃 and in this particular case I think I can answer it: previously MallocChecker only did a postCall() callback, so before that free() (whose definition is not available) was evaluated by conservativeEvalCall() and that process invalidated the memory region which was passed to free(), which changed its contents from UndefinedVal to an unknown symbolic value. Now you provide an evalCall, so free does not invalidate the value stored in its argument and you can see the UndefinedVal which was placed there by malloc.
To handle this, I think you should add an explicit invalidation step within the evalCall of freeing functions to signify that the previous knowledge about the contents of the released memory region is no longer valid.
More precisely I can imagine three alternatives:
- (1) A "regular" invalidation would introduce fresh symbols to represent the data readable through the now-invalid freed pointer. (At first we don't know anything about these symbols, but if they participate in conditionals, they can become constrained.) This is equivalent to the old behavior of the checker.
- (2) We could fill the region with
UnknownVal, which is a "we're confused, assume the best about it" fallback placeholder. This is very similar to plan (1) but if I understand it correctly theUnknownVals won't become constrained. - (3) We could fill the region with
UndefinedVal, which is a "this is garbage, reading it is an error" aggressive placeholder. In the testcases that you highlighted, this wouldn't change anything (becausemalloc()already fills the uninitialized memory withUnknownVal), but it would ensure that after code likethe analyzer knows thatint* p = (int*)malloc(sizeof(int)); *p = 123; free(p);
*pis an undefined possibly-overwritten value (and not the concrete value 123).
Among these (3) is the closest representation of the language standard, because reading freed memory regions can produce arbitrary garbage.
However, if the reporting of use-after-free errors is enabled (within MallocChecker.cpp), then the difference between (1)-(3) is irrelevant, because use-after-free is checked as a precondition and we never reach the point where we actually read the contents of the released memory.
Therefore I think (1) (or perhaps 2??) would be the most user-friendly solution, because if the user enables some subcheckers of MallocChecker, but disables the use-after-free check for some reason, then they presumably wouldn't want to see the "use of undefined value" errors that are essentially use-after-free errors.
| @@ -67,19 +67,6 @@ void testGlobalNoThrowPlacementExprNewBeforeOverload() { | |||
| int *p = new(std::nothrow) int; | |||
| } // leak-warning{{Potential leak of memory pointed to by 'p'}} | |||
|
|
|||
| //----- Standard pointer placement operators | |||
| void testGlobalPointerPlacementNew() { | |||
There was a problem hiding this comment.
Could you briefly explain why do you delete this testcase? (I'm just curious -- I don't see an obvious reason, but I didn't dig into the handling of placement operators, so I presume that this is justified.)
There was a problem hiding this comment.
Sure! This tests checks that CSA looks into new implementation and sees that it just returns some constant pointer. Since PR changes this behavior, test no longer works and CSA reports memory leaks
|
Thank you so much for review! After invalidating location in |
|
Since all pipelines has successfully finished, I am changing state to normal pr |
pskrgag
changed the title
BindExpr in evalCallBindExpr in evalCall
| @@ -58,14 +60,14 @@ void testFreeOpNew() { | |||
| void *p = operator new(0); | |||
| free(p); | |||
| // mismatch-warning@-1{{Memory allocated by 'operator new' should be deallocated by 'delete', not 'free()'}} | |||
| } // leak-warning{{Potential leak of memory pointed to by 'p'}} | |||
| } | |||
There was a problem hiding this comment.
I feel that the disappearance of this "Potential leak" warning (and the one in the next TC) is a bit unfortunate, because it's not fair to say that a mismatched invalid deallocator call prevents the leak.
Could we keep this warning in a straightforward way that doesn't involve ugly hacks or lots of added code?
There was a problem hiding this comment.
To be clear, these leak-warning s were added during WIP stage of this PR and then I dropped it after I added invalidation of freed regions.
I will anyway check if it is be possible to add them, tho
There was a problem hiding this comment.
To be clear, these leak-warning s were added during WIP stage of this PR
Oops, I was only looking at the "changes since last review" diff and didn't notice this. Based on this, I'd lean toward preserving the old behavior to make this commit closer to being an NFC. (But I'd support adding this leak warning as a separate commit, if you're interested.)
There was a problem hiding this comment.
I didn't really have time to look in depth into this.
To me, a switch from eval-call to post-call should be NFC for the most part. It would be nice if we could separate out that NFC part in a PR, and then deal with the breaking changes in a limited PR. That would ease the reviewers job I think.
It is, but it causes |
Minor correction: this commit switches from post-call to eval-call, not the other way around.
The "main" part of this change (the actual post-call -> eval-call switch) is close to being NFC, but is not exactly an NFC because there are subtle differences between the two callback kinds. It would be possible to move a few minor cleanup changes (e.g. the removal of the superfluous argument of |
Thank you for review! I have no rights to push to llvm-project. So you can merge if you think it's good time to do so =) |
|
I merged the commit (after slightly clarifying the PR description / commit message by emphasizing that the breaking change only affects an unlikely case -- I hope you don't mind that). |
PR refactors
MallocCheckerto not violate invariant ofBindExpr, which should be called only duringevalCallto avoid conflicts.To achieve this, most of
postCalllogic was moved toevalCallwith addition return value binding in case of processing of allocation functions. Check functions prototypes was changed to useStatewith bound return value.checkDelimlogic was left inpostCallto avoid conflicts withStreamCheckerwhich also evaluatesgetlineand friends.PR also introduces breaking change in the unlikely case when the definition of an allocation function (e.g.
malloc()) is visible: now checker does not try to inline allocation functions and assumes their initial semantics.Closes #73830