From c860de8565f047cdb509d8cfe817ed84c4739380 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sat, 23 Jan 2021 10:52:01 -0600 Subject: [PATCH] Fix issue 8143: valueFlowCondition: before and inside while (#3045) --- lib/astutils.cpp | 18 +++++++ lib/astutils.h | 5 ++ lib/reverseanalyzer.cpp | 15 ++++-- lib/reverseanalyzer.h | 1 + lib/valueflow.cpp | 105 ++++++++++++++++++++++++++------------- test/testnullpointer.cpp | 42 ++++++++++++++++ 6 files changed, 148 insertions(+), 38 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index f9720bec5..3bbe45267 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -81,6 +81,24 @@ const Token* findAstNode(const Token* ast, const std::function& pred) +{ + if (!precedes(start, end)) + return nullptr; + if (exprid == 0) + return nullptr; + for (const Token* tok = start; tok != end; tok = tok->next()) { + if (tok->exprId() != exprid) + continue; + if (pred(tok)) + return tok; + } + return nullptr; +} + static void astFlattenRecursive(const Token *tok, std::vector *result, const char* op, nonneg int depth = 0) { ++depth; diff --git a/lib/astutils.h b/lib/astutils.h index d506606e1..828d2296d 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -28,6 +28,7 @@ #include #include "errortypes.h" +#include "mathlib.h" #include "utils.h" class Function; @@ -52,6 +53,10 @@ void visitAstNodes(const Token *ast, std::function visitor); const Token* findAstNode(const Token* ast, const std::function& pred); +const Token* findExpression(MathLib::bigint exprid, + const Token* start, + const Token* end, + const std::function& pred); std::vector astFlatten(const Token* tok, const char* op); diff --git a/lib/reverseanalyzer.cpp b/lib/reverseanalyzer.cpp index 9130bb583..e894c554c 100644 --- a/lib/reverseanalyzer.cpp +++ b/lib/reverseanalyzer.cpp @@ -116,8 +116,11 @@ struct ReverseTraversal { return nullptr; } - void traverse(Token* start) { - for (Token* tok = start->previous(); tok; tok = tok->previous()) { + void traverse(Token* start, const Token* end = nullptr) + { + if (start == end) + return; + for (Token* tok = start->previous(); tok != end; tok = tok->previous()) { if (tok == start || (tok->str() == "{" && (tok->scope()->type == Scope::ScopeType::eFunction || tok->scope()->type == Scope::ScopeType::eLambda))) { break; @@ -171,7 +174,7 @@ struct ReverseTraversal { assignTok->astOperand2()->scope()->bodyEnd, a, settings); - valueFlowGenericReverse(assignTok->astOperand1()->previous(), a, settings); + valueFlowGenericReverse(assignTok->astOperand1()->previous(), end, a, settings); } } } @@ -284,3 +287,9 @@ void valueFlowGenericReverse(Token* start, const ValuePtr& a, const Se ReverseTraversal rt{a, settings}; rt.traverse(start); } + +void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr& a, const Settings* settings) +{ + ReverseTraversal rt{a, settings}; + rt.traverse(start, end); +} diff --git a/lib/reverseanalyzer.h b/lib/reverseanalyzer.h index 82a258148..8908fa2b9 100644 --- a/lib/reverseanalyzer.h +++ b/lib/reverseanalyzer.h @@ -26,5 +26,6 @@ template class ValuePtr; void valueFlowGenericReverse(Token* start, const ValuePtr& a, const Settings* settings); +void valueFlowGenericReverse(Token* start, const Token* end, const ValuePtr& a, const Settings* settings); #endif diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index b255bc723..fbccd7978 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2432,6 +2432,7 @@ static void valueFlowReverse(TokenList* tokenlist, } static void valueFlowReverse(Token* tok, + const Token* const endToken, const Token* const varToken, const std::list& values, TokenList* tokenlist, @@ -2442,16 +2443,25 @@ static void valueFlowReverse(Token* tok, auto aliases = getAliasesFromValues(values); for (const ValueFlow::Value& v : values) { VariableAnalyzer a(var, v, aliases, tokenlist); - valueFlowGenericReverse(tok, a, settings); + valueFlowGenericReverse(tok, endToken, a, settings); } } else { for (const ValueFlow::Value& v : values) { ExpressionAnalyzer a(varToken, v, tokenlist); - valueFlowGenericReverse(tok, a, settings); + valueFlowGenericReverse(tok, endToken, a, settings); } } } +static void valueFlowReverse(Token* tok, + const Token* const varToken, + const std::list& values, + TokenList* tokenlist, + const Settings* settings) +{ + valueFlowReverse(tok, nullptr, varToken, values, tokenlist, settings); +} + std::string lifetimeType(const Token *tok, const ValueFlow::Value *val) { std::string result; @@ -4058,6 +4068,7 @@ struct ConditionHandler { const Settings* settings) const = 0; virtual void reverse(Token* start, + const Token* endToken, const Token* exprTok, const std::list& values, TokenList* tokenlist, @@ -4139,6 +4150,8 @@ struct ConditionHandler { if (Token::Match(top, "%assign%")) return; + if (Token::Match(cond.vartok->astParent(), "%assign%|++|--")) + return; if (Token::simpleMatch(tok->astParent(), "?") && tok->astParent()->isExpandedMacro()) { if (settings->debugwarnings) @@ -4159,6 +4172,23 @@ struct ConditionHandler { return; } + std::list values = cond.true_values; + if (cond.true_values != cond.false_values) + values.insert(values.end(), cond.false_values.begin(), cond.false_values.end()); + + // extra logic for unsigned variables 'i>=1' => possible value can also be 0 + if (Token::Match(tok, "<|>")) { + values.remove_if([](const ValueFlow::Value& v) { + if (v.isIntValue()) + return v.intvalue != 0; + return false; + }); + if (cond.vartok->valueType() && cond.vartok->valueType()->sign != ValueType::Sign::UNSIGNED) + return; + } + if (values.empty()) + return; + // bailout: for/while-condition, variable is changed in while loop if (Token::Match(top->previous(), "for|while (") && Token::simpleMatch(top->link(), ") {")) { @@ -4177,40 +4207,31 @@ struct ConditionHandler { } // Variable changed in loop code - if (Token::Match(top->previous(), "for|while (")) { - const Token* const start = top; - const Token* const block = top->link()->next(); - const Token* const end = block->link(); + const Token* const start = top; + const Token* const block = top->link()->next(); + const Token* const end = block->link(); - if (isExpressionChanged(cond.vartok, start, end, settings, tokenlist->isCPP())) { - if (settings->debugwarnings) - bailout(tokenlist, - errorLogger, - tok, - "variable '" + cond.vartok->expressionString() + "' used in loop"); - return; + if (isExpressionChanged(cond.vartok, start, end, settings, tokenlist->isCPP())) { + // If its reassigned in loop then analyze from the end + if (!Token::Match(tok, "%assign%|++|--") && + findExpression(cond.vartok->exprId(), start, end, [&](const Token* tok2) { + return Token::Match(tok2->astParent(), "%assign%") && astIsLHS(tok2); + })) { + // Start at the end of the loop body + Token* bodyTok = top->link()->next(); + reverse(bodyTok->link(), bodyTok, cond.vartok, values, tokenlist, settings); } + if (settings->debugwarnings) + bailout(tokenlist, + errorLogger, + tok, + "variable '" + cond.vartok->expressionString() + "' used in loop"); + return; } } - std::list values = cond.true_values; - if (cond.true_values != cond.false_values) - values.insert(values.end(), cond.false_values.begin(), cond.false_values.end()); - - // extra logic for unsigned variables 'i>=1' => possible value can also be 0 - if (Token::Match(tok, "<|>")) { - values.remove_if([](const ValueFlow::Value& v) { - if (v.isIntValue()) - return v.intvalue != 0; - return false; - }); - if (cond.vartok->valueType() && cond.vartok->valueType()->sign != ValueType::Sign::UNSIGNED) - return; - } - if (values.empty()) - return; Token* startTok = tok->astParent() ? tok->astParent() : tok->previous(); - reverse(startTok, cond.vartok, values, tokenlist, settings); + reverse(startTok, nullptr, cond.vartok, values, tokenlist, settings); }); } @@ -4453,11 +4474,13 @@ struct SimpleConditionHandler : ConditionHandler { } virtual void reverse(Token* start, + const Token* endToken, const Token* exprTok, const std::list& values, TokenList* tokenlist, - const Settings* settings) const OVERRIDE { - return valueFlowReverse(start, exprTok, values, tokenlist, settings); + const Settings* settings) const OVERRIDE + { + return valueFlowReverse(start, endToken, exprTok, values, tokenlist, settings); } virtual Condition parse(const Token* tok, const Settings*) const OVERRIDE { @@ -5803,6 +5826,7 @@ static Analyzer::Action valueFlowContainerForward(Token* tok, } static void valueFlowContainerReverse(Token* tok, + const Token* const endToken, const Token* const varToken, const std::list& values, TokenList* tokenlist, @@ -5812,10 +5836,19 @@ static void valueFlowContainerReverse(Token* tok, auto aliases = getAliasesFromValues(values); for (const ValueFlow::Value& value : values) { ContainerVariableAnalyzer a(var, value, aliases, tokenlist); - valueFlowGenericReverse(tok, a, settings); + valueFlowGenericReverse(tok, endToken, a, settings); } } +static void valueFlowContainerReverse(Token* tok, + const Token* const varToken, + const std::list& values, + TokenList* tokenlist, + const Settings* settings) +{ + valueFlowContainerReverse(tok, nullptr, varToken, values, tokenlist, settings); +} + static bool isContainerSizeChanged(const Token *tok, int depth) { if (!tok) @@ -6162,15 +6195,17 @@ struct ContainerConditionHandler : ConditionHandler { } virtual void reverse(Token* start, + const Token* endTok, const Token* exprTok, const std::list& values, TokenList* tokenlist, - const Settings* settings) const OVERRIDE { + const Settings* settings) const OVERRIDE + { if (values.empty()) return; if (!exprTok->variable()) return; - return valueFlowContainerReverse(start, exprTok, values, tokenlist, settings); + return valueFlowContainerReverse(start, endTok, exprTok, values, tokenlist, settings); } virtual Condition parse(const Token* tok, const Settings*) const OVERRIDE { diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index d7dbf9c01..22e39a360 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -109,6 +109,7 @@ private: TEST_CASE(nullpointer66); // #10024 TEST_CASE(nullpointer67); // #10062 TEST_CASE(nullpointer68); + TEST_CASE(nullpointer69); // #8143 TEST_CASE(nullpointer_addressOf); // address of TEST_CASE(nullpointerSwitch); // #2626 TEST_CASE(nullpointer_cast); // #4692 @@ -2135,6 +2136,47 @@ private: ASSERT_EQUALS("", errout.str()); } + void nullpointer69() + { + check("void f(const Scope *scope) {\n" + " if (scope->definedType) {}\n" + " while (scope) {\n" + " scope = scope->nestedIn;\n" + " enumerator = scope->findEnumerator();\n" + " }\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:5]: (warning) Either the condition 'scope' is redundant or there is possible null pointer dereference: scope.\n", + errout.str()); + + check("void f(const Scope *scope) {\n" + " if (scope->definedType) {}\n" + " while (scope && scope->nestedIn) {\n" + " if (scope->type == Scope::eFunction && scope->functionOf)\n" + " scope = scope->functionOf;\n" + " else\n" + " scope = scope->nestedIn;\n" + " enumerator = scope->findEnumerator();\n" + " }\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:3] -> [test.cpp:8]: (warning) Either the condition 'scope' is redundant or there is possible null pointer dereference: scope.\n", + errout.str()); + + check("struct a {\n" + " a *b() const;\n" + " void c();\n" + "};\n" + "void d() {\n" + " for (a *e;;) {\n" + " e->b()->c();\n" + " while (e)\n" + " e = e->b();\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void nullpointer_addressOf() { // address of check("void f() {\n" " struct X *x = 0;\n"