From af214e8212efa303e664920a468de00ee0b1fe3d Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Thu, 15 Aug 2019 03:44:55 -0500 Subject: [PATCH] Fix issue 8825: ValueFlow: uninitialized struct member (#2087) * Pass uninit value across pointers * Add more testing --- lib/astutils.cpp | 156 ++++++++++++++++++++++------------------- lib/astutils.h | 2 + lib/checkuninitvar.cpp | 22 ++++-- lib/checkuninitvar.h | 7 +- lib/valueflow.cpp | 18 +++-- test/testuninitvar.cpp | 52 ++++++++++++++ 6 files changed, 169 insertions(+), 88 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index fb051ce5c..174ee424a 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -995,6 +995,87 @@ 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) +{ + if (!tok) + return false; + const Token *tok2 = tok; + while (Token::simpleMatch(tok2->astParent(), "*") || (Token::simpleMatch(tok2->astParent(), ".") && !Token::simpleMatch(tok2->astParent()->astParent(), "(")) || + (Token::simpleMatch(tok2->astParent(), "[") && tok2 == tok2->astParent()->astOperand1())) + tok2 = tok2->astParent(); + + if (Token::Match(tok2->astParent(), "++|--")) + return true; + + if (tok2->astParent() && tok2->astParent()->isAssignmentOp()) { + if (tok2 == tok2->astParent()->astOperand1()) + return true; + // Check if assigning to a non-const lvalue + const Variable * var = getLHSVariable(tok2->astParent()); + if (var && var->isReference() && !var->isConst() && var->nameToken() && var->nameToken()->next() == tok2->astParent()) { + if (!var->isLocal() || isVariableChanged(var, settings, cpp, depth - 1)) + return true; + } + } + + if (isLikelyStreamRead(cpp, tok->previous())) + return true; + + if (isLikelyStream(cpp, tok2)) + return true; + + // Member function call + if (tok->variable() && Token::Match(tok2->astParent(), ". %name%") && isFunctionCall(tok2->astParent()->next()) && tok2->astParent()->astOperand1() == tok2) { + const Variable * var = tok->variable(); + bool isConst = var && var->isConst(); + if (!isConst && var) { + const ValueType * valueType = var->valueType(); + isConst = (valueType && valueType->pointer == 1 && valueType->constness == 1); + } + + const Token *ftok = tok->tokAt(2); + const Function * fun = ftok->function(); + if (!isConst && (!fun || !fun->isConst())) + return true; + else + return false; + } + + const Token *ftok = tok2; + while (ftok && (!Token::Match(ftok, "[({]") || ftok->isCast())) + ftok = ftok->astParent(); + + if (ftok && Token::Match(ftok->link(), ")|} !!{")) { + const Token * ptok = tok2; + while (Token::Match(ptok->astParent(), ".|::|[")) + ptok = ptok->astParent(); + bool inconclusive = false; + bool isChanged = isVariableChangedByFunctionCall(ptok, settings, &inconclusive); + isChanged |= inconclusive; + if (isChanged) + return true; + } + + const Token *parent = tok2->astParent(); + while (Token::Match(parent, ".|::")) + parent = parent->astParent(); + if (parent && parent->tokType() == Token::eIncDecOp) + return true; + + if (Token::simpleMatch(tok2->astParent(), ":") && tok2->astParent()->astParent() && Token::simpleMatch(tok2->astParent()->astParent()->previous(), "for (")) { + const Token * varTok = tok2->astParent()->previous(); + if (!varTok) + return false; + const Variable * loopVar = varTok->variable(); + if (!loopVar) + return false; + if (!loopVar->isConst() && loopVar->isReference() && isVariableChanged(loopVar, settings, cpp, depth - 1)) + return true; + return false; + } + return false; +} + bool isVariableChanged(const Token *start, const Token *end, const nonneg int varid, bool globalvar, const Settings *settings, bool cpp, int depth) { if (!precedes(start, end)) @@ -1008,81 +1089,8 @@ bool isVariableChanged(const Token *start, const Token *end, const nonneg int va return true; continue; } - - const Token *tok2 = tok; - while (Token::simpleMatch(tok2->astParent(), "*") || (Token::simpleMatch(tok2->astParent(), ".") && !Token::simpleMatch(tok2->astParent()->astParent(), "(")) || - (Token::simpleMatch(tok2->astParent(), "[") && tok2 == tok2->astParent()->astOperand1())) - tok2 = tok2->astParent(); - - if (Token::Match(tok2->astParent(), "++|--")) + if (isVariableChanged(tok, settings, cpp, depth)) return true; - - if (tok2->astParent() && tok2->astParent()->isAssignmentOp()) { - if (tok2 == tok2->astParent()->astOperand1()) - return true; - // Check if assigning to a non-const lvalue - const Variable * var = getLHSVariable(tok2->astParent()); - if (var && var->isReference() && !var->isConst() && var->nameToken() && var->nameToken()->next() == tok2->astParent()) { - if (!var->isLocal() || isVariableChanged(var, settings, cpp, depth - 1)) - return true; - } - } - - if (isLikelyStreamRead(cpp, tok->previous())) - return true; - - if (isLikelyStream(cpp, tok2)) - return true; - - // Member function call - if (tok->variable() && Token::Match(tok2->astParent(), ". %name%") && isFunctionCall(tok2->astParent()->next()) && tok2->astParent()->astOperand1() == tok2) { - const Variable * var = tok->variable(); - bool isConst = var && var->isConst(); - if (!isConst && var) { - const ValueType * valueType = var->valueType(); - isConst = (valueType && valueType->pointer == 1 && valueType->constness == 1); - } - - const Token *ftok = tok->tokAt(2); - const Function * fun = ftok->function(); - if (!isConst && (!fun || !fun->isConst())) - return true; - else - continue; - } - - const Token *ftok = tok2; - while (ftok && (!Token::Match(ftok, "[({]") || ftok->isCast())) - ftok = ftok->astParent(); - - if (ftok && Token::Match(ftok->link(), ")|} !!{")) { - const Token * ptok = tok2; - while (Token::Match(ptok->astParent(), ".|::|[")) - ptok = ptok->astParent(); - bool inconclusive = false; - bool isChanged = isVariableChangedByFunctionCall(ptok, settings, &inconclusive); - isChanged |= inconclusive; - if (isChanged) - return true; - } - - const Token *parent = tok2->astParent(); - while (Token::Match(parent, ".|::")) - parent = parent->astParent(); - if (parent && parent->tokType() == Token::eIncDecOp) - return true; - - if (Token::simpleMatch(tok2->astParent(), ":") && tok2->astParent()->astParent() && Token::simpleMatch(tok2->astParent()->astParent()->previous(), "for (")) { - const Token * varTok = tok2->astParent()->previous(); - if (!varTok) - continue; - const Variable * loopVar = varTok->variable(); - if (!loopVar) - continue; - if (!loopVar->isConst() && loopVar->isReference() && isVariableChanged(loopVar, settings, cpp, depth - 1)) - return true; - continue; - } } return false; } diff --git a/lib/astutils.h b/lib/astutils.h index 847c25417..e159d1339 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -135,6 +135,8 @@ bool isVariableChangedByFunctionCall(const Token *tok, const Settings *settings, /** 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 Variable * var, const Settings *settings, bool cpp, int depth = 20); bool isAliased(const Variable *var); diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index 4131fbc9c..cacec03e8 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -1269,9 +1269,11 @@ void CheckUninitVar::uninitdataError(const Token *tok, const std::string &varnam reportError(tok, Severity::error, "uninitdata", "$symbol:" + varname + "\nMemory is allocated but not initialized: $symbol", CWE908, false); } -void CheckUninitVar::uninitvarError(const Token *tok, const std::string &varname) +void CheckUninitVar::uninitvarError(const Token *tok, const std::string &varname, ErrorPath errorPath) { - reportError(tok, Severity::error, "uninitvar", "$symbol:" + varname + "\nUninitialized variable: $symbol", CWE908, false); + errorPath.emplace_back(tok, ""); + reportError(errorPath, Severity::error, "uninitvar", "$symbol:" + varname + "\nUninitialized variable: $symbol", CWE908, false); + // reportError(tok, Severity::error, "uninitvar", "$symbol:" + varname + "\nUninitialized variable: $symbol", CWE908, false); } void CheckUninitVar::uninitStructMemberError(const Token *tok, const std::string &membername) @@ -1295,14 +1297,20 @@ void CheckUninitVar::valueFlowUninit() tok = tok->linkAt(1); continue; } - if (!tok->variable() || tok->values().size() != 1U) + if (!tok->variable()) continue; - const ValueFlow::Value &v = tok->values().front(); - if (v.valueType != ValueFlow::Value::ValueType::UNINIT || v.isInconclusive()) + if (Token::simpleMatch(tok->astParent(), ".") && tok->astParent()->astOperand1() == tok) continue; - if (!isVariableUsage(tok, tok->variable()->isPointer(), NO_ALLOC)) + auto v = std::find_if(tok->values().begin(), tok->values().end(), std::mem_fn(&ValueFlow::Value::isUninitValue)); + if (v == tok->values().end()) continue; - uninitvarError(tok, tok->str()); + if (v->isInconclusive()) + continue; + if (!isVariableUsage(tok, tok->variable()->isPointer(), tok->variable()->isArray() ? ARRAY : NO_ALLOC)) + continue; + if (isVariableChanged(tok, mSettings, mTokenizer->isCPP())) + continue; + uninitvarError(tok, tok->str(), v->errorPath); } } } diff --git a/lib/checkuninitvar.h b/lib/checkuninitvar.h index 19f60ecb6..8d11842ee 100644 --- a/lib/checkuninitvar.h +++ b/lib/checkuninitvar.h @@ -109,7 +109,12 @@ public: void uninitstringError(const Token *tok, const std::string &varname, bool strncpy_); void uninitdataError(const Token *tok, const std::string &varname); - void uninitvarError(const Token *tok, const std::string &varname); + void uninitvarError(const Token *tok, const std::string &varname, ErrorPath errorPath); + void uninitvarError(const Token *tok, const std::string &varname) + { + ErrorPath errorPath; + uninitvarError(tok, varname, errorPath); + } void uninitvarError(const Token *tok, const std::string &varname, Alloc alloc) { if (alloc == NO_CTOR_CALL || alloc == CTOR_CALL) uninitdataError(tok, varname); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 272d1bcf4..c986eec76 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -399,10 +399,6 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti if (!tok->addValue(value)) return; - // Don't set parent for uninitialized values - if (value.isUninitValue()) - return; - Token *parent = tok->astParent(); if (!parent) return; @@ -459,6 +455,14 @@ static void setTokenValue(Token* tok, const ValueFlow::Value &value, const Setti return; } + if (value.isUninitValue()) { + if (parent->isUnaryOp("&")) + setTokenValue(parent, value, settings); + else if (Token::Match(parent, ". %var%") && parent->astOperand1() == tok) + setTokenValue(parent->astOperand2(), value, settings); + return; + } + // cast.. if (const Token *castType = getCastTypeStartToken(parent)) { const ValueType &valueType = ValueType::parseDecl(castType, settings); @@ -4987,8 +4991,8 @@ static void valueFlowUninit(TokenList *tokenlist, SymbolDatabase * /*symbolDatab pointer |= vardecl->str() == "*"; vardecl = vardecl->next(); } - if (!stdtype && !pointer) - continue; + // if (!stdtype && !pointer) + // continue; if (!Token::Match(vardecl, "%var% ;")) continue; if (Token::Match(vardecl, "%varid% ; %varid% =", vardecl->varId())) @@ -4999,6 +5003,8 @@ static void valueFlowUninit(TokenList *tokenlist, SymbolDatabase * /*symbolDatab if ((!var->isPointer() && var->type() && var->type()->needInitialization != Type::NeedInitialization::True) || !var->isLocal() || var->isStatic() || var->isExtern() || var->isReference() || var->isThrow()) continue; + if (!var->type() && !stdtype && !pointer) + continue; ValueFlow::Value uninitValue; uninitValue.setKnown(); diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index 3f947cda5..f958926e1 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -81,6 +81,7 @@ private: TEST_CASE(syntax_error); // Ticket #5073 TEST_CASE(trac_5970); TEST_CASE(valueFlowUninit); + TEST_CASE(uninitvar_ipa); TEST_CASE(isVariableUsageDeref); // *p @@ -3978,6 +3979,57 @@ private: ASSERT_EQUALS("", errout.str()); } + void uninitvar_ipa() { + // #8825 + valueFlowUninit("typedef struct {\n" + " int flags;\n" + "} someType_t;\n" + "void bar(const someType_t * const p) {\n" + " if( (p->flags & 0xF000) == 0xF000){}\n" + "}\n" + "void f(void) {\n" + " someType_t gVar;\n" + " bar(&gVar);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:9] -> [test.cpp:5]: (error) Uninitialized variable: flags\n", errout.str()); + + valueFlowUninit("typedef struct \n" + "{\n" + " int flags[3];\n" + "} someType_t;\n" + "void f(void) {\n" + " someType_t gVar;\n" + " if(gVar.flags[1] == 42){}\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:7]: (error) Uninitialized variable: flags\n", errout.str()); + + valueFlowUninit("void f(bool * x) {\n" + " *x = false;\n" + "}\n" + "void g() {\n" + " bool b;\n" + " f(&b);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("struct A { bool b; };" + "void f(A * x) {\n" + " x->b = false;\n" + "}\n" + "void g() {\n" + " A b;\n" + " f(&b);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("std::string f() {\n" + " std::ostringstream ostr;\n" + " ostr << \"\";\n" + " return ostr.str();\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void isVariableUsageDeref() { // *p checkUninitVar("void f() {\n"