Skip to content

Fix owned iterator arrow false positives #8888

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
KiritoYG wants to merge 2 commits into
base: main
Choose a base branch
from
+495 −3
Open

Fix owned iterator arrow false positives #8888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce2ff87
Fix invalid iterator warning for member-backed arrow access
KiritoYG Sep 24, 2026
52edaae
Fix initialization through singleton owned arrow writes
KiritoYG Sep 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 82 additions & 0 deletions lib/astutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3512,6 +3512,88 @@ bool isLeafDot(const Token* tok)
return isLeafDot(parent);
}

static const Variable* singlePlainDataMember(const Scope* scope)
{
if (!scope || (scope->type != ScopeType::eStruct && scope->type != ScopeType::eClass) ||
!scope->definedType || !scope->definedType->derivedFrom.empty() || scope->numConstructors != 0)
return nullptr;
if (std::any_of(scope->functionList.cbegin(), scope->functionList.cend(), [](const Function& function) {
return function.hasVirtualSpecifier() || function.hasOverrideSpecifier();
}))
return nullptr;
if (std::any_of(scope->nestedList.cbegin(), scope->nestedList.cend(), [](const Scope* nested) {
return nested->type == ScopeType::eUnion;
}))
return nullptr;
const Variable* member = nullptr;
for (const Variable& var : scope->varlist) {
if (var.isStatic())
continue;
if (member || !var.isPublic() || var.isArray() || var.isPointer() || var.isReference() ||
var.isRValueReference() || var.isVolatile() || var.hasDefault())
return nullptr;
member = &var;
}
return member;
}

const Variable* getSingleMemberArrowWriteTarget(const Token* tok)

@danmar danmar Oct 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an AI review. Take it with a grain of salt and feel free to reject it by resolving the comment.

I am worried that the uninitvar part is tailored to the exact example in the ticket. I built the PR and tried small variations of it:

struct P2 { int m_place; int m_other; };
struct It2 { P2 m_ptr; P2* operator->() { return &m_ptr; } };
It2 f2() { It2 it; it->m_place = 0; it->m_other = 0; return it; }   // still uninitvar + uninitStructMember

struct P3 { int m_place; };
struct It3 { P3 m_ptr; int m_extra; P3* operator->() { return &m_ptr; } };
It3 f3() { It3 it; it->m_place = 0; it.m_extra = 0; return it; }    // still uninitvar + uninitStructMember

Only the variant with exactly one member in both the iterator and the pointee is fixed. That adds about 80 lines of very specific shape matching to astutils, plus hooks in ValueFlowAnalyzer::analyzeMatch(), MemberExpressionAnalyzer and two places in checkuninitvar. That is a lot of special-casing in core code for one layout.

Maybe split the PR? The checkstl eraseDereference part is self-contained. For the uninit part, a more general approach could map it->x to it.m_ptr.x when operator-> is known to return &m_ptr, so the existing member tracking handles any layout. Alternatively, treat a call to a non-const user operator-> on an uninitialized object like other non-const member calls.

{
if (!Token::Match(tok, ". %name%") || tok->originalName() != "->" || !tok->astOperand1())
return nullptr;
// Exclude bindings, conditional/unevaluated operands and nested writes
// whose evaluation order is not established by this projection.
const Token* operation = tok->astParent();
if (!operation || operation->astParent() || operation->astOperand1() != tok ||
(!operation->isAssignmentOp() && !Token::Match(operation, "++|--")))
return nullptr;
const Variable* receiver = tok->astOperand1()->variable();
if (!receiver || !receiver->isLocal() || receiver->isPointer() || receiver->isArray() || receiver->isReference() ||
receiver->isRValueReference() || receiver->isVolatile())
return nullptr;
// A deferred lambda body does not initialize a captured outer object.
for (const Scope* enclosing = tok->scope(); enclosing && enclosing != receiver->scope(); enclosing = enclosing->nestedIn) {
if (enclosing->type == ScopeType::eLambda || enclosing->type == ScopeType::eFunction)
return nullptr;
}
const Scope* scope = receiver->typeScope();
const Variable* member = singlePlainDataMember(scope);
if (!member)
return nullptr;
const Variable* leaf = singlePlainDataMember(member->typeScope());
if (!leaf || !leaf->valueType() || !leaf->valueType()->isPrimitive() ||
tok->astOperand2() != tok->next() || tok->strAt(1) != leaf->name() ||
(tok->next()->variable() && tok->next()->variable() != leaf))
return nullptr;

const auto operators = scope->functionMap.equal_range("operator->");
if (operators.first == operators.second)
return nullptr;
for (auto it = operators.first; it != operators.second; ++it) {
const Function* function = it->second;
if (!function->functionScope || !Function::returnsPointer(function) ||
function->retType != member->type() || function->argCount() != 0 || function->isVolatile())
return nullptr;
const Token* body = function->functionScope->bodyStart;
if (!Token::simpleMatch(body, "{ return &") || !body->tokAt(2)->isUnaryOp("&"))
return nullptr;
const Token* memberToken = body->tokAt(3);
if (Token::simpleMatch(memberToken, "this ."))
memberToken = memberToken->tokAt(2);
if (!Token::Match(memberToken, "%var% ; }") || memberToken->variable() != member ||
memberToken->tokAt(2) != function->functionScope->bodyEnd)
return nullptr;
}

// A member, free or friend operator& can change the returned address.
// Keep the proof independent of overload resolution for address-of.
if (std::any_of(scope->symdb.scopeList.cbegin(), scope->symdb.scopeList.cend(), [](const Scope& candidate) {
return candidate.functionMap.count("operator&") != 0;

@danmar danmar Oct 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an AI review. Take it with a grain of salt and feel free to reject it by resolving the comment.

Performance: this loops over the whole symdb.scopeList every time getSingleMemberArrowWriteTarget() gets this far. The function is called from ValueFlowAnalyzer::analyzeMatch(), from MemberExpressionAnalyzer::match() and twice from checkuninitvar, so the scan can repeat many times per file. If this is kept, maybe compute "has any operator&" once (checkstl.cpp in this PR already does a similar scan once per iterators() call).

}))
return nullptr;
return member;
}

ExprUsage getExprUsage(const Token* tok, int indirect, const Settings& settings)
{
const Token* parent = tok->astParent();
Expand Down
5 changes: 5 additions & 0 deletions lib/astutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,11 @@ bool isConstVarExpression(const Token* tok, const std::function<bool(const Token

bool isLeafDot(const Token* tok);

// Identify a standalone scalar write through a pure overloaded arrow when the
// receiver's only member contains only that scalar. This is not a general alias
// summary: reads, bindings, nested expressions and captured writes are excluded.
const Variable* getSingleMemberArrowWriteTarget(const Token* tok);

enum class ExprUsage : std::uint8_t { None, NotUsed, PassedByReference, Used, Inconclusive };

ExprUsage getExprUsage(const Token* tok, int indirect, const Settings& settings);
Expand Down
51 changes: 50 additions & 1 deletion lib/checkstl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,38 @@ static bool isIterator(const Variable *var, bool& inconclusiveType)
return true;
}

static bool iteratorArrowReturnsMemberAddress(const Variable* var)
{
const Scope* scope = var->typeScope();
if (!scope || !var->type()->derivedFrom.empty())
return false;
const auto operators = scope->functionMap.equal_range("operator->");
if (operators.first == operators.second)
return false;

// Do not assume which cv/ref-qualified overload is selected.
for (auto it = operators.first; it != operators.second; ++it) {
const Function* function = it->second;
if (!function->functionScope || !Function::returnsPointer(function))
return false;
const Token* body = function->functionScope->bodyStart;
if (!Token::simpleMatch(body, "{ return &"))
return false;
const Token* memberToken = body->tokAt(3);
if (Token::simpleMatch(memberToken, "this ."))
memberToken = memberToken->tokAt(2);
if (!Token::Match(memberToken, "%var% ; }") || memberToken->tokAt(2) != function->functionScope->bodyEnd)
return false;
const Variable* member = memberToken->variable();
if (!member || member->scope() != scope || !member->isMember() || member->isStatic() ||
member->isPointer() || member->isReference() || member->isRValueReference())
return false;
if (member->type() && member->type()->getFunction("operator&"))
return false;
}
return true;
}

static std::string getContainerName(const Token *containerToken)
{
if (!containerToken)
Expand Down Expand Up @@ -455,6 +487,20 @@ void CheckStlImpl::iterators()

const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();

// A free or friend unary operator& can change the meaning of returning &member.
bool hasNonMemberAddressOperator = false;
for (const Scope& scope : symbolDatabase->scopeList) {
const auto operators = scope.functionMap.equal_range("operator&");
for (auto it = operators.first; it != operators.second; ++it) {
if (it->second->argCount() == 1 && (it->second->isFriend() || !scope.isClassOrStructOrUnion())) {
hasNonMemberAddressOperator = true;
break;
}
}
if (hasNonMemberAddressOperator)
break;
}

// Filling map of iterators id and their scope begin
std::map<int, const Token*> iteratorScopeBeginInfo;
for (const Variable* var : symbolDatabase->variableList()) {
Expand Down Expand Up @@ -615,7 +661,10 @@ void CheckStlImpl::iterators()
dereferenceErasedError(eraseToken, tok2, tok2->strAt(1), inconclusiveType);
tok2 = tok2->next();
} else if (!validIterator && Token::Match(tok2, "%varid% . %name%", iteratorId)) {
dereferenceErasedError(eraseToken, tok2, tok2->str(), inconclusiveType);
// A known operator-> can expose the iterator object's own storage before assignment.
if (eraseToken || !inconclusiveType || tok2->next()->originalName() != "->" || hasNonMemberAddressOperator ||
!iteratorArrowReturnsMemberAddress(var))
dereferenceErasedError(eraseToken, tok2, tok2->str(), inconclusiveType);
tok2 = tok2->tokAt(2);
}

Expand Down
14 changes: 13 additions & 1 deletion lib/checkuninitvar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1441,6 +1441,12 @@

bool CheckUninitVarImpl::isMemberVariableAssignment(const Token *tok, const std::string &membervar) const
{
if (const Variable* member = getSingleMemberArrowWriteTarget(tok->astParent())) {
const Token* access = tok->astParent();
if (access->astOperand1() == tok && member->name() == membervar &&
Token::simpleMatch(access->astParent(), "=") && astIsLHS(access))
return true;
}
if (Token::Match(tok, "%name% . %name%") && tok->strAt(2) == membervar) {
if (Token::Match(tok->tokAt(3), "[=.[]"))
return true;
Expand Down Expand Up @@ -1671,7 +1677,13 @@
(tok->astParent()->next()->variable() || tok->astParent()->next()->isEnumerator()))
continue;
}
const ExprUsage usage = getExprUsage(tok, v->indirect, mSettings);
// For a proven singleton accessor, the scalar operation also
// describes the receiver's initialization state (including ++).
const Token* usageToken = tok;
if (v->indirect == 0 && getSingleMemberArrowWriteTarget(tok->astParent()) &&
tok->astParent()->astOperand1() == tok)
usageToken = tok->astParent();
const ExprUsage usage = getExprUsage(usageToken, v->indirect, mSettings);
if (usage == ExprUsage::NotUsed || usage == ExprUsage::Inconclusive)
continue;
if (!v->subexpressions.empty() && usage == ExprUsage::PassedByReference)
Expand Down
10 changes: 10 additions & 0 deletions lib/vf_analyzers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,14 @@ struct ValueFlowAnalyzer : Analyzer {

Action analyzeMatch(const Token* tok, Direction d) const {
const Token* parent = tok->astParent();
const ValueFlow::Value* value = getValue(tok);
if (value && value->isUninitValue() && value->indirect == 0 &&
getSingleMemberArrowWriteTarget(parent) && parent->astOperand1() == tok) {
// The accessor only takes an address. For this singleton layout,
// the selected scalar and the receiver have the same init state.
const Token* operation = parent->astParent();
return operation->str() == "=" ? Action::Invalid : Action::Read | Action::Invalid;
}
if (d == Direction::Reverse && isGlobal() && !dependsOnThis() && Token::Match(parent, ". %name% (")) {
Action a = isGlobalModified(parent->next());
if (a != Action::None)
Expand Down Expand Up @@ -1505,6 +1513,8 @@ struct MemberExpressionAnalyzer : SubExpressionAnalyzer {
{
if (!Token::Match(tok, ". %var%"))
return false;
if (const Variable* member = getSingleMemberArrowWriteTarget(tok))
return !exact || member->name() == varname;
if (!exact)
return true;
return tok->strAt(1) == varname;
Expand Down
Loading
Loading