From 0335671b353000c25cff31f066efa539efa7b279 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sun, 5 Sep 2021 00:35:33 -0500 Subject: [PATCH] Fix 10450: regression, FP : Iterator 'iter' from different container 'l' are used together (#3436) --- lib/astutils.cpp | 23 +++++++++++++------- lib/astutils.h | 6 +++++- lib/valueflow.cpp | 54 +++++++++++++++++++++++++++++++++++++---------- test/teststl.cpp | 23 ++++++++++++++++++++ 4 files changed, 86 insertions(+), 20 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 1477a4395..f277ab488 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -807,7 +807,11 @@ static void followVariableExpressionError(const Token *tok1, const Token *tok2, errors->push_back(item); } -std::vector followAllReferences(const Token* tok, bool inconclusive, ErrorPath errors, int depth) +std::vector followAllReferences(const Token* tok, + bool temporary, + bool inconclusive, + ErrorPath errors, + int depth) { struct ReferenceTokenLess { bool operator()(const ReferenceToken& x, const ReferenceToken& y) const { @@ -831,10 +835,11 @@ std::vector followAllReferences(const Token* tok, bool inconclus } else if (Token::simpleMatch(var->declEndToken(), "=")) { errors.emplace_back(var->declEndToken(), "Assigned to reference."); const Token *vartok = var->declEndToken()->astOperand2(); - if (vartok == tok) + if (vartok == tok || (!temporary && isTemporary(true, vartok, nullptr, true) && + (var->isConst() || var->isRValueReference()))) return {{tok, std::move(errors)}}; if (vartok) - return followAllReferences(vartok, inconclusive, std::move(errors), depth - 1); + return followAllReferences(vartok, temporary, inconclusive, std::move(errors), depth - 1); } else { return {{tok, std::move(errors)}}; } @@ -844,9 +849,9 @@ std::vector followAllReferences(const Token* tok, bool inconclus const Token* tok2 = tok->astOperand2(); std::vector refs; - refs = followAllReferences(tok2->astOperand1(), inconclusive, errors, depth - 1); + refs = followAllReferences(tok2->astOperand1(), temporary, inconclusive, errors, depth - 1); result.insert(refs.begin(), refs.end()); - refs = followAllReferences(tok2->astOperand2(), inconclusive, errors, depth - 1); + refs = followAllReferences(tok2->astOperand2(), temporary, inconclusive, errors, depth - 1); result.insert(refs.begin(), refs.end()); if (!inconclusive && result.size() != 1) @@ -865,7 +870,8 @@ std::vector followAllReferences(const Token* tok, bool inconclus for (const Token* returnTok : returns) { if (returnTok == tok) continue; - for (const ReferenceToken& rt:followAllReferences(returnTok, inconclusive, errors, depth - returns.size())) { + for (const ReferenceToken& rt : + followAllReferences(returnTok, temporary, inconclusive, errors, depth - returns.size())) { const Variable* argvar = rt.token->variable(); if (!argvar) return {{tok, std::move(errors)}}; @@ -880,7 +886,8 @@ std::vector followAllReferences(const Token* tok, bool inconclus ErrorPath er = errors; er.emplace_back(returnTok, "Return reference."); er.emplace_back(tok->previous(), "Called function passing '" + argTok->expressionString() + "'."); - std::vector refs = followAllReferences(argTok, inconclusive, std::move(er), depth - returns.size()); + std::vector refs = + followAllReferences(argTok, temporary, inconclusive, std::move(er), depth - returns.size()); result.insert(refs.begin(), refs.end()); if (!inconclusive && result.size() > 1) return {{tok, std::move(errors)}}; @@ -898,7 +905,7 @@ const Token* followReferences(const Token* tok, ErrorPath* errors) { if (!tok) return nullptr; - std::vector refs = followAllReferences(tok, false); + std::vector refs = followAllReferences(tok, true, false); if (refs.size() == 1) { if (errors) *errors = refs.front().errors; diff --git a/lib/astutils.h b/lib/astutils.h index e7a34aa64..452c9092e 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -145,7 +145,11 @@ struct ReferenceToken { ErrorPath errors; }; -std::vector followAllReferences(const Token* tok, bool inconclusive = true, ErrorPath errors = ErrorPath{}, int depth = 20); +std::vector followAllReferences(const Token* tok, + bool temporary = true, + bool inconclusive = true, + ErrorPath errors = ErrorPath{}, + int depth = 20); const Token* followReferences(const Token* tok, ErrorPath* errors = nullptr); bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2, const Library& library, bool pure, bool followVar, ErrorPath* errors=nullptr); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 62f62596f..e0a79627a 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -3917,21 +3917,53 @@ static void valueFlowLifetime(TokenList *tokenlist, SymbolDatabase*, ErrorLogger isContainerOfPointers = vt.pointer > 0; } - LifetimeStore ls; + ValueFlow::Value master; + master.valueType = ValueFlow::Value::ValueType::LIFETIME; + master.lifetimeScope = ValueFlow::Value::LifetimeScope::Local; - if (astIsIterator(parent->tokAt(2))) - ls = LifetimeStore{tok, "Iterator to container is created here.", ValueFlow::Value::LifetimeKind::Iterator}; - else if ((astIsPointer(parent->tokAt(2)) && !isContainerOfPointers) || Token::Match(parent->next(), "data|c_str")) - ls = LifetimeStore{tok, "Pointer to container is created here.", ValueFlow::Value::LifetimeKind::Object}; - else + if (astIsIterator(parent->tokAt(2))) { + master.errorPath.emplace_back(parent->tokAt(2), "Iterator to container is created here."); + master.lifetimeKind = ValueFlow::Value::LifetimeKind::Iterator; + } else if ((astIsPointer(parent->tokAt(2)) && !isContainerOfPointers) || + Token::Match(parent->next(), "data|c_str")) { + master.errorPath.emplace_back(parent->tokAt(2), "Pointer to container is created here."); + master.lifetimeKind = ValueFlow::Value::LifetimeKind::Object; + } else { continue; + } - // Dereferencing - if (tok->isUnaryOp("*") || parent->originalName() == "->") - ls.byDerefCopy(parent->tokAt(2), tokenlist, errorLogger, settings); - else - ls.byRef(parent->tokAt(2), tokenlist, errorLogger, settings); + std::vector toks = {}; + if (tok->isUnaryOp("*") || parent->originalName() == "->") { + for (const ValueFlow::Value& v : tok->values()) { + if (!v.isSymbolicValue()) + continue; + if (v.isKnown()) + continue; + if (v.intvalue != 0) + continue; + if (!v.tokvalue) + continue; + toks.push_back(v.tokvalue); + } + } else { + toks = {tok}; + } + for (const Token* tok2 : toks) { + for (const ReferenceToken& rt : followAllReferences(tok2, false)) { + ValueFlow::Value value = master; + value.tokvalue = rt.token; + value.errorPath.insert(value.errorPath.begin(), rt.errors.begin(), rt.errors.end()); + setTokenValue(parent->tokAt(2), value, tokenlist->getSettings()); + + if (!rt.token->variable()) { + LifetimeStore ls = LifetimeStore{ + rt.token, master.errorPath.back().second, ValueFlow::Value::LifetimeKind::Object}; + ls.byRef(parent->tokAt(2), tokenlist, errorLogger, settings); + } + } + } + valueFlowForwardLifetime(parent->tokAt(2), tokenlist, errorLogger, settings); } // Check constructors else if (Token::Match(tok, "=|return|%type%|%var% {")) { diff --git a/test/teststl.cpp b/test/teststl.cpp index deb11f336..e523690db 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -70,6 +70,7 @@ private: TEST_CASE(iterator25); // #9742 TEST_CASE(iterator26); // #9176 TEST_CASE(iterator27); // #10378 + TEST_CASE(iterator28); // #10450 TEST_CASE(iteratorExpression); TEST_CASE(iteratorSameExpression); TEST_CASE(mismatchingContainerIterator); @@ -1487,6 +1488,28 @@ private: ASSERT_EQUALS("", errout.str()); } + void iterator28() + { + // #10450 + check("struct S {\n" + " struct Private {\n" + " std::list l;\n" + " };\n" + " std::unique_ptr p;\n" + " int foo();\n" + "};\n" + "int S::foo() {\n" + " for(auto iter = p->l.begin(); iter != p->l.end(); ++iter) {\n" + " if(*iter == 1) {\n" + " p->l.erase(iter);\n" + " return 1;\n" + " }\n" + " }\n" + " return 0;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void iteratorExpression() { check("std::vector& f();\n" "std::vector& g();\n"