From 0b9e823fc8ce8eb7c53e61d2fa42750e2e32eca8 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Fri, 30 Aug 2019 11:32:45 -0500 Subject: [PATCH] Fix issue 9305: False positive uninitvar - struct initialized via function (#2123) --- lib/astutils.cpp | 26 ++++++------ lib/astutils.h | 10 ++--- lib/checkother.cpp | 2 +- lib/checkuninitvar.cpp | 2 +- lib/valueflow.cpp | 78 ++++++++++++++++++++++++------------ test/testastutils.cpp | 2 +- test/testsimplifytypedef.cpp | 5 ++- test/testuninitvar.cpp | 16 ++++++++ 8 files changed, 94 insertions(+), 47 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index fa595ab01..19e1e46e1 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -910,14 +910,14 @@ bool isReturnScope(const Token * const endToken, const Settings * settings, bool return false; } -bool isVariableChangedByFunctionCall(const Token *tok, nonneg int varid, const Settings *settings, bool *inconclusive) +bool isVariableChangedByFunctionCall(const Token *tok, int indirect, nonneg int varid, const Settings *settings, bool *inconclusive) { if (!tok) return false; if (tok->varId() == varid) - return isVariableChangedByFunctionCall(tok, settings, inconclusive); - return isVariableChangedByFunctionCall(tok->astOperand1(), varid, settings, inconclusive) || - isVariableChangedByFunctionCall(tok->astOperand2(), varid, settings, inconclusive); + return isVariableChangedByFunctionCall(tok, indirect, settings, inconclusive); + return isVariableChangedByFunctionCall(tok->astOperand1(), indirect, varid, settings, inconclusive) || + isVariableChangedByFunctionCall(tok->astOperand2(), indirect, varid, settings, inconclusive); } static bool isScopeBracket(const Token *tok) @@ -933,7 +933,7 @@ static bool isScopeBracket(const Token *tok) return false; } -bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings, bool *inconclusive) +bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Settings *settings, bool *inconclusive) { if (!tok) return false; @@ -1038,7 +1038,7 @@ bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings, const Variable *arg = tok->function()->getArgumentVar(argnr); - if (addressOf) { + if (addressOf || (indirect > 0 && arg && arg->isPointer())) { if (!(arg && arg->isConst())) return true; // If const is applied to the pointer, then the value can still be modified @@ -1049,7 +1049,7 @@ bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings, return arg && !arg->isConst() && arg->isReference(); } -bool isVariableChanged(const Token *tok, const Settings *settings, bool cpp, int depth) +bool isVariableChanged(const Token *tok, int indirect, const Settings *settings, bool cpp, int depth) { if (!tok) return false; @@ -1104,7 +1104,7 @@ bool isVariableChanged(const Token *tok, const Settings *settings, bool cpp, int while (Token::Match(ptok->astParent(), ".|::|[")) ptok = ptok->astParent(); bool inconclusive = false; - bool isChanged = isVariableChangedByFunctionCall(ptok, settings, &inconclusive); + bool isChanged = isVariableChangedByFunctionCall(ptok, indirect, settings, &inconclusive); isChanged |= inconclusive; if (isChanged) return true; @@ -1132,10 +1132,10 @@ bool isVariableChanged(const Token *tok, const Settings *settings, bool cpp, int bool isVariableChanged(const Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth) { - return findVariableChanged(start, end, varid, globalvar, settings, cpp, depth) != nullptr; + return findVariableChanged(start, end, 0, varid, globalvar, settings, cpp, depth) != nullptr; } -Token* findVariableChanged(Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth) +Token* findVariableChanged(Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth) { if (!precedes(start, end)) return nullptr; @@ -1148,15 +1148,15 @@ Token* findVariableChanged(Token *start, const Token *end, const nonneg int vari return tok; continue; } - if (isVariableChanged(tok, settings, cpp, depth)) + if (isVariableChanged(tok, indirect, settings, cpp, depth)) return tok; } return nullptr; } -const Token* findVariableChanged(const Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth) +const Token* findVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth) { - return findVariableChanged(const_cast(start), end, varid, globalvar, settings, cpp, depth); + return findVariableChanged(const_cast(start), end, indirect, varid, globalvar, settings, cpp, depth); } bool isVariableChanged(const Variable * var, const Settings *settings, bool cpp, int depth) diff --git a/lib/astutils.h b/lib/astutils.h index de78da1bf..5e40dd475 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -129,7 +129,7 @@ bool isReturnScope(const Token *endToken, const Settings * settings = nullptr, b * @param settings program settings * @param inconclusive pointer to output variable which indicates that the answer of the question is inconclusive */ -bool isVariableChangedByFunctionCall(const Token *tok, nonneg int varid, const Settings *settings, bool *inconclusive); +bool isVariableChangedByFunctionCall(const Token *tok, int indirect, nonneg int varid, const Settings *settings, bool *inconclusive); /** Is variable changed by function call? * In case the answer of the question is inconclusive, e.g. because the function declaration is not known @@ -139,17 +139,17 @@ bool isVariableChangedByFunctionCall(const Token *tok, nonneg int varid, const S * @param settings program settings * @param inconclusive pointer to output variable which indicates that the answer of the question is inconclusive */ -bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings, bool *inconclusive); +bool isVariableChangedByFunctionCall(const Token *tok, int indirect, const Settings *settings, bool *inconclusive); /** Is variable changed in block of code? */ bool isVariableChanged(const Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); -bool isVariableChanged(const Token *tok, const Settings *settings, bool cpp, int depth = 20); +bool isVariableChanged(const Token *tok, int indirect, const Settings *settings, bool cpp, int depth = 20); bool isVariableChanged(const Variable * var, const Settings *settings, bool cpp, int depth = 20); -const Token* findVariableChanged(const Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); -Token* findVariableChanged(Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); +const Token* findVariableChanged(const Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); +Token* findVariableChanged(Token *start, const Token *end, int indirect, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth = 20); bool isAliased(const Variable *var); diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 939b4e5a3..d321299b1 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2719,7 +2719,7 @@ void CheckOther::checkAccessOfMovedVariable() else inconclusive = true; } else { - const bool isVariableChanged = isVariableChangedByFunctionCall(tok, mSettings, &inconclusive); + const bool isVariableChanged = isVariableChangedByFunctionCall(tok, 0, mSettings, &inconclusive); accessOfMoved = !isVariableChanged && checkUninitVar.isVariableUsage(tok, false, CheckUninitVar::NO_ALLOC); if (inconclusive) { accessOfMoved = !isMovedParameterAllowedForInconclusiveFunction(tok); diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index bd101a77d..7dc76b3ca 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -1357,7 +1357,7 @@ void CheckUninitVar::valueFlowUninit() const bool isleaf = isLeafDot(tok) || uninitderef; if (Token::Match(tok->astParent(), ". %var%") && !isleaf) continue; - if (!Token::Match(tok->astParent(), ". %name% (") && !uninitderef && isVariableChanged(tok, mSettings, mTokenizer->isCPP())) + if (!Token::Match(tok->astParent(), ". %name% (") && !uninitderef && isVariableChanged(tok, v->indirect, mSettings, mTokenizer->isCPP())) continue; uninitvarError(tok, tok->str(), v->errorPath); const Token * nextTok = tok; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 040c4a37b..1e76109d5 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -169,11 +169,13 @@ static void bailoutInternal(TokenList *tokenlist, ErrorLogger *errorLogger, cons #define bailout(tokenlist, errorLogger, tok, what) bailoutInternal(tokenlist, errorLogger, tok, what, __FILE__, __LINE__, "(valueFlow)") #endif -static void changeKnownToPossible(std::list &values) +static void changeKnownToPossible(std::list &values, int indirect=-1) { - std::list::iterator it; - for (it = values.begin(); it != values.end(); ++it) - it->changeKnownToPossible(); + for (ValueFlow::Value& v: values) { + if (indirect >= 0 && v.indirect != indirect) + continue; + v.changeKnownToPossible(); + } } /** @@ -1690,7 +1692,7 @@ static void valueFlowReverse(TokenList *tokenlist, // assigned by subfunction? bool inconclusive = false; - if (isVariableChangedByFunctionCall(tok2, settings, &inconclusive)) { + if (isVariableChangedByFunctionCall(tok2, std::max(val.indirect, val2.indirect), settings, &inconclusive)) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok2, "possible assignment of " + tok2->str() + " by subfunction"); break; @@ -2071,6 +2073,15 @@ static bool isAliasOf(const Variable * var, const Token *tok, nonneg int varid, return false; } +static std::set getIndirections(const std::list& values) +{ + std::set result; + std::transform(values.begin(), values.end(), std::inserter(result, result.end()), [](const ValueFlow::Value& v) { + return std::max(0, v.indirect); + }); + return result; +} + static bool valueFlowForward(Token * const startToken, const Token * const endToken, const Variable * const var, @@ -2204,16 +2215,21 @@ static bool valueFlowForward(Token * const startToken, // conditional block of code that assigns variable.. else if (!tok2->varId() && Token::Match(tok2, "%name% (") && Token::simpleMatch(tok2->linkAt(1), ") {")) { // is variable changed in condition? - Token* tokChanged = findVariableChanged(tok2->next(), tok2->next()->link(), varid, var->isGlobal(), settings, tokenlist->isCPP()); - if (tokChanged != nullptr) { - // Set the value before bailing - if (tokChanged->varId() == varid) { - for (const ValueFlow::Value &v : values) { - if (!v.isNonValue()) - continue; - setTokenValue(tokChanged, v, settings); + for(int i:getIndirections(values)) { + Token* tokChanged = findVariableChanged(tok2->next(), tok2->next()->link(), i, varid, var->isGlobal(), settings, tokenlist->isCPP()); + if (tokChanged != nullptr) { + // Set the value before bailing + if (tokChanged->varId() == varid) { + for (const ValueFlow::Value &v : values) { + if (!v.isNonValue()) + continue; + setTokenValue(tokChanged, v, settings); + } } + values.remove_if([&](const ValueFlow::Value& v) { return v.indirect == i; }); } + } + if (values.empty()) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok2, "variable " + var->name() + " valueFlowForward, assignment in condition"); return false; @@ -2534,8 +2550,10 @@ static bool valueFlowForward(Token * const startToken, Token *expr = (condValue.intvalue != 0) ? op2->astOperand1() : op2->astOperand2(); for (const ValueFlow::Value &v : values) valueFlowAST(expr, varid, v, settings); - if (isVariableChangedByFunctionCall(expr, varid, settings, nullptr)) - changeKnownToPossible(values); + if (isVariableChangedByFunctionCall(expr, 0, varid, settings, nullptr)) + changeKnownToPossible(values, 0); + if (isVariableChangedByFunctionCall(expr, 1, varid, settings, nullptr)) + changeKnownToPossible(values, 1); } else { for (const ValueFlow::Value &v : values) { const ProgramMemory programMemory(getProgramMemory(tok2, varid, v)); @@ -2730,16 +2748,24 @@ static bool valueFlowForward(Token * const startToken, } // assigned by subfunction? - bool inconclusive = false; - if (isVariableChangedByFunctionCall(tok2, settings, &inconclusive)) { + for(int i:getIndirections(values)) { + bool inconclusive = false; + if (isVariableChangedByFunctionCall(tok2, i, settings, &inconclusive)) { + values.remove_if([&](const ValueFlow::Value& v) { return v.indirect <= i; }); + } + if (inconclusive) { + for (ValueFlow::Value &v : values) { + if (v.indirect != i) + continue; + v.setInconclusive(); + } + } + } + if (values.empty()) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok2, "possible assignment of " + tok2->str() + " by subfunction"); return false; } - if (inconclusive) { - for (ValueFlow::Value &v : values) - v.setInconclusive(); - } if (tok2->strAt(1) == "." && tok2->next()->originalName() != "->") { if (settings->inconclusive) { for (ValueFlow::Value &v : values) @@ -2751,10 +2777,12 @@ static bool valueFlowForward(Token * const startToken, } } // Variable changed - if (isVariableChanged(tok2, settings, tokenlist->isCPP())) { - values.remove_if(std::mem_fn(&ValueFlow::Value::isUninitValue)); - } - } else if (isAliasOf(var, tok2, varid, values) && isVariableChanged(tok2, settings, tokenlist->isCPP())) { + for(int i:getIndirections(values)) { + // Remove unintialized values if modified + if (isVariableChanged(tok2, i, settings, tokenlist->isCPP())) + values.remove_if([&](const ValueFlow::Value& v) { return v.isUninitValue() && v.indirect <= i; }); + } + } else if (isAliasOf(var, tok2, varid, values) && isVariableChanged(tok2, 0, settings, tokenlist->isCPP())) { if (settings->debugwarnings) bailout(tokenlist, errorLogger, tok2, "Alias variable was modified."); // Bail at the end of the statement if its in an assignment diff --git a/test/testastutils.cpp b/test/testastutils.cpp index d6f899e31..951d21941 100644 --- a/test/testastutils.cpp +++ b/test/testastutils.cpp @@ -159,7 +159,7 @@ private: std::istringstream istr(code); tokenizer.tokenize(istr, "test.cpp"); const Token * const argtok = Token::findmatch(tokenizer.tokens(), pattern); - return ::isVariableChangedByFunctionCall(argtok, &settings, inconclusive); + return ::isVariableChangedByFunctionCall(argtok, 0, &settings, inconclusive); } void isVariableChangedByFunctionCall() { diff --git a/test/testsimplifytypedef.cpp b/test/testsimplifytypedef.cpp index e17647a9f..9420d1753 100644 --- a/test/testsimplifytypedef.cpp +++ b/test/testsimplifytypedef.cpp @@ -1150,7 +1150,10 @@ private: "}"; ASSERT_EQUALS(expected, tok(code, false)); - ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:28]: (debug) valueflow.cpp:3109:valueFlowFunctionReturn bailout: function return; nontrivial function body\n", errout.str()); + ASSERT_EQUALS_WITHOUT_LINENUMBERS( + "[test.cpp:28]: (debug) valueflow.cpp:3109:valueFlowFunctionReturn bailout: function return; nontrivial function body\n" + "[test.cpp:26]: (debug) valueflow.cpp::valueFlowForward bailout: possible assignment of s by subfunction\n" + , errout.str()); } void simplifyTypedef36() { diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index 8e21b77fa..8914e55a8 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -4411,6 +4411,22 @@ private: " return;\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + // # 9305 + valueFlowUninit("struct kf {\n" + " double x;\n" + "};\n" + "void set(kf* k) {\n" + " k->x = 0;\n" + "}\n" + "void cal() {\n" + " KF b;\n" + " KF* pb = &b;\n" + " set( pb);\n" + " if (pb->x)\n" + " return;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void uninitvar_memberfunction() {