From 0685f09cb8316dd492aceea60413a3ffd9b2987f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= Date: Thu, 25 Sep 2025 15:21:12 +0200 Subject: [PATCH 1/4] [analyzer] Avoid use of `CallEvent`s with obsolete state The method `ExprEngine::evalCall` handles multiple state transitions and activates various checker callbacks that take a `CallEvent` parameter (among other parameters). Unfortunately some of these callbacks (EvalCall and pointer escape) were called with a `CallEvent` instance whose attached state was obsolete. This commit fixes this inconsistency by attaching the right state to the `CallEvent`s before their state becomes relevant. I found these inconsistencies as I was trying to understand this part of the source code, so I don't know about any concrete bugs that are caused by them -- but they are definitely fishy. I think it would be nice to handle the "has right state" / "has undefined state" distinction statically within the type system (to prevent the reoccurrence of similar inconsistencies), but I opted for just fixing the current issues as a first step. --- .../StaticAnalyzer/Core/CheckerManager.cpp | 15 ++-- .../Core/ExprEngineCallAndReturn.cpp | 69 +++++++++++-------- 2 files changed, 48 insertions(+), 36 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp index 44c6f9f52cca6..6d80518b010dd 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -731,33 +731,36 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst, ExplodedNodeSet checkDst; NodeBuilder B(Pred, checkDst, Eng.getBuilderContext()); + ProgramStateRef State = Pred->getState(); + CallEventRef<> UpdatedCall = Call.cloneWithState(State); + // Check if any of the EvalCall callbacks can evaluate the call. for (const auto &EvalCallChecker : EvalCallCheckers) { // TODO: Support the situation when the call doesn't correspond // to any Expr. ProgramPoint L = ProgramPoint::getProgramPoint( - Call.getOriginExpr(), ProgramPoint::PostStmtKind, + UpdatedCall->getOriginExpr(), ProgramPoint::PostStmtKind, Pred->getLocationContext(), EvalCallChecker.Checker); bool evaluated = false; { // CheckerContext generates transitions(populates checkDest) on // destruction, so introduce the scope to make sure it gets properly // populated. CheckerContext C(B, Eng, Pred, L); - evaluated = EvalCallChecker(Call, C); + evaluated = EvalCallChecker(*UpdatedCall, C); } #ifndef NDEBUG if (evaluated && evaluatorChecker) { - const auto toString = [](const CallEvent &Call) -> std::string { + const auto toString = [](CallEventRef<> Call) -> std::string { std::string Buf; llvm::raw_string_ostream OS(Buf); - Call.dump(OS); + Call->dump(OS); return Buf; }; std::string AssertionMessage = llvm::formatv( "The '{0}' call has been already evaluated by the {1} checker, " "while the {2} checker also tried to evaluate the same call. At " "most one checker supposed to evaluate a call.", - toString(Call), evaluatorChecker, + toString(UpdatedCall), evaluatorChecker, EvalCallChecker.Checker->getDebugTag()); llvm_unreachable(AssertionMessage.c_str()); } @@ -774,7 +777,7 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst, // If none of the checkers evaluated the call, ask ExprEngine to handle it. if (!evaluatorChecker) { NodeBuilder B(Pred, Dst, Eng.getBuilderContext()); - Eng.defaultEvalCall(B, Pred, Call, CallOpts); + Eng.defaultEvalCall(B, Pred, *UpdatedCall, CallOpts); } } } diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index 0c491b8c4ca90..9360b423f1c08 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -628,6 +628,8 @@ void ExprEngine::VisitCallExpr(const CallExpr *CE, ExplodedNode *Pred, ProgramStateRef ExprEngine::finishArgumentConstruction(ProgramStateRef State, const CallEvent &Call) { + // WARNING: The state attached to 'Call' may be obsolete, do not call any + // methods that rely on it! const Expr *E = Call.getOriginExpr(); // FIXME: Constructors to placement arguments of operator new // are not supported yet. @@ -653,6 +655,8 @@ ProgramStateRef ExprEngine::finishArgumentConstruction(ProgramStateRef State, void ExprEngine::finishArgumentConstruction(ExplodedNodeSet &Dst, ExplodedNode *Pred, const CallEvent &Call) { + // WARNING: The state attached to 'Call' may be obsolete, do not call any + // methods that rely on it! ProgramStateRef State = Pred->getState(); ProgramStateRef CleanedState = finishArgumentConstruction(State, Call); if (CleanedState == State) { @@ -670,35 +674,40 @@ void ExprEngine::finishArgumentConstruction(ExplodedNodeSet &Dst, } void ExprEngine::evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred, - const CallEvent &Call) { - // WARNING: At this time, the state attached to 'Call' may be older than the - // state in 'Pred'. This is a minor optimization since CheckerManager will - // use an updated CallEvent instance when calling checkers, but if 'Call' is - // ever used directly in this function all callers should be updated to pass - // the most recent state. (It is probably not worth doing the work here since - // for some callers this will not be necessary.) + const CallEvent &CallTemplate) { + // WARNING: As this function performs transitions between several different + // states (perhaps in a branching structure) we must be careful to avoid + // referencing obsolete or irrelevant states. In particular, 'CallEvent' + // instances have an attached state (because this is is convenient within the + // checker callbacks) and it is our responsibility to keep these up-to-date. + // In fact, the parameter 'CallTemplate' is a "template" because its attached + // state may be older than the state of 'Pred' (which will be further + // transformed by the transitions within this method). + // (Note that 'runCheckersFor*Call' and 'finishArgumentConstruction' are + // prepared to take this template and and attach the proper state before + // forwarding it to the checkers.) // Run any pre-call checks using the generic call interface. ExplodedNodeSet dstPreVisit; - getCheckerManager().runCheckersForPreCall(dstPreVisit, Pred, - Call, *this); + getCheckerManager().runCheckersForPreCall(dstPreVisit, Pred, CallTemplate, + *this); // Actually evaluate the function call. We try each of the checkers // to see if the can evaluate the function call, and get a callback at // defaultEvalCall if all of them fail. ExplodedNodeSet dstCallEvaluated; - getCheckerManager().runCheckersForEvalCall(dstCallEvaluated, dstPreVisit, - Call, *this, EvalCallOptions()); + getCheckerManager().runCheckersForEvalCall( + dstCallEvaluated, dstPreVisit, CallTemplate, *this, EvalCallOptions()); // If there were other constructors called for object-type arguments // of this call, clean them up. ExplodedNodeSet dstArgumentCleanup; for (ExplodedNode *I : dstCallEvaluated) - finishArgumentConstruction(dstArgumentCleanup, I, Call); + finishArgumentConstruction(dstArgumentCleanup, I, CallTemplate); ExplodedNodeSet dstPostCall; getCheckerManager().runCheckersForPostCall(dstPostCall, dstArgumentCleanup, - Call, *this); + CallTemplate, *this); // Escaping symbols conjured during invalidating the regions above. // Note that, for inlined calls the nodes were put back into the worklist, @@ -708,12 +717,13 @@ void ExprEngine::evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred, // Run pointerEscape callback with the newly conjured symbols. SmallVector, 8> Escaped; for (ExplodedNode *I : dstPostCall) { - NodeBuilder B(I, Dst, *currBldrCtx); ProgramStateRef State = I->getState(); + CallEventRef<> Call = CallTemplate.cloneWithState(State); + NodeBuilder B(I, Dst, *currBldrCtx); Escaped.clear(); { unsigned Arg = -1; - for (const ParmVarDecl *PVD : Call.parameters()) { + for (const ParmVarDecl *PVD : Call->parameters()) { ++Arg; QualType ParamTy = PVD->getType(); if (ParamTy.isNull() || @@ -722,13 +732,13 @@ void ExprEngine::evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred, QualType Pointee = ParamTy->getPointeeType(); if (Pointee.isConstQualified() || Pointee->isVoidType()) continue; - if (const MemRegion *MR = Call.getArgSVal(Arg).getAsRegion()) + if (const MemRegion *MR = Call->getArgSVal(Arg).getAsRegion()) Escaped.emplace_back(loc::MemRegionVal(MR), State->getSVal(MR, Pointee)); } } State = processPointerEscapedOnBind(State, Escaped, I->getLocationContext(), - PSK_EscapeOutParameters, &Call); + PSK_EscapeOutParameters, &*Call); if (State == I->getState()) Dst.insert(I); @@ -1212,48 +1222,47 @@ static bool isTrivialObjectAssignment(const CallEvent &Call) { } void ExprEngine::defaultEvalCall(NodeBuilder &Bldr, ExplodedNode *Pred, - const CallEvent &CallTemplate, + const CallEvent &Call, const EvalCallOptions &CallOpts) { // Make sure we have the most recent state attached to the call. ProgramStateRef State = Pred->getState(); - CallEventRef<> Call = CallTemplate.cloneWithState(State); // Special-case trivial assignment operators. - if (isTrivialObjectAssignment(*Call)) { - performTrivialCopy(Bldr, Pred, *Call); + if (isTrivialObjectAssignment(Call)) { + performTrivialCopy(Bldr, Pred, Call); return; } // Try to inline the call. // The origin expression here is just used as a kind of checksum; // this should still be safe even for CallEvents that don't come from exprs. - const Expr *E = Call->getOriginExpr(); + const Expr *E = Call.getOriginExpr(); ProgramStateRef InlinedFailedState = getInlineFailedState(State, E); if (InlinedFailedState) { // If we already tried once and failed, make sure we don't retry later. State = InlinedFailedState; } else { - RuntimeDefinition RD = Call->getRuntimeDefinition(); - Call->setForeign(RD.isForeign()); + RuntimeDefinition RD = Call.getRuntimeDefinition(); + Call.setForeign(RD.isForeign()); const Decl *D = RD.getDecl(); - if (shouldInlineCall(*Call, D, Pred, CallOpts)) { + if (shouldInlineCall(Call, D, Pred, CallOpts)) { if (RD.mayHaveOtherDefinitions()) { AnalyzerOptions &Options = getAnalysisManager().options; // Explore with and without inlining the call. if (Options.getIPAMode() == IPAK_DynamicDispatchBifurcate) { - BifurcateCall(RD.getDispatchRegion(), *Call, D, Bldr, Pred); + BifurcateCall(RD.getDispatchRegion(), Call, D, Bldr, Pred); return; } // Don't inline if we're not in any dynamic dispatch mode. if (Options.getIPAMode() != IPAK_DynamicDispatch) { - conservativeEvalCall(*Call, Bldr, Pred, State); + conservativeEvalCall(Call, Bldr, Pred, State); return; } } - ctuBifurcate(*Call, D, Bldr, Pred, State); + ctuBifurcate(Call, D, Bldr, Pred, State); return; } } @@ -1261,10 +1270,10 @@ void ExprEngine::defaultEvalCall(NodeBuilder &Bldr, ExplodedNode *Pred, // If we can't inline it, clean up the state traits used only if the function // is inlined. State = removeStateTraitsUsedForArrayEvaluation( - State, dyn_cast_or_null(E), Call->getLocationContext()); + State, dyn_cast_or_null(E), Call.getLocationContext()); // Also handle the return value and invalidate the regions. - conservativeEvalCall(*Call, Bldr, Pred, State); + conservativeEvalCall(Call, Bldr, Pred, State); } void ExprEngine::BifurcateCall(const MemRegion *BifurReg, From 16490339f0f9da77b715d8508586fdba3a534fe3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= Date: Tue, 7 Oct 2025 16:45:52 +0200 Subject: [PATCH 2/4] Fix doubled word typos --- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp index 9360b423f1c08..e388e356497c5 100644 --- a/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp +++ b/clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp @@ -678,13 +678,13 @@ void ExprEngine::evalCall(ExplodedNodeSet &Dst, ExplodedNode *Pred, // WARNING: As this function performs transitions between several different // states (perhaps in a branching structure) we must be careful to avoid // referencing obsolete or irrelevant states. In particular, 'CallEvent' - // instances have an attached state (because this is is convenient within the + // instances have an attached state (because this is convenient within the // checker callbacks) and it is our responsibility to keep these up-to-date. // In fact, the parameter 'CallTemplate' is a "template" because its attached // state may be older than the state of 'Pred' (which will be further // transformed by the transitions within this method). // (Note that 'runCheckersFor*Call' and 'finishArgumentConstruction' are - // prepared to take this template and and attach the proper state before + // prepared to take this template and attach the proper state before // forwarding it to the checkers.) // Run any pre-call checks using the generic call interface. From 5b8416ff78af298c79bba14be0ecc2ca6e50daa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= Date: Tue, 7 Oct 2025 16:51:51 +0200 Subject: [PATCH 3/4] Revert some non-functional changes to reduce diff size --- clang/lib/StaticAnalyzer/Core/CheckerManager.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp index 6d80518b010dd..d6b341ba0274d 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -750,17 +750,17 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst, } #ifndef NDEBUG if (evaluated && evaluatorChecker) { - const auto toString = [](CallEventRef<> Call) -> std::string { + const auto toString = [](const CallEvent &Call) -> std::string { std::string Buf; llvm::raw_string_ostream OS(Buf); - Call->dump(OS); + Call.dump(OS); return Buf; }; std::string AssertionMessage = llvm::formatv( "The '{0}' call has been already evaluated by the {1} checker, " "while the {2} checker also tried to evaluate the same call. At " "most one checker supposed to evaluate a call.", - toString(UpdatedCall), evaluatorChecker, + toString(Call), evaluatorChecker, EvalCallChecker.Checker->getDebugTag()); llvm_unreachable(AssertionMessage.c_str()); } From abccb02b867a1eaf980c97429fc39932ab4429d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= Date: Tue, 7 Oct 2025 16:52:40 +0200 Subject: [PATCH 4/4] Fix a typo in an old comment (unrelated to the commit) --- clang/lib/StaticAnalyzer/Core/CheckerManager.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp index d6b341ba0274d..8ee4832643b91 100644 --- a/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp +++ b/clang/lib/StaticAnalyzer/Core/CheckerManager.cpp @@ -742,7 +742,7 @@ void CheckerManager::runCheckersForEvalCall(ExplodedNodeSet &Dst, UpdatedCall->getOriginExpr(), ProgramPoint::PostStmtKind, Pred->getLocationContext(), EvalCallChecker.Checker); bool evaluated = false; - { // CheckerContext generates transitions(populates checkDest) on + { // CheckerContext generates transitions (populates checkDest) on // destruction, so introduce the scope to make sure it gets properly // populated. CheckerContext C(B, Eng, Pred, L);