Fix unique_ptr aggregate initialization false positives #155131
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Vidur (vidur2) ChangesHi, I am working on a PR for issue #153300. Currently I dont have a regression test or anything for this yet. This is just the initial fix. Patch is 29.71 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/155131.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index efb980962e811..eda1c73b6e029 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -231,12 +231,15 @@ class RefState {
LLVM_DUMP_METHOD void dump(raw_ostream &OS) const {
switch (K) {
-#define CASE(ID) case ID: OS << #ID; break;
- CASE(Allocated)
- CASE(AllocatedOfSizeZero)
- CASE(Released)
- CASE(Relinquished)
- CASE(Escaped)
+#define CASE(ID) \
+ case ID: \
+ OS << #ID; \
+ break;
+ CASE(Allocated)
+ CASE(AllocatedOfSizeZero)
+ CASE(Released)
+ CASE(Relinquished)
+ CASE(Escaped)
}
}
@@ -304,8 +307,7 @@ struct ReallocPair {
ID.AddPointer(ReallocatedSym);
}
bool operator==(const ReallocPair &X) const {
- return ReallocatedSym == X.ReallocatedSym &&
- Kind == X.Kind;
+ return ReallocatedSym == X.ReallocatedSym && Kind == X.Kind;
}
};
@@ -422,27 +424,28 @@ class MallocChecker
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 checkPostObjCMessage(const ObjCMethodCall &Call,
+ CheckerContext &C) const;
void checkPostStmt(const BlockExpr *BE, CheckerContext &C) const;
void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
void checkPreStmt(const ReturnStmt *S, CheckerContext &C) const;
void checkEndFunction(const ReturnStmt *S, CheckerContext &C) const;
ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond,
- bool Assumption) const;
+ bool Assumption) const;
void checkLocation(SVal l, bool isLoad, const Stmt *S,
CheckerContext &C) const;
ProgramStateRef checkPointerEscape(ProgramStateRef State,
- const InvalidatedSymbols &Escaped,
- const CallEvent *Call,
- PointerEscapeKind Kind) const;
+ const InvalidatedSymbols &Escaped,
+ const CallEvent *Call,
+ PointerEscapeKind Kind) const;
ProgramStateRef checkConstPointerEscape(ProgramStateRef State,
const InvalidatedSymbols &Escaped,
const CallEvent *Call,
PointerEscapeKind Kind) const;
- void printState(raw_ostream &Out, ProgramStateRef State,
- const char *NL, const char *Sep) const override;
+ void printState(raw_ostream &Out, ProgramStateRef State, const char *NL,
+ const char *Sep) const override;
StringRef getDebugTag() const override { return "MallocChecker"; }
@@ -789,9 +792,10 @@ class MallocChecker
///
/// We assume that pointers do not escape through calls to system functions
/// not handled by this checker.
- bool mayFreeAnyEscapedMemoryOrIsModeledExplicitly(const CallEvent *Call,
- ProgramStateRef State,
- SymbolRef &EscapingSymbol) const;
+ bool
+ mayFreeAnyEscapedMemoryOrIsModeledExplicitly(const CallEvent *Call,
+ ProgramStateRef State,
+ SymbolRef &EscapingSymbol) const;
/// Implementation of the checkPointerEscape callbacks.
[[nodiscard]] ProgramStateRef
@@ -1253,9 +1257,8 @@ MallocChecker::performKernelMalloc(const CallEvent &Call, CheckerContext &C,
NonLoc ZeroFlag = C.getSValBuilder()
.makeIntVal(*KernelZeroFlagVal, FlagsEx->getType())
.castAs<NonLoc>();
- SVal MaskedFlagsUC = C.getSValBuilder().evalBinOpNN(State, BO_And,
- Flags, ZeroFlag,
- FlagsEx->getType());
+ SVal MaskedFlagsUC = C.getSValBuilder().evalBinOpNN(
+ State, BO_And, Flags, ZeroFlag, FlagsEx->getType());
if (MaskedFlagsUC.isUnknownOrUndef())
return std::nullopt;
DefinedSVal MaskedFlags = MaskedFlagsUC.castAs<DefinedSVal>();
@@ -1485,7 +1488,8 @@ void MallocChecker::checkGMemdup(ProgramStateRef State, const CallEvent &Call,
void MallocChecker::checkGMallocN(ProgramStateRef State, const CallEvent &Call,
CheckerContext &C) const {
SVal Init = UndefinedVal();
- SVal TotalSize = evalMulForBufferSize(C, Call.getArgExpr(0), Call.getArgExpr(1));
+ SVal TotalSize =
+ evalMulForBufferSize(C, Call.getArgExpr(0), Call.getArgExpr(1));
State = MallocMemAux(C, Call, TotalSize, Init, State,
AllocationFamily(AF_Malloc));
State = ProcessZeroAllocCheck(C, Call, 0, State);
@@ -1497,7 +1501,8 @@ void MallocChecker::checkGMallocN0(ProgramStateRef State, const CallEvent &Call,
CheckerContext &C) const {
SValBuilder &SB = C.getSValBuilder();
SVal Init = SB.makeZeroVal(SB.getContext().CharTy);
- SVal TotalSize = evalMulForBufferSize(C, Call.getArgExpr(0), Call.getArgExpr(1));
+ SVal TotalSize =
+ evalMulForBufferSize(C, Call.getArgExpr(0), Call.getArgExpr(1));
State = MallocMemAux(C, Call, TotalSize, Init, State,
AllocationFamily(AF_Malloc));
State = ProcessZeroAllocCheck(C, Call, 0, State);
@@ -2061,8 +2066,8 @@ ProgramStateRef MallocChecker::FreeMemAux(CheckerContext &C,
/// Checks if the previous call to free on the given symbol failed - if free
/// failed, returns true. Also, returns the corresponding return value symbol.
-static bool didPreviousFreeFail(ProgramStateRef State,
- SymbolRef Sym, SymbolRef &RetStatusSymbol) {
+static bool didPreviousFreeFail(ProgramStateRef State, SymbolRef Sym,
+ SymbolRef &RetStatusSymbol) {
const SymbolRef *Ret = State->get<FreeReturnValue>(Sym);
if (Ret) {
assert(*Ret && "We should not store the null return symbol");
@@ -2318,8 +2323,7 @@ MallocChecker::FreeMemAux(CheckerContext &C, const Expr *ArgExpr,
// Check if the memory location being freed is the actual location
// allocated, or an offset.
RegionOffset Offset = R->getAsOffset();
- if (Offset.isValid() &&
- !Offset.hasSymbolicOffset() &&
+ if (Offset.isValid() && !Offset.hasSymbolicOffset() &&
Offset.getOffset() != 0) {
const Expr *AllocExpr = cast<Expr>(RsBase->getStmt());
HandleOffsetFree(C, ArgVal, ArgExpr->getSourceRange(), ParentExpr,
@@ -2365,9 +2369,8 @@ MallocChecker::FreeMemAux(CheckerContext &C, const Expr *ArgExpr,
// Normal free.
if (Hold)
- return State->set<RegionState>(SymBase,
- RefState::getRelinquished(Family,
- ParentExpr));
+ return State->set<RegionState>(
+ SymBase, RefState::getRelinquished(Family, ParentExpr));
return State->set<RegionState>(SymBase,
RefState::getReleased(Family, ParentExpr));
@@ -2606,12 +2609,12 @@ void MallocChecker::HandleMismatchedDealloc(CheckerContext &C,
os << " allocated by " << AllocOs.str();
os << " should be deallocated by ";
- printExpectedDeallocName(os, RS->getAllocationFamily());
+ printExpectedDeallocName(os, RS->getAllocationFamily());
- if (printMemFnName(DeallocOs, C, DeallocExpr))
- os << ", not " << DeallocOs.str();
+ if (printMemFnName(DeallocOs, C, DeallocExpr))
+ os << ", not " << DeallocOs.str();
- printOwnershipTakesList(os, C, DeallocExpr);
+ printOwnershipTakesList(os, C, DeallocExpr);
}
auto R = std::make_unique<PathSensitiveBugReport>(
@@ -2648,8 +2651,7 @@ void MallocChecker::HandleOffsetFree(CheckerContext &C, SVal ArgVal,
assert(MR && "Only MemRegion based symbols can have offset free errors");
RegionOffset Offset = MR->getAsOffset();
- assert((Offset.isValid() &&
- !Offset.hasSymbolicOffset() &&
+ assert((Offset.isValid() && !Offset.hasSymbolicOffset() &&
Offset.getOffset() != 0) &&
"Only symbols with a valid offset can have offset free errors");
@@ -2658,11 +2660,8 @@ void MallocChecker::HandleOffsetFree(CheckerContext &C, SVal ArgVal,
os << "Argument to ";
if (!printMemFnName(os, C, DeallocExpr))
os << "deallocator";
- os << " is offset by "
- << offsetBytes
- << " "
- << ((abs(offsetBytes) > 1) ? "bytes" : "byte")
- << " from the start of ";
+ os << " is offset by " << offsetBytes << " "
+ << ((abs(offsetBytes) > 1) ? "bytes" : "byte") << " from the start of ";
if (AllocExpr && printMemFnName(AllocNameOs, C, AllocExpr))
os << "memory allocated by " << AllocNameOs.str();
else
@@ -2892,8 +2891,8 @@ MallocChecker::ReallocMemAux(CheckerContext &C, const CallEvent &Call,
// Record the info about the reallocated symbol so that we could properly
// process failed reallocation.
- stateRealloc = stateRealloc->set<ReallocPairs>(ToPtr,
- ReallocPair(FromPtr, Kind));
+ stateRealloc =
+ stateRealloc->set<ReallocPairs>(ToPtr, ReallocPair(FromPtr, Kind));
// The reallocated symbol should stay alive for as long as the new symbol.
C.getSymbolManager().addSymbolDependency(ToPtr, FromPtr);
return stateRealloc;
@@ -2951,8 +2950,7 @@ MallocChecker::LeakInfo MallocChecker::getAllocationSite(const ExplodedNode *N,
// Allocation node, is the last node in the current or parent context in
// which the symbol was tracked.
const LocationContext *NContext = N->getLocationContext();
- if (NContext == LeakContext ||
- NContext->isParentOf(LeakContext))
+ if (NContext == LeakContext || NContext->isParentOf(LeakContext))
AllocNode = N;
N = N->pred_empty() ? nullptr : *(N->pred_begin());
}
@@ -2988,9 +2986,8 @@ void MallocChecker::HandleLeak(SymbolRef Sym, ExplodedNode *N,
const Stmt *AllocationStmt = AllocNode->getStmtForDiagnostics();
if (AllocationStmt)
- LocUsedForUniqueing = PathDiagnosticLocation::createBegin(AllocationStmt,
- C.getSourceManager(),
- AllocNode->getLocationContext());
+ LocUsedForUniqueing = PathDiagnosticLocation::createBegin(
+ AllocationStmt, C.getSourceManager(), AllocNode->getLocationContext());
SmallString<200> buf;
llvm::raw_svector_ostream os(buf);
@@ -3012,8 +3009,7 @@ void MallocChecker::HandleLeak(SymbolRef Sym, ExplodedNode *N,
}
void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
- CheckerContext &C) const
-{
+ CheckerContext &C) const {
ProgramStateRef state = C.getState();
RegionStateTy OldRS = state->get<RegionState>();
RegionStateTy::Factory &F = state->get_context<RegionState>();
@@ -3031,8 +3027,7 @@ void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
if (RS == OldRS) {
// We shouldn't have touched other maps yet.
- assert(state->get<ReallocPairs>() ==
- C.getState()->get<ReallocPairs>());
+ assert(state->get<ReallocPairs>() == C.getState()->get<ReallocPairs>());
assert(state->get<FreeReturnValue>() ==
C.getState()->get<FreeReturnValue>());
return;
@@ -3074,6 +3069,43 @@ void MallocChecker::checkPostCall(const CallEvent &Call,
(*PostFN)(this, C.getState(), Call, C);
return;
}
+
+ ProgramStateRef State = C.getState();
+
+ if (const auto *Ctor = dyn_cast<CXXConstructorCall>(&Call)) {
+ // Ensure we are constructing a concrete object/subobject.
+ if (const MemRegion *ObjUnderConstr = Ctor->getCXXThisVal().getAsRegion()) {
+ ProgramStateRef NewState = State;
+
+ for (unsigned I = 0, E = Call.getNumArgs(); I != E; ++I) {
+ SVal ArgV = Call.getArgSVal(I);
+
+ SymbolRef Sym = ArgV.getAsSymbol();
+ if (!Sym)
+ continue;
+
+ // Look up current ref-state for this symbol in the RegionState map.
+ if (const RefState *RS = State->get<RegionState>(Sym)) {
+ // Only re-label symbols that are still owned allocations from C++
+ // new/new[].
+ if (RS->isAllocated() &&
+ (RS->getAllocationFamily().Kind == AF_CXXNew ||
+ RS->getAllocationFamily().Kind == AF_CXXNewArray)) {
+
+ // Mark as Relinquished at the constructor site: ownership moves
+ // into the constructed subobject. Pass the ctor's origin expr as
+ // the statement associated with this transition.
+ NewState = NewState->set<RegionState>(
+ Sym, RefState::getRelinquished(RS->getAllocationFamily(),
+ Ctor->getOriginExpr()));
+ }
+ }
+ }
+
+ if (NewState != State)
+ C.addTransition(NewState);
+ }
+ }
}
void MallocChecker::checkPreCall(const CallEvent &Call,
@@ -3165,8 +3197,7 @@ void MallocChecker::checkPreCall(const CallEvent &Call,
}
}
-void MallocChecker::checkPreStmt(const ReturnStmt *S,
- CheckerContext &C) const {
+void MallocChecker::checkPreStmt(const ReturnStmt *S, CheckerContext &C) const {
checkEscapeOnReturn(S, C);
}
@@ -3198,7 +3229,7 @@ void MallocChecker::checkEscapeOnReturn(const ReturnStmt *S,
if (const MemRegion *MR = RetVal.getAsRegion())
if (isa<FieldRegion, ElementRegion>(MR))
if (const SymbolicRegion *BMR =
- dyn_cast<SymbolicRegion>(MR->getBaseRegion()))
+ dyn_cast<SymbolicRegion>(MR->getBaseRegion()))
Sym = BMR->getSymbol();
// Check if we are returning freed memory.
@@ -3218,14 +3249,13 @@ void MallocChecker::checkPostStmt(const BlockExpr *BE,
return;
ProgramStateRef state = C.getState();
- const BlockDataRegion *R =
- cast<BlockDataRegion>(C.getSVal(BE).getAsRegion());
+ const BlockDataRegion *R = cast<BlockDataRegion>(C.getSVal(BE).getAsRegion());
auto ReferencedVars = R->referenced_vars();
if (ReferencedVars.empty())
return;
- SmallVector<const MemRegion*, 10> Regions;
+ SmallVector<const MemRegion *, 10> Regions;
const LocationContext *LC = C.getLocationContext();
MemRegionManager &MemMgr = C.getSValBuilder().getRegionManager();
@@ -3237,8 +3267,7 @@ void MallocChecker::checkPostStmt(const BlockExpr *BE,
Regions.push_back(VR);
}
- state =
- state->scanReachableSymbols<StopTrackingCallback>(Regions).getState();
+ state = state->scanReachableSymbols<StopTrackingCallback>(Regions).getState();
C.addTransition(state);
}
@@ -3295,8 +3324,7 @@ void MallocChecker::checkUseZeroAllocated(SymbolRef Sym, CheckerContext &C,
if (const RefState *RS = C.getState()->get<RegionState>(Sym)) {
if (RS->isAllocatedOfSizeZero())
HandleUseZeroAlloc(C, RS->getStmt()->getSourceRange(), Sym);
- }
- else if (C.getState()->contains<ReallocSizeZeroSymbols>(Sym)) {
+ } else if (C.getState()->contains<ReallocSizeZeroSymbols>(Sym)) {
HandleUseZeroAlloc(C, S->getSourceRange(), Sym);
}
}
@@ -3313,9 +3341,8 @@ void MallocChecker::checkLocation(SVal l, bool isLoad, const Stmt *S,
// If a symbolic region is assumed to NULL (or another constant), stop tracking
// it - assuming that allocation failed on this path.
-ProgramStateRef MallocChecker::evalAssume(ProgramStateRef state,
- SVal Cond,
- bool Assumption) const {
+ProgramStateRef MallocChecker::evalAssume(ProgramStateRef state, SVal Cond,
+ bool Assumption) const {
RegionStateTy RS = state->get<RegionState>();
for (SymbolRef Sym : llvm::make_first_range(RS)) {
// If the symbol is assumed to be NULL, remove it from consideration.
@@ -3340,7 +3367,8 @@ ProgramStateRef MallocChecker::evalAssume(ProgramStateRef state,
if (RS->isReleased()) {
switch (ReallocPair.Kind) {
case OAR_ToBeFreedAfterFailure:
- state = state->set<RegionState>(ReallocSym,
+ state = state->set<RegionState>(
+ ReallocSym,
RefState::getAllocated(RS->getAllocationFamily(), RS->getStmt()));
break;
case OAR_DoNotTrackAfterFailure:
@@ -3358,9 +3386,8 @@ ProgramStateRef MallocChecker::evalAssume(ProgramStateRef state,
}
bool MallocChecker::mayFreeAnyEscapedMemoryOrIsModeledExplicitly(
- const CallEvent *Call,
- ProgramStateRef State,
- SymbolRef &EscapingSymbol) const {
+ const CallEvent *Call, ProgramStateRef State,
+ SymbolRef &EscapingSymbol) const {
assert(Call);
EscapingSymbol = nullptr;
@@ -3470,8 +3497,8 @@ bool MallocChecker::mayFreeAnyEscapedMemoryOrIsModeledExplicitly(
// Do not warn on pointers passed to 'setbuf' when used with std streams,
// these leaks might be intentional when setting the buffer for stdio.
// http://stackoverflow.com/questions/2671151/who-frees-setvbuf-buffer
- if (FName == "setbuf" || FName =="setbuffer" ||
- FName == "setlinebuf" || FName == "setvbuf") {
+ if (FName == "setbuf" || FName == "setbuffer" || FName == "setlinebuf" ||
+ FName == "setvbuf") {
if (Call->getNumArgs() >= 1) {
const Expr *ArgE = Call->getArgExpr(0)->IgnoreParenCasts();
if (const DeclRefExpr *ArgDRE = dyn_cast<DeclRefExpr>(ArgE))
@@ -3521,18 +3548,16 @@ bool MallocChecker::mayFreeAnyEscapedMemoryOrIsModeledExplicitly(
return false;
}
-ProgramStateRef MallocChecker::checkPointerEscape(ProgramStateRef State,
- const InvalidatedSymbols &Escaped,
- const CallEvent *Call,
- PointerEscapeKind Kind) const {
+ProgramStateRef MallocChecker::checkPointerEscape(
+ ProgramStateRef State, const InvalidatedSymbols &Escaped,
+ const CallEvent *Call, PointerEscapeKind Kind) const {
return checkPointerEscapeAux(State, Escaped, Call, Kind,
/*IsConstPointerEscape*/ false);
}
-ProgramStateRef MallocChecker::checkConstPointerEscape(ProgramStateRef State,
- const InvalidatedSymbols &Escaped,
- const CallEvent *Call,
- PointerEscapeKind Kind) const {
+ProgramStateRef MallocChecker::checkConstPointerEscape(
+ ProgramStateRef State, const InvalidatedSymbols &Escaped,
+ const CallEvent *Call, PointerEscapeKind Kind) const {
// If a const pointer escapes, it may not be freed(), but it could be deleted.
return checkPointerEscapeAux(State, Escaped, Call, Kind,
/*IsConstPointerEscape*/ true);
@@ -3724,61 +3749,61 @@ PathDiagnosticPieceRef MallocBugVisitor::VisitNode(const ExplodedNode *N,
}
Msg = OS.str();
break;
- }
- case AF_None:
- assert(false && "Unhandled allocation family!");
- return nullptr;
- }
+ }
+ case AF_None:
+ assert(false && "Unhandled allocation family!");
+ return nullptr;
+ }
- // Record the stack frame that is _responsible_ for this memory release
- // event. This will be used by the false positive suppression heuristics
- // that recognize the release points of reference-counted objects.
- //
- // Usually (e.g. in C) we say that the _responsible_ stack frame is the
...
[truncated]
|
|
Thanks for working on this 🙂 ! Is your code ready for a first round of review, or are you still planning significant changes? By the way, please change the title of this PR to e.g. "Fix unique_ptr aggregate initialization false positives" or something similar that makes it somewhat recognizable. I see that it's difficult to give a short title to this particular change (and long titles should be avoided), but "Fix for false positive issue" is so generic that it would be suitable for at least a third of all analyzer commits. |
vidur2
changed the title
|
Besides the regression test stuff, I should be ready for review. Im sorry about messing up the naming convention, I havent contributed too much to open source stuff. |
|
Ok, I made the changes |
You can test this locally with the following command:git-clang-format --diff HEAD~1 HEAD --extensions cpp -- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp clang/test/Analysis/NewDeleteLeaks.cppView the diff from clang-format here.diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 1c45ecaad..c7b1fa01c 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -417,7 +417,7 @@ public:
// This last frontend is associated with a single bug type which is not used
// elsewhere and has a different bug category, so it's declared separately.
CheckerFrontendWithBugType TaintedAllocChecker{"Tainted Memory Allocation",
- categories::TaintedData};
+ categories::TaintedData};
using LeakInfo = std::pair<const ExplodedNode *, const MemRegion *>;
@@ -433,7 +433,7 @@ public:
ProgramStateRef evalAssume(ProgramStateRef state, SVal Cond,
bool Assumption) const;
void checkLocation(SVal l, bool isLoad, const Stmt *S,
- CheckerContext &C) const;
+ CheckerContext &C) const;
ProgramStateRef checkPointerEscape(ProgramStateRef State,
const InvalidatedSymbols &Escaped,
@@ -3015,8 +3015,7 @@ void MallocChecker::HandleLeak(SymbolRef Sym, ExplodedNode *N,
}
void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
- CheckerContext &C) const
-{
+ CheckerContext &C) const {
ProgramStateRef state = C.getState();
RegionStateTy OldRS = state->get<RegionState>();
RegionStateTy::Factory &F = state->get_context<RegionState>();
@@ -3568,10 +3567,9 @@ ProgramStateRef MallocChecker::checkPointerEscape(ProgramStateRef State,
/*IsConstPointerEscape*/ false);
}
-ProgramStateRef MallocChecker::checkConstPointerEscape(ProgramStateRef State,
- const InvalidatedSymbols &Escaped,
- const CallEvent *Call,
- PointerEscapeKind Kind) const {
+ProgramStateRef MallocChecker::checkConstPointerEscape(
+ ProgramStateRef State, const InvalidatedSymbols &Escaped,
+ const CallEvent *Call, PointerEscapeKind Kind) const {
// If a const pointer escapes, it may not be freed(), but it could be deleted.
return checkPointerEscapeAux(State, Escaped, Call, Kind,
/*IsConstPointerEscape*/ true);
|
There was a problem hiding this comment.
Im sorry about messing up the naming convention, I havent contributed too much to open source stuff.
No harm done, this is part of the plan 🙂 Every contributor must learn these sometime and this time it was your turn to do so.
I read the suggested code changes and overall I like them, this seems to be a useful change and the code seems to be nice.
However, as I started the CI processes, it seems that several tests fail with the current revision of the commit. You can find the details of the failures e.g. by searching for the string "FAIL: Clang" in the (very long...) raw command line output.
By the way it seems that the pull request #152751 (which is also under review right now) proposes a very similar, but more constrained heuristic: it also extends the checkPostCall callback of MallocChecker, but it only marks pointers passed to a constructor as "escaped" if it is the constructor of either unique_ptr or shared_ptr. (That commit also introduces another heuristic which checks unique_ptr or shared_ptr fields of temporary objects to avoid a different sort of false positive.)
We'll need to pay attention to avoid introducing redundancies or conflicting heuristics.
| (RS->getAllocationFamily().Kind == AF_CXXNew || | ||
| RS->getAllocationFamily().Kind == AF_CXXNewArray)) { |
There was a problem hiding this comment.
I know that it's unusual to use malloc in C++, but it works if somebody decides to use it, so personally I wouldn't limit this logic to new/new[]. (But this is just a vague feeling, not a strong opinion.)
What do you think?
| ProgramStateRef State = C.getState(); | ||
|
|
||
| if (const auto *Ctor = dyn_cast<CXXConstructorCall>(&Call)) { | ||
| // Ensure we are constructing a concrete object/subobject. | ||
| if (const MemRegion *ObjUnderConstr = Ctor->getCXXThisVal().getAsRegion()) { | ||
| ProgramStateRef NewState = State; | ||
|
|
||
| for (unsigned I = 0, E = Call.getNumArgs(); I != E; ++I) { | ||
| SVal ArgV = Call.getArgSVal(I); | ||
|
|
||
| SymbolRef Sym = ArgV.getAsSymbol(); | ||
| if (!Sym) | ||
| continue; | ||
|
|
||
| // Look up current ref-state for this symbol in the RegionState map. | ||
| if (const RefState *RS = State->get<RegionState>(Sym)) { | ||
| // Only re-label symbols that are still owned allocations from C++ | ||
| // new/new[]. | ||
| if (RS->isAllocated() && | ||
| (RS->getAllocationFamily().Kind == AF_CXXNew || | ||
| RS->getAllocationFamily().Kind == AF_CXXNewArray)) { | ||
|
|
||
| // Mark as Relinquished at the constructor site: ownership moves | ||
| // into the constructed subobject. Pass the ctor's origin expr as | ||
| // the statement associated with this transition. | ||
| NewState = NewState->set<RegionState>( | ||
| Sym, RefState::getRelinquished(RS->getAllocationFamily(), | ||
| Ctor->getOriginExpr())); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (NewState != State) | ||
| C.addTransition(NewState); | ||
| } | ||
| } |
There was a problem hiding this comment.
This new code forms a separate logical unit, so I think it would be nice to place it in a separate method called e.g. handleRelenquishedToConstructor or something similar. (This would be closer to how the rest of the file is organized.)
| namespace prvalue_aggregate_transfer { | ||
|
|
||
| void ok_aggregate_from_factory() { | ||
| Foo foo = {make_bar(1), make_bar(2)}; // expected-no-diagnostics |
There was a problem hiding this comment.
In our test framework // expected-no-diagnostics would imply that there are no diagnostics anywhere in the test file, which is obviously not the case here.
If you want to verify that some code doesn't produce warnings, you don't need to add any magic comment: warnings automatically imply failure of the test unless they are matched by an // expected-warning comment (in which case not seeing the warning implies failure).
Some tests do add comments like // no-warning or // no-crash to mark significant lines (e.g. lines that would trigger a bug under earlier versions), but these comments are only for the human readers: they don't have any special meaning for our test system.
For the failure, is it possible that the heuristic is too liberal? it seems to be missing certain warnings. I also am running this on my local machine, so im only building the clang-tidy stuff. Is there a way to just run those tests? |
I didn't check the test results exhaustively, but at first glance it seems that your change silences too many warnings.
Short answer: you should build the cmake target Long answer: The LLVM/Clang project contains two separate tools that perform analysis of C/C++ code:
[1] However, in addition to its own checks, clang-tidy is capable of running the checkers of the static analyzer. For example the "clang-tidy check" called |
|
Thanks! Based on your review of the code do you have any suggestions on how I should go about tightening the heuristic? |
Your code puts pointers (that were allocated by To fix the concrete bug that was reported in #153300 it is sufficient to recognize (by checking the class name) that I understand that it is annoying when some work turns out to be redundant; but unfortunately this is a part of decentralized development. (In fact, a few days ago I was in the same shoes when my commit #155237 had to be abandoned because somebody else fixed that bug a bit earlier than me.) Of course recognizing the name of In this direction you could follow the observations in #153300 (comment) to develop a "proper solution" that fixes the underlying issue which prevents the simulated execution of the destructor. This may be a difficult approach (you'd need to tweak tricky internals of the analyzer engine), but if you manage to develop a solution, then that would be an useful improvement of the analyzer. |
|
So is it best to close this PR? |
Yes, I think this PR should be closed (because although it is a good approach for fixing the bug, we have that other commit which is further along the same path). |
4 participants
Hi, I am working on a PR for issue #153300. Currently I dont have a regression test or anything for this yet. This is just the initial fix.