diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp index 0f3328075..ff824ca8c 100644 --- a/lib/programmemory.cpp +++ b/lib/programmemory.cpp @@ -214,13 +214,36 @@ static void fillProgramMemoryFromAssignments(ProgramMemory& pm, const Token* tok } } +static void removeModifiedVars(ProgramMemory& pm, const Token* tok, const Token* origin) +{ + for (auto i = pm.values.begin(), last = pm.values.end(); i != last;) { + if (isVariableChanged(origin, tok, i->first, false, nullptr, true)) { + i = pm.values.erase(i); + } else { + ++i; + } + } +} + +static ProgramMemory getInitialProgramState(const Token* tok, + const Token* origin, + const std::unordered_map& vars = std::unordered_map{}) +{ + ProgramMemory pm; + if (origin) { + fillProgramMemoryFromConditions(pm, origin, nullptr); + const ProgramMemory state = pm; + fillProgramMemoryFromAssignments(pm, tok, state, vars); + removeModifiedVars(pm, tok, origin); + } + return pm; +} + ProgramMemory getProgramMemory(const Token *tok, nonneg int varid, const ValueFlow::Value &value) { ProgramMemory programMemory; - if (value.tokvalue) - fillProgramMemoryFromConditions(programMemory, value.tokvalue, nullptr); - if (value.condition) - fillProgramMemoryFromConditions(programMemory, value.condition, nullptr); + programMemory.replace(getInitialProgramState(tok, value.tokvalue)); + programMemory.replace(getInitialProgramState(tok, value.condition)); programMemory.setValue(varid, value); if (value.varId) programMemory.setIntValue(value.varId, value.varvalue); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index d5d4cf579..c23e606d5 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -2178,6 +2178,44 @@ static void valueFlowForwardExpression(Token* startToken, } } +static bool bifurcate(const Token* tok, const std::set& varids, const Settings* settings, int depth = 20) +{ + if (depth < 0) + return false; + if (!tok) + return true; + if (tok->hasKnownIntValue()) + return true; + if (Token::Match(tok, "%cop%")) + return bifurcate(tok->astOperand1(), varids, settings) && bifurcate(tok->astOperand2(), varids, settings, depth); + if (Token::Match(tok, "%var%")) { + if (varids.count(tok->varId()) > 0) + return true; + const Variable* var = tok->variable(); + if (!var) + return false; + const Token* start = var->declEndToken(); + if (!start) + return false; + if (Token::Match(start, "; %varid% =", var->declarationId())) + start = start->tokAt(2); + if (var->isConst() || + !isVariableChanged(start->next(), tok, var->declarationId(), var->isGlobal(), settings, true)) + return var->isArgument() || bifurcate(start->astOperand2(), varids, settings, depth - 1); + return false; + } + return false; +} + +static void addCondition(std::list& values, const Token* condTok, bool assume) +{ + for (ValueFlow::Value& v : values) + if (assume) + v.errorPath.emplace_back(condTok, "Assuming condition is true."); + else + v.errorPath.emplace_back(condTok, "Assuming condition is false."); +} + static bool valueFlowForwardVariable(Token* const startToken, const Token* const endToken, const Variable* const var, @@ -2393,9 +2431,21 @@ static bool valueFlowForwardVariable(Token* const startToken, falsevalues.push_back(v); } + bool unknown = false; + if (truevalues.empty() && falsevalues.empty() && condTok && + std::all_of( + condTok->values().begin(), condTok->values().end(), std::mem_fn(&ValueFlow::Value::isNonValue)) && + bifurcate(condTok, {varid}, settings)) { + truevalues = values; + addCondition(truevalues, condTok, true); + falsevalues = values; + addCondition(falsevalues, condTok, false); + unknown = true; + } if (!truevalues.empty() || !falsevalues.empty()) { + Token* tok3 = tok2; // '{' - const Token * const startToken1 = tok2->linkAt(1)->next(); + const Token* const startToken1 = tok3->linkAt(1)->next(); bool vfresult = valueFlowForwardVariable(startToken1->next(), startToken1->link(), @@ -2408,22 +2458,24 @@ static bool valueFlowForwardVariable(Token* const startToken, errorLogger, settings); - if (!condAlwaysFalse && isVariableChanged(startToken1, startToken1->link(), varid, var->isGlobal(), settings, tokenlist->isCPP())) { + if (!unknown && !condAlwaysFalse && + isVariableChanged( + startToken1, startToken1->link(), varid, var->isGlobal(), settings, tokenlist->isCPP())) { removeValues(values, truevalues); lowerToPossible(values); } // goto '}' - tok2 = startToken1->link(); + tok3 = startToken1->link(); - if (isEscapeScope(startToken1, tokenlist, true) || !vfresult) { + if (!unknown && (isEscapeScope(startToken1, tokenlist, true) || !vfresult)) { if (condAlwaysTrue) return false; removeValues(values, truevalues); } - if (Token::simpleMatch(tok2, "} else {")) { - const Token * const startTokenElse = tok2->tokAt(2); + if (Token::simpleMatch(tok3, "} else {")) { + const Token* const startTokenElse = tok3->tokAt(2); vfresult = valueFlowForwardVariable(startTokenElse->next(), startTokenElse->link(), @@ -2436,15 +2488,17 @@ static bool valueFlowForwardVariable(Token* const startToken, errorLogger, settings); - if (!condAlwaysTrue && isVariableChanged(startTokenElse, startTokenElse->link(), varid, var->isGlobal(), settings, tokenlist->isCPP())) { + if (!unknown && !condAlwaysTrue && + isVariableChanged( + startTokenElse, startTokenElse->link(), varid, var->isGlobal(), settings, tokenlist->isCPP())) { removeValues(values, falsevalues); lowerToPossible(values); } // goto '}' - tok2 = startTokenElse->link(); + tok3 = startTokenElse->link(); - if (isEscapeScope(startTokenElse, tokenlist, true) || !vfresult) { + if (!unknown && (isEscapeScope(startTokenElse, tokenlist, true) || !vfresult)) { if (condAlwaysFalse) return false; removeValues(values, falsevalues); @@ -2452,7 +2506,10 @@ static bool valueFlowForwardVariable(Token* const startToken, } if (values.empty()) return false; - continue; + if (!unknown) { + tok2 = tok3; + continue; + } } Token * const start = tok2->linkAt(1)->next(); @@ -6261,9 +6318,9 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase, valueFlowTerminatingCondition(tokenlist, symboldatabase, errorLogger, settings); valueFlowBeforeCondition(tokenlist, symboldatabase, errorLogger, settings); valueFlowAfterMove(tokenlist, symboldatabase, errorLogger, settings); - valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings); valueFlowAfterCondition(tokenlist, symboldatabase, errorLogger, settings); valueFlowInferCondition(tokenlist, settings); + valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings); valueFlowSwitchVariable(tokenlist, symboldatabase, errorLogger, settings); valueFlowForLoop(tokenlist, symboldatabase, errorLogger, settings); valueFlowSubFunction(tokenlist, errorLogger, settings); diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 3d2a05077..636e20c7c 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -3474,6 +3474,17 @@ private: " }\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + // #9541 + check("int f(int pos, int a) {\n" + " if (pos <= 0)\n" + " pos = 0;\n" + " else if (pos < a)\n" + " if(pos > 0)\n" + " --pos;\n" + " return pos;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:5]: (style) Condition 'pos>0' is always true\n", errout.str()); } void alwaysTrueContainer() { diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index 7aaa0408a..acaf9f826 100644 --- a/test/testnullpointer.cpp +++ b/test/testnullpointer.cpp @@ -88,6 +88,8 @@ private: TEST_CASE(nullpointer46); // #9441 TEST_CASE(nullpointer47); // #6850 TEST_CASE(nullpointer48); // #9196 + TEST_CASE(nullpointer49); // #7804 + TEST_CASE(nullpointer50); // #6462 TEST_CASE(nullpointer_addressOf); // address of TEST_CASE(nullpointerSwitch); // #2626 TEST_CASE(nullpointer_cast); // #4692 @@ -1448,6 +1450,19 @@ private: " g(0);\n" "}\n", true); ASSERT_EQUALS("", errout.str()); + + check("bool f(int*);\n" + "void g(int* x) {\n" + " bool b = f(x);\n" + " if (b) {\n" + " *x = 1;\n" + " }\n" + "}\n" + "void h() {\n" + " g(0);\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); } void nullpointer36() { @@ -1643,6 +1658,42 @@ private: ASSERT_EQUALS("", errout.str()); } + void nullpointer49() + { + check("void f(int *p, int n) {\n" + " int *q = 0;\n" + " if(n > 10) q = p;\n" + " *p +=2;\n" + " if(n < 120) *q+=12;\n" + "}\n"); + TODO_ASSERT_EQUALS("[test.cpp:5]: (warning) Possible null pointer dereference: q\n", "", errout.str()); + + check("void f(int *p, int n) {\n" + " int *q = 0;\n" + " if(n > 10) q = p;\n" + " *p +=2;\n" + " if(n > 10) *q+=12;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + + void nullpointer50() + { + check("void f(int *p, int a) {\n" + " if(!p) {\n" + " if(a > 0) {\n" + " if(a > 10){}\n" + " else {\n" + " *p = 0;\n" + " }\n" + " }\n" + " }\n" + "}\n"); + ASSERT_EQUALS( + "[test.cpp:2] -> [test.cpp:6]: (warning) Either the condition '!p' is redundant or there is possible null pointer dereference: p.\n", + errout.str()); + } + void nullpointer_addressOf() { // address of check("void f() {\n" " struct X *x = 0;\n" @@ -2834,7 +2885,9 @@ private: " if (a != 0)\n" " *p = 0;\n" "}", true); - TODO_ASSERT_EQUALS("[test.cpp:3]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", "", errout.str()); + ASSERT_EQUALS( + "[test.cpp:3]: (warning) Possible null pointer dereference if the default parameter value is used: p\n", + errout.str()); check("void f(int *p = 0) {\n" " p = a;\n" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 3dab3c61c..69b4949a2 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -1676,7 +1676,7 @@ private: " ++x;\n" " return 2 + x;\n" "}"; - ASSERT_EQUALS(false, testValueOfX(code, 4U, 123)); + ASSERT_EQUALS(true, testValueOfX(code, 4U, 123)); code = "void f() {\n" " int x = 1;\n" @@ -1839,7 +1839,7 @@ private: " else if (x && i == 1) {}\n" " }\n" "}\n"; - ASSERT_EQUALS(false, testValueOfX(code, 6U, 0)); + ASSERT_EQUALS(true, testValueOfX(code, 6U, 0)); // multivariables code = "void f(int a) {\n"