Author: Callan Date: 2026-11-01T20:38:10Z New Revision: 15b43f2e657770ce47b9424492ef12fa2a05b472
URL: https://github.com/llvm/llvm-project/commit/15b43f2e657770ce47b9424492ef12fa2a05b472 DIFF: https://github.com/llvm/llvm-project/commit/15b43f2e657770ce47b9424492ef12fa2a05b472.diff LOG: [analyzer] Reject non-symbolic iterator offsets in alpha iterator checkers (#232622) `evalBinOp()` can return a concrete value for a constrained iterator offset. Storing `getAsSymbol()` from that result leaves a null offset in the model, which is later dereferenced. Return failure for non-symbolic results and let `++`/`--` fall back to a fresh offset, preserving the container and the old position for postfix results. Added: clang/test/Analysis/iterator-concrete-offset.cpp Modified: clang/lib/StaticAnalyzer/Checkers/Iterator.cpp clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp Removed: ################################################################################ diff --git a/clang/lib/StaticAnalyzer/Checkers/Iterator.cpp b/clang/lib/StaticAnalyzer/Checkers/Iterator.cpp index 0f067ae0a7657d..a3d37a454f90bf 100644 --- a/clang/lib/StaticAnalyzer/Checkers/Iterator.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/Iterator.cpp @@ -246,12 +246,17 @@ ProgramStateRef advancePosition(ProgramStateRef State, SVal Iter, IntDist = nonloc::ConcreteInt(BVF.getValue(-IntDist.getValue())); BinOp = (BinOp == BO_Add) ? BO_Sub : BO_Add; } - const auto NewPos = - Pos->setTo(SVB.evalBinOp(State, BinOp, - nonloc::SymbolVal(Pos->getOffset()), - IntDist, SymMgr.getType(Pos->getOffset())) - .getAsSymbol()); - return setIteratorPosition(State, Iter, NewPos); + const auto NewOffset = + SVB.evalBinOp(State, BinOp, nonloc::SymbolVal(Pos->getOffset()), IntDist, + SymMgr.getType(Pos->getOffset())) + .getAsSymbol(); + // Even a symbolic offset can simplify to a concrete value. + // FIXME: Represent iterator offsets as NonLoc so that concrete results can be + // preserved. Rejecting them here makes callers lose position information or + // skip range checks. + if (!NewOffset) + return nullptr; + return setIteratorPosition(State, Iter, Pos->setTo(NewOffset)); } // This function tells the analyzer's engine that symbols produced by our diff --git a/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp b/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp index bb3920bd17762d..0b7a7903e71c0f 100644 --- a/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp @@ -104,10 +104,10 @@ class IteratorModeling void processComparison(CheckerContext &C, ProgramStateRef State, SymbolRef Sym1, SymbolRef Sym2, SVal RetVal, OverloadedOperatorKind Op) const; - void handleIncrement(CheckerContext &C, SVal RetVal, SVal Iter, - bool Postfix) const; - void handleDecrement(CheckerContext &C, SVal RetVal, SVal Iter, - bool Postfix) const; + void handleIncrement(CheckerContext &C, ConstCFGElementRef Elem, SVal RetVal, + SVal Iter, bool Postfix) const; + void handleDecrement(CheckerContext &C, ConstCFGElementRef Elem, SVal RetVal, + SVal Iter, bool Postfix) const; void handleRandomIncrOrDecr(CheckerContext &C, ConstCFGElementRef Elem, OverloadedOperatorKind Op, SVal RetVal, SVal Iterator, SVal Amount) const; @@ -402,23 +402,23 @@ IteratorModeling::handleOverloadedOperator(CheckerContext &C, } } else if (isIncrementOperator(Op)) { if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { - handleIncrement(C, Call.getReturnValue(), InstCall->getCXXThisVal(), - Call.getNumArgs()); + handleIncrement(C, Call.getCFGElementRef(), Call.getReturnValue(), + InstCall->getCXXThisVal(), Call.getNumArgs()); return; } - handleIncrement(C, Call.getReturnValue(), Call.getArgSVal(0), - Call.getNumArgs()); + handleIncrement(C, Call.getCFGElementRef(), Call.getReturnValue(), + Call.getArgSVal(0), Call.getNumArgs()); return; } else if (isDecrementOperator(Op)) { if (const auto *InstCall = dyn_cast<CXXInstanceCall>(&Call)) { - handleDecrement(C, Call.getReturnValue(), InstCall->getCXXThisVal(), - Call.getNumArgs()); + handleDecrement(C, Call.getCFGElementRef(), Call.getReturnValue(), + InstCall->getCXXThisVal(), Call.getNumArgs()); return; } - handleDecrement(C, Call.getReturnValue(), Call.getArgSVal(0), - Call.getNumArgs()); + handleDecrement(C, Call.getCFGElementRef(), Call.getReturnValue(), + Call.getArgSVal(0), Call.getNumArgs()); return; } } @@ -536,7 +536,8 @@ void IteratorModeling::processComparison(CheckerContext &C, } } -void IteratorModeling::handleIncrement(CheckerContext &C, SVal RetVal, +void IteratorModeling::handleIncrement(CheckerContext &C, + ConstCFGElementRef Elem, SVal RetVal, SVal Iter, bool Postfix) const { // Increment the symbolic expressions which represents the position of the // iterator @@ -550,19 +551,22 @@ void IteratorModeling::handleIncrement(CheckerContext &C, SVal RetVal, auto NewState = advancePosition(State, Iter, OO_Plus, nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1)))); - assert(NewState && - "Advancing position by concrete int should always be successful"); + // FIXME: When the new offset is concrete, this fallback discards its value + // and its relation to the previous position. + if (!NewState) + NewState = createIteratorPosition(State, Iter, Pos->getContainer(), Elem, + C.getStackFrame(), C.blockCount()); const auto *NewPos = getIteratorPosition(NewState, Iter); assert(NewPos && "Iterator should have position after successful advancement"); - State = setIteratorPosition(State, Iter, *NewPos); - State = setIteratorPosition(State, RetVal, Postfix ? *Pos : *NewPos); + State = setIteratorPosition(NewState, RetVal, Postfix ? *Pos : *NewPos); C.addTransition(State); } -void IteratorModeling::handleDecrement(CheckerContext &C, SVal RetVal, +void IteratorModeling::handleDecrement(CheckerContext &C, + ConstCFGElementRef Elem, SVal RetVal, SVal Iter, bool Postfix) const { // Decrement the symbolic expressions which represents the position of the // iterator @@ -576,15 +580,17 @@ void IteratorModeling::handleDecrement(CheckerContext &C, SVal RetVal, auto NewState = advancePosition(State, Iter, OO_Minus, nonloc::ConcreteInt(BVF.getValue(llvm::APSInt::get(1)))); - assert(NewState && - "Advancing position by concrete int should always be successful"); + // FIXME: When the new offset is concrete, this fallback discards its value + // and its relation to the previous position. + if (!NewState) + NewState = createIteratorPosition(State, Iter, Pos->getContainer(), Elem, + C.getStackFrame(), C.blockCount()); const auto *NewPos = getIteratorPosition(NewState, Iter); assert(NewPos && "Iterator should have position after successful advancement"); - State = setIteratorPosition(State, Iter, *NewPos); - State = setIteratorPosition(State, RetVal, Postfix ? *Pos : *NewPos); + State = setIteratorPosition(NewState, RetVal, Postfix ? *Pos : *NewPos); C.addTransition(State); } @@ -623,6 +629,8 @@ void IteratorModeling::handleRandomIncrOrDecr(CheckerContext &C, State = setIteratorPosition(State, TgtVal, *NewPos); C.addTransition(State); } else { + // FIXME: A concrete new offset is replaced with a fresh symbol here, + // losing its value and its relation to the original position. assignToContainer(C, Elem, TgtVal, Pos->getContainer()); } } @@ -668,6 +676,8 @@ void IteratorModeling::handlePtrIncrOrDecr(CheckerContext &C, ProgramStateRef NewState = setIteratorPosition(State, NewVal, *NewPos); C.addTransition(NewState); } else { + // FIXME: A concrete new offset is replaced with a fresh symbol here, + // losing its value and its relation to the original position. assignToContainer(C, Elem, NewVal, OldPos->getContainer()); } } diff --git a/clang/test/Analysis/iterator-concrete-offset.cpp b/clang/test/Analysis/iterator-concrete-offset.cpp new file mode 100644 index 00000000000000..c2abe8ca6f70de --- /dev/null +++ b/clang/test/Analysis/iterator-concrete-offset.cpp @@ -0,0 +1,73 @@ +// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,alpha.cplusplus.IteratorRange,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=false -verify %s +// RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,alpha.cplusplus.IteratorRange,debug.DebugIteratorModeling,debug.ExprInspection -analyzer-config aggressive-binary-operation-simplification=true -analyzer-config c++-container-inlining=true -verify %s + +#include "Inputs/system-header-simulator-cxx.h" + +template <typename Iterator> +long clang_analyzer_iterator_position(const Iterator &); +template <typename Iterator> +void *clang_analyzer_iterator_container(const Iterator &); +void clang_analyzer_eval(bool); + +// Constrain the abstract offset so that advancing it produces a concrete value. +void prefix_increment(const std::vector<int> &V) { + auto I = V.begin(); + if (clang_analyzer_iterator_position(I) != 1) + return; + + auto J = ++I; + clang_analyzer_eval(clang_analyzer_iterator_container(I) == &V); // expected-warning{{TRUE}} + clang_analyzer_eval(clang_analyzer_iterator_container(J) == &V); // expected-warning{{TRUE}} + // expected-warning@+1{{TRUE}} + clang_analyzer_eval(clang_analyzer_iterator_position(I) == + clang_analyzer_iterator_position(J)); + clang_analyzer_eval(clang_analyzer_iterator_position(I) == 2); // expected-warning{{TRUE}} expected-warning{{FALSE}} FIXME: This should be only TRUE. +} + +void postfix_increment(const std::vector<int> &V) { + auto I = V.begin(); + if (clang_analyzer_iterator_position(I) != 1) + return; + + auto J = I++; + clang_analyzer_eval(clang_analyzer_iterator_container(I) == &V); // expected-warning{{TRUE}} + clang_analyzer_eval(clang_analyzer_iterator_container(J) == &V); // expected-warning{{TRUE}} + clang_analyzer_eval(clang_analyzer_iterator_position(J) == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(clang_analyzer_iterator_position(I) == 2); // expected-warning{{TRUE}} expected-warning{{FALSE}} FIXME: This should be only TRUE. +} + +void prefix_decrement(const std::vector<int> &V) { + auto I = V.end(); + if (clang_analyzer_iterator_position(I) != 1) + return; + + auto J = --I; + clang_analyzer_eval(clang_analyzer_iterator_container(I) == &V); // expected-warning{{TRUE}} + clang_analyzer_eval(clang_analyzer_iterator_container(J) == &V); // expected-warning{{TRUE}} + // expected-warning@+1{{TRUE}} + clang_analyzer_eval(clang_analyzer_iterator_position(I) == + clang_analyzer_iterator_position(J)); + clang_analyzer_eval(clang_analyzer_iterator_position(I) == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}} FIXME: This should be only TRUE. +} + +void postfix_decrement(const std::vector<int> &V) { + auto I = V.end(); + if (clang_analyzer_iterator_position(I) != 1) + return; + + auto J = I--; + clang_analyzer_eval(clang_analyzer_iterator_container(I) == &V); // expected-warning{{TRUE}} + clang_analyzer_eval(clang_analyzer_iterator_container(J) == &V); // expected-warning{{TRUE}} + clang_analyzer_eval(clang_analyzer_iterator_position(J) == 1); // expected-warning{{TRUE}} + clang_analyzer_eval(clang_analyzer_iterator_position(I) == 0); // expected-warning{{TRUE}} expected-warning{{FALSE}} FIXME: This should be only TRUE. +} + +void compound_assignment(const std::vector<int> &V) { + auto I = V.begin(); + if (clang_analyzer_iterator_position(I) != 1) + return; + + I += 2; + clang_analyzer_eval(clang_analyzer_iterator_container(I) == &V); // expected-warning{{TRUE}} + clang_analyzer_eval(clang_analyzer_iterator_position(I) == 3); // expected-warning{{TRUE}} expected-warning{{FALSE}} FIXME: This should be only TRUE. +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
