From e20ddd55d66b462af1893e79267681195f4ef8fa Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sat, 30 Oct 2021 00:43:37 -0500 Subject: [PATCH] Propagate partially uninit variables in ValueFlow (#3533) --- lib/analyzer.h | 5 ++ lib/checkuninitvar.cpp | 71 +++++++++++++++++++++----- lib/checkuninitvar.h | 5 +- lib/valueflow.cpp | 72 +++++++++++++++++++++++++- lib/valueflow.h | 3 ++ test/testsuppressions.cpp | 3 ++ test/testuninitvar.cpp | 104 +++++++++++++++++++++++++++++++++++++- 7 files changed, 245 insertions(+), 18 deletions(-) diff --git a/lib/analyzer.h b/lib/analyzer.h index dd4fecdb1..353538325 100644 --- a/lib/analyzer.h +++ b/lib/analyzer.h @@ -50,6 +50,7 @@ struct Analyzer { Idempotent = (1 << 5), Incremental = (1 << 6), SymbolicMatch = (1 << 7), + Internal = (1 << 8), }; void set(unsigned int f, bool state = true) { @@ -96,6 +97,10 @@ struct Analyzer { return get(SymbolicMatch); } + bool isInternal() const { + return get(Internal); + } + bool matches() const { return get(Match); } diff --git a/lib/checkuninitvar.cpp b/lib/checkuninitvar.cpp index ebd3d3e79..d0bf9661b 100644 --- a/lib/checkuninitvar.cpp +++ b/lib/checkuninitvar.cpp @@ -1484,6 +1484,37 @@ void CheckUninitVar::uninitvarError(const Token *tok, const std::string &varname reportError(errorPath, Severity::error, "uninitvar", "$symbol:" + varname + "\nUninitialized variable: $symbol", CWE_USE_OF_UNINITIALIZED_VARIABLE, Certainty::normal); } +void CheckUninitVar::uninitvarError(const Token* tok, const ValueFlow::Value& v) +{ + const Token* ltok = tok; + if (tok && Token::simpleMatch(tok->astParent(), ".") && astIsRHS(tok)) + ltok = tok->astParent(); + const std::string& varname = ltok ? ltok->expressionString() : "x"; + ErrorPath errorPath = v.errorPath; + errorPath.emplace_back(tok, ""); + if (v.subexpressions.empty()) { + reportError(errorPath, + Severity::error, + "uninitvar", + "$symbol:" + varname + "\nUninitialized variable: $symbol", + CWE_USE_OF_UNINITIALIZED_VARIABLE, + Certainty::normal); + return; + } + std::string vars = v.subexpressions.size() == 1 ? "variable: " : "variables: "; + std::string prefix; + for (const std::string& var : v.subexpressions) { + vars += prefix + varname + "." + var; + prefix = ", "; + } + reportError(errorPath, + Severity::error, + "uninitvar", + "$symbol:" + varname + "\nUninitialized " + vars, + CWE_USE_OF_UNINITIALIZED_VARIABLE, + Certainty::normal); +} + void CheckUninitVar::uninitStructMemberError(const Token *tok, const std::string &membername) { reportError(tok, @@ -1492,20 +1523,34 @@ void CheckUninitVar::uninitStructMemberError(const Token *tok, const std::string "$symbol:" + membername + "\nUninitialized struct member: $symbol", CWE_USE_OF_UNINITIALIZED_VARIABLE, Certainty::normal); } -static bool isUsedByFunction(const Token* tok, int indirect, const Settings* settings) +enum class FunctionUsage { None, PassedByReference, Used }; + +static FunctionUsage getFunctionUsage(const Token* tok, int indirect, const Settings* settings) { const bool addressOf = tok->astParent() && tok->astParent()->isUnaryOp("&"); int argnr; const Token* ftok = getTokenArgumentFunction(tok, argnr); if (!ftok) - return false; - const bool isnullbad = settings->library.isnullargbad(ftok, argnr + 1); - if (indirect == 0 && astIsPointer(tok) && !addressOf && isnullbad) - return true; - bool hasIndirect = false; - const bool isuninitbad = settings->library.isuninitargbad(ftok, argnr + 1, indirect, &hasIndirect); - return isuninitbad && (!addressOf || isnullbad); + return FunctionUsage::None; + if (ftok->function()) { + std::vector args = getArgumentVars(ftok, argnr); + for (const Variable* arg : args) { + if (!arg) + continue; + if (arg->isReference()) + return FunctionUsage::PassedByReference; + } + } else { + const bool isnullbad = settings->library.isnullargbad(ftok, argnr + 1); + if (indirect == 0 && astIsPointer(tok) && !addressOf && isnullbad) + return FunctionUsage::Used; + bool hasIndirect = false; + const bool isuninitbad = settings->library.isuninitargbad(ftok, argnr + 1, indirect, &hasIndirect); + if (isuninitbad && (!addressOf || isnullbad)) + return FunctionUsage::Used; + } + return FunctionUsage::None; } static bool isLeafDot(const Token* tok) @@ -1573,7 +1618,10 @@ void CheckUninitVar::valueFlowUninit() if (Token::Match(tok->astParent(), ". %var%") && !isleaf) continue; } - if (!isUsedByFunction(tok, v->indirect, mSettings)) { + FunctionUsage fusage = getFunctionUsage(tok, v->indirect, mSettings); + if (!v->subexpressions.empty() && fusage == FunctionUsage::PassedByReference) + continue; + if (fusage != FunctionUsage::Used) { if (!(Token::Match(tok->astParent(), ". %name% (") && uninitderef) && isVariableChanged(tok, v->indirect, mSettings, mTokenizer->isCPP())) continue; @@ -1581,10 +1629,7 @@ void CheckUninitVar::valueFlowUninit() if (isVariableChangedByFunctionCall(tok, v->indirect, mSettings, &inconclusive) || inconclusive) continue; } - const Token* ltok = tok; - if (Token::simpleMatch(tok->astParent(), ".") && astIsRHS(tok)) - ltok = tok->astParent(); - uninitvarError(ltok, ltok->expressionString(), v->errorPath); + uninitvarError(tok, *v); ids.insert(tok->exprId()); if (v->tokvalue) ids.insert(v->tokvalue->exprId()); diff --git a/lib/checkuninitvar.h b/lib/checkuninitvar.h index c97fe2562..d8ba62c64 100644 --- a/lib/checkuninitvar.h +++ b/lib/checkuninitvar.h @@ -110,6 +110,7 @@ public: /** @brief Analyse all file infos for all TU */ bool analyseWholeProgram(const CTU::FileInfo *ctu, const std::list &fileInfo, const Settings& settings, ErrorLogger &errorLogger) OVERRIDE; + void uninitvarError(const Token* tok, const ValueFlow::Value& v); 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, ErrorPath errorPath); @@ -132,10 +133,12 @@ private: void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE { CheckUninitVar c(nullptr, settings, errorLogger); + ValueFlow::Value v{}; + // error + c.uninitvarError(nullptr, v); c.uninitstringError(nullptr, "varname", true); c.uninitdataError(nullptr, "varname"); - c.uninitvarError(nullptr, "varname"); c.uninitStructMemberError(nullptr, "a.b"); } diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 485344975..aec2ea6e3 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -564,6 +564,12 @@ static void setTokenValue(Token* tok, ValueFlow::Value value, const Settings* se if (Token::Match(tok, ". %var%")) setTokenValue(tok->next(), value, settings); ValueFlow::Value pvalue = value; + if (!value.subexpressions.empty()) { + if (Token::Match(parent, ". %var%") && contains(value.subexpressions, parent->next()->str())) + pvalue.subexpressions.clear(); + } + if (!pvalue.subexpressions.empty()) + return; if (parent->isUnaryOp("&")) { pvalue.indirect++; setTokenValue(parent, pvalue, settings); @@ -1957,6 +1963,10 @@ struct ValueFlowAnalyzer : Analyzer { virtual bool match(const Token* tok) const = 0; + virtual bool internalMatch(const Token*) const { + return false; + } + virtual bool isAlias(const Token* tok, bool& inconclusive) const = 0; using ProgramState = std::unordered_map; @@ -2332,6 +2342,8 @@ struct ValueFlowAnalyzer : Analyzer { const bool inconclusiveRefs = refs.size() != 1; for (const ReferenceToken& ref:refs) { Action a = analyzeToken(ref.token, tok, d, inconclusiveRefs); + if (internalMatch(ref.token)) + a |= Action::Internal; if (a != Action::None) return a; } @@ -2428,6 +2440,11 @@ struct ValueFlowAnalyzer : Analyzer { makeConditional(); } + virtual void internalUpdate(Token*, const ValueFlow::Value&, Direction) + { + assert(false && "Internal update unimplemented."); + } + virtual void update(Token* tok, Action a, Direction d) OVERRIDE { ValueFlow::Value* value = getValue(tok); if (!value) @@ -2439,6 +2456,8 @@ struct ValueFlowAnalyzer : Analyzer { value = &localValue; isSameSymbolicValue(tok, &localValue); } + if (a.isInternal()) + internalUpdate(tok, *value, d); // Read first when moving forward if (d == Direction::Forward && a.isRead()) setTokenValue(tok, *value, getSettings()); @@ -2699,10 +2718,13 @@ struct OppositeExpressionAnalyzer : ExpressionAnalyzer { }; struct SubExpressionAnalyzer : ExpressionAnalyzer { - SubExpressionAnalyzer() : ExpressionAnalyzer() {} + using PartialReadContainer = std::vector>; + // A shared_ptr is used so partial reads can be captured even after forking + std::shared_ptr partialReads; + SubExpressionAnalyzer() : ExpressionAnalyzer(), partialReads(nullptr) {} SubExpressionAnalyzer(const Token* e, const ValueFlow::Value& val, const TokenList* t) - : ExpressionAnalyzer(e, val, t) + : ExpressionAnalyzer(e, val, t), partialReads(std::make_shared()) {} virtual bool submatch(const Token* tok, bool exact = true) const = 0; @@ -2718,6 +2740,14 @@ struct SubExpressionAnalyzer : ExpressionAnalyzer { { return tok->astOperand1() && tok->astOperand1()->exprId() == expr->exprId() && submatch(tok); } + virtual bool internalMatch(const Token* tok) const OVERRIDE + { + return tok->exprId() == expr->exprId() && !(astIsLHS(tok) && submatch(tok->astParent(), false)); + } + virtual void internalUpdate(Token* tok, const ValueFlow::Value& v, Direction) OVERRIDE + { + partialReads->push_back(std::make_pair(tok, v)); + } // No reanalysis for subexression virtual ValuePtr reanalyze(Token*, const std::string&) const OVERRIDE { @@ -6387,6 +6417,19 @@ static bool needsInitialization(const Variable* var, bool cpp) return false; } +static void addToErrorPath(ValueFlow::Value& value, const ValueFlow::Value& from) +{ + std::unordered_set locations; + if (from.condition && !value.condition) + value.condition = from.condition; + std::copy_if(from.errorPath.begin(), + from.errorPath.end(), + std::back_inserter(value.errorPath), + [&](const ErrorPathItem& e) { + return locations.insert(e.first).second; + }); +} + static void valueFlowUninit(TokenList* tokenlist, SymbolDatabase* /*symbolDatabase*/, const Settings* settings) { for (Token *tok = tokenlist->front(); tok; tok = tok->next()) { @@ -6423,6 +6466,7 @@ static void valueFlowUninit(TokenList* tokenlist, SymbolDatabase* /*symbolDataba bool partial = false; + std::map partialReads; if (const Scope* scope = var->typeScope()) { if (Token::findsimplematch(scope->bodyStart, "union", scope->bodyEnd)) continue; @@ -6435,9 +6479,32 @@ static void valueFlowUninit(TokenList* tokenlist, SymbolDatabase* /*symbolDataba } MemberExpressionAnalyzer analyzer(memVar.nameToken()->str(), vardecl, uninitValue, tokenlist); valueFlowGenericForward(vardecl->next(), vardecl->scope()->bodyEnd, analyzer, settings); + + for (auto&& p : *analyzer.partialReads) { + Token* tok2 = p.first; + const ValueFlow::Value& v = p.second; + // Try to insert into map + auto pp = partialReads.insert(std::make_pair(tok2, v)); + ValueFlow::Value& v2 = pp.first->second; + bool inserted = pp.second; + // Merge the two values if it is already in map + if (!inserted) { + if (v.valueType != v2.valueType) + continue; + addToErrorPath(v2, v); + } + v2.subexpressions.push_back(memVar.nameToken()->str()); + } } } + for (auto&& p : partialReads) { + Token* tok2 = p.first; + const ValueFlow::Value& v = p.second; + + setTokenValue(tok2, v, settings); + } + if (partial) continue; @@ -7370,6 +7437,7 @@ ValueFlow::Value::Value(const Token* c, long long val, Bound b) indirect(0), path(0), wideintvalue(0), + subexpressions(), lifetimeKind(LifetimeKind::Object), lifetimeScope(LifetimeScope::Local), valueKind(ValueKind::Possible) diff --git a/lib/valueflow.h b/lib/valueflow.h index ebc272c1d..cc226eeb8 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -98,6 +98,7 @@ namespace ValueFlow { indirect(0), path(0), wideintvalue(val), + subexpressions(), lifetimeKind(LifetimeKind::Object), lifetimeScope(LifetimeScope::Local), valueKind(ValueKind::Possible) @@ -352,6 +353,8 @@ namespace ValueFlow { /** int value before implicit truncation */ long long wideintvalue; + std::vector subexpressions; + enum class LifetimeKind { // Pointer points to a member of lifetime Object, diff --git a/test/testsuppressions.cpp b/test/testsuppressions.cpp index 3bf8f364e..c275c584a 100644 --- a/test/testsuppressions.cpp +++ b/test/testsuppressions.cpp @@ -81,6 +81,9 @@ private: Suppressions::ErrorMessage errorMessage(const std::string &errorId) const { Suppressions::ErrorMessage ret; ret.errorId = errorId; + ret.hash = 0; + ret.lineNumber = 0; + ret.certainty = Certainty::CertaintyLevel::normal; return ret; } diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index de03fe293..af5b6ecf1 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -4543,7 +4543,7 @@ private: " p = new S(io);\n" " p->Write();\n" "}"); - ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:10]: (error) Uninitialized variable: p\n", errout.str()); + ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:10]: (error) Uninitialized variable: p.rIo\n", errout.str()); // Unknown types { @@ -5229,6 +5229,15 @@ private: "}"); ASSERT_EQUALS("", errout.str()); + valueFlowUninit("struct AB { int a; int b; };\n" + "void do_something(const struct AB &ab) { a = ab.b; }\n" + "void f(void) {\n" + " struct AB ab;\n" + " ab.a = 0;\n" + " do_something(ab);\n" + "}"); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:2]: (error) Uninitialized variable: ab.b\n", errout.str()); + valueFlowUninit("struct AB { int a; int b; };\n" "void f(void) {\n" " struct AB ab;\n" @@ -5252,6 +5261,97 @@ private: "test.c"); ASSERT_EQUALS("[test.c:4]: (error) Uninitialized variable: ab.a\n", errout.str()); + valueFlowUninit("struct AB { int a; int b; };\n" + "void f(void) {\n" + " struct AB ab;\n" + " ab.a = 1;\n" + " x = ab;\n" + "}\n", + "test.c"); + ASSERT_EQUALS("[test.c:5]: (error) Uninitialized variable: ab.b\n", errout.str()); + + valueFlowUninit("struct AB { int a; int b; };\n" + "void f(void) {\n" + " struct AB ab;\n" + " ab.a = 1;\n" + " x = *(&ab);\n" + "}\n", + "test.c"); + ASSERT_EQUALS("[test.c:5] -> [test.c:5]: (error) Uninitialized variable: *(&ab).b\n", errout.str()); + + valueFlowUninit("void f(void) {\n" + " struct AB ab;\n" + " int x;\n" + " ab.a = (void*)&x;\n" + " dostuff(&ab,0);\n" + "}\n", + "test.c"); + ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("struct Element {\n" + " static void f() { }\n" + "};\n" + "void test() {\n" + " Element *element; element->f();\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str()); + + valueFlowUninit("struct Element {\n" + " static void f() { }\n" + "};\n" + "void test() {\n" + " Element *element; (*element).f();\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str()); + + valueFlowUninit("struct Element {\n" + " static int v;\n" + "};\n" + "void test() {\n" + " Element *element; element->v;\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str()); + + valueFlowUninit("struct Element {\n" + " static int v;\n" + "};\n" + "void test() {\n" + " Element *element; (*element).v;\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str()); + + valueFlowUninit("struct Element {\n" + " void f() { }\n" + "};\n" + "void test() {\n" + " Element *element; element->f();\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str()); + + valueFlowUninit("struct Element {\n" + " void f() { }\n" + "};\n" + "void test() {\n" + " Element *element; (*element).f();\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str()); + + valueFlowUninit("struct Element {\n" + " int v;\n" + "};\n" + "void test() {\n" + " Element *element; element->v;\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str()); + + valueFlowUninit("struct Element {\n" + " int v;\n" + "};\n" + "void test() {\n" + " Element *element; (*element).v;\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: element\n", errout.str()); + valueFlowUninit("struct AB { int a; int b; };\n" // pass struct member by address "void f(void) {\n" " struct AB ab;\n" @@ -5472,7 +5572,7 @@ private: " s.a = 0;\n" " return s;\n" "}\n"); - TODO_ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: s.b\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: s.b\n", errout.str()); valueFlowUninit("struct S { int a; int b; };\n" // #9810 "void f(void) {\n"