From 1a5449cbeb69440f995eb067cb7ab45c98bf05f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 1 Jul 2021 22:08:00 +0200 Subject: [PATCH] Fixed #10327 (ValueFlow; Wrong Uninit value in called function) --- lib/checkuninitvar.cpp | 32 +++++++++++++++++++++----------- lib/checkuninitvar.h | 2 ++ lib/valueflow.cpp | 12 ++++++++++++ test/testuninitvar.cpp | 18 ++++++++++++++++++ 4 files changed, 53 insertions(+), 11 deletions(-) diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index 65c1da3b5..79436317b 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -1093,7 +1093,7 @@ static bool isVoidCast(const Token *tok) return Token::simpleMatch(tok, "(") && tok->isCast() && tok->valueType() && tok->valueType()->type == ValueType::Type::VOID && tok->valueType()->pointer == 0; } -const Token* CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer, Alloc alloc, int indirect) const +const Token* CheckUninitVar::isVariableUsage(bool cpp, const Token *vartok, const Library& library, bool pointer, Alloc alloc, int indirect) { const Token *valueExpr = vartok; // non-dereferenced , no address of value as variable while (Token::Match(valueExpr->astParent(), ".|::") && astIsRhs(valueExpr)) @@ -1197,17 +1197,17 @@ const Token* CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer, parent = parent->astParent(); if (Token::simpleMatch(parent, "{")) return valueExpr; - const int use = isFunctionParUsage(valueExpr, pointer, alloc, indirect); + const int use = isFunctionParUsage(valueExpr, library, pointer, alloc, indirect); return (use>0) ? valueExpr : nullptr; } if (derefValue && Token::Match(derefValue->astParent(), "[(,]") && (derefValue->astParent()->str() == "," || astIsRhs(derefValue))) { - const int use = isFunctionParUsage(derefValue, false, NO_ALLOC, indirect); + const int use = isFunctionParUsage(derefValue, library, false, NO_ALLOC, indirect); return (use>0) ? derefValue : nullptr; } if (valueExpr->astParent()->isUnaryOp("&")) { const Token *parent = valueExpr->astParent(); if (Token::Match(parent->astParent(), "[(,]") && (parent->astParent()->str() == "," || astIsRhs(parent))) { - const int use = isFunctionParUsage(valueExpr, pointer, alloc, indirect); + const int use = isFunctionParUsage(valueExpr, library, pointer, alloc, indirect); return (use>0) ? valueExpr : nullptr; } return nullptr; @@ -1253,18 +1253,18 @@ const Token* CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer, // Stream read/write // FIXME this code is a hack!! - if (mTokenizer->isCPP() && Token::Match(valueExpr->astParent(), "<<|>>")) { - if (isLikelyStreamRead(mTokenizer->isCPP(), vartok->previous())) + if (cpp && Token::Match(valueExpr->astParent(), "<<|>>")) { + if (isLikelyStreamRead(cpp, vartok->previous())) return nullptr; if (valueExpr->valueType() && valueExpr->valueType()->type == ValueType::Type::VOID) return nullptr; } - if (astIsRhs(derefValue) && isLikelyStreamRead(mTokenizer->isCPP(), derefValue->astParent())) + if (astIsRhs(derefValue) && isLikelyStreamRead(cpp, derefValue->astParent())) return nullptr; // Assignment with overloaded & - if (mTokenizer->isCPP() && Token::simpleMatch(valueExpr->astParent(), "&") && astIsRhs(valueExpr)) { + if (cpp && Token::simpleMatch(valueExpr->astParent(), "&") && astIsRhs(valueExpr)) { const Token *parent = valueExpr->astParent(); while (Token::simpleMatch(parent, "&") && parent->isBinaryOp()) parent = parent->astParent(); @@ -1280,13 +1280,18 @@ const Token* CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer, return derefValue ? derefValue : valueExpr; } +const Token* CheckUninitVar::isVariableUsage(const Token *vartok, bool pointer, Alloc alloc, int indirect) const +{ + return CheckUninitVar::isVariableUsage(mTokenizer->isCPP(), vartok, mSettings->library, pointer, alloc, indirect); +} + /*** * Is function parameter "used" so a "usage of uninitialized variable" can * be written? If parameter is passed "by value" then it is "used". If it * is passed "by reference" then it is not necessarily "used". * @return -1 => unknown 0 => not used 1 => used */ -int CheckUninitVar::isFunctionParUsage(const Token *vartok, bool pointer, Alloc alloc, int indirect) const +int CheckUninitVar::isFunctionParUsage(const Token *vartok, const Library& library, bool pointer, Alloc alloc, int indirect) { bool unknown = false; const Token *parent = getAstParentSkipPossibleCastAndAddressOf(vartok, &unknown); @@ -1339,11 +1344,11 @@ int CheckUninitVar::isFunctionParUsage(const Token *vartok, bool pointer, Alloc // control-flow statement reading the variable "by value" return alloc == NO_ALLOC; } else { - const bool isnullbad = mSettings->library.isnullargbad(start->previous(), argumentNumber + 1); + const bool isnullbad = library.isnullargbad(start->previous(), argumentNumber + 1); if (indirect == 0 && pointer && !address && isnullbad && alloc == NO_ALLOC) return 1; bool hasIndirect = false; - const bool isuninitbad = mSettings->library.isuninitargbad(start->previous(), argumentNumber + 1, indirect, &hasIndirect); + const bool isuninitbad = library.isuninitargbad(start->previous(), argumentNumber + 1, indirect, &hasIndirect); if (alloc != NO_ALLOC) return (isnullbad || hasIndirect) && isuninitbad; return isuninitbad && (!address || isnullbad); @@ -1354,6 +1359,11 @@ int CheckUninitVar::isFunctionParUsage(const Token *vartok, bool pointer, Alloc return -1; } +int CheckUninitVar::isFunctionParUsage(const Token *vartok, bool pointer, Alloc alloc, int indirect) const +{ + return CheckUninitVar::isFunctionParUsage(vartok, mSettings->library, pointer, alloc, indirect); +} + bool CheckUninitVar::isMemberVariableAssignment(const Token *tok, const std::string &membervar) const { if (Token::Match(tok, "%name% . %name%") && tok->strAt(2) == membervar) { diff --git a/lib/checkuninitvar.h b/lib/checkuninitvar.h index bdc684631..d466f9415 100644 --- a/lib/checkuninitvar.h +++ b/lib/checkuninitvar.h @@ -84,7 +84,9 @@ public: bool checkLoopBody(const Token *tok, const Variable& var, const Alloc alloc, const std::string &membervar, const bool suppressErrors); const Token* checkLoopBodyRecursive(const Token *start, const Variable& var, const Alloc alloc, const std::string &membervar, bool &bailout) const; void checkRhs(const Token *tok, const Variable &var, Alloc alloc, nonneg int number_of_if, const std::string &membervar); + static const Token *isVariableUsage(bool cpp, const Token *vartok, const Library &library, bool pointer, Alloc alloc, int indirect = 0); const Token *isVariableUsage(const Token *vartok, bool pointer, Alloc alloc, int indirect = 0) const; + static int isFunctionParUsage(const Token *vartok, const Library &library, bool pointer, Alloc alloc, int indirect = 0); int isFunctionParUsage(const Token *vartok, bool pointer, Alloc alloc, int indirect = 0) const; bool isMemberVariableAssignment(const Token *tok, const std::string &membervar) const; bool isMemberVariableUsage(const Token *tok, bool isPointer, Alloc alloc, const std::string &membervar) const; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index cadf8a6bb..dab8ef659 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -79,6 +79,7 @@ #include "analyzer.h" #include "astutils.h" +#include "checkuninitvar.h" #include "errorlogger.h" #include "errortypes.h" #include "forwardanalyzer.h" @@ -4255,6 +4256,12 @@ static void valueFlowAfterAssign(TokenList *tokenlist, SymbolDatabase* symboldat values.remove_if([&](const ValueFlow::Value& value) { return value.valueType == ValueFlow::Value::ValueType::CONTAINER_SIZE; }); + // If assignment copy by value, remove Uninit values.. + if ((tok->astOperand1()->valueType() && tok->astOperand1()->valueType()->pointer == 0) || + (tok->astOperand1()->variable() && tok->astOperand1()->variable()->isReference() && tok->astOperand1()->variable()->nameToken() == tok->astOperand1())) + values.remove_if([&](const ValueFlow::Value& value) { + return value.isUninitValue(); + }); if (values.empty()) continue; const bool init = vars.size() == 1 && vars.front()->nameToken() == tok->astOperand1(); @@ -5777,6 +5784,11 @@ static void valueFlowSubFunction(TokenList* tokenlist, SymbolDatabase* symboldat }); // Don't forward container sizes for now since programmemory can't evaluate conditions argvalues.remove_if(std::mem_fn(&ValueFlow::Value::isContainerSizeValue)); + // Remove uninit values if argument is passed by value + if (argtok->variable() && !argtok->variable()->isPointer() && argvalues.size() == 1 && argvalues.front().isUninitValue()) { + if (CheckUninitVar::isVariableUsage(tokenlist->isCPP(), argtok, settings->library, false, CheckUninitVar::Alloc::NO_ALLOC, 0)) + continue; + } if (argvalues.empty()) continue; diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index 58e8d0459..137dd5f4e 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -4844,6 +4844,24 @@ private: " return f.i;\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + // #10327 - avoid extra warnings for uninitialized variable + valueFlowUninit("void dowork( int me ) {\n" + " if ( me == 0 ) {}\n" // <- not uninitialized + "}\n" + "\n" + "int main() {\n" + " int me;\n" + " dowork(me);\n" // <- me is uninitialized + "}"); + ASSERT_EQUALS("[test.cpp:7]: (error) Uninitialized variable: me\n", errout.str()); + + valueFlowUninit("int foo() {\n" + " int x;\n" + " int a = x;\n" // <- x is uninitialized + " return a;\n" // <- a has been initialized + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Uninitialized variable: x\n", errout.str()); } void uninitvar_ipa() {