From 115f17cfe6f62d7669fe40dcb5364f3356dac0de Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Tue, 4 Apr 2023 14:55:09 -0500 Subject: [PATCH] ValueFlow: Improve the starting point for uninitialized variables to find more uninitialized usages after many conditionals (#4930) --- lib/valueflow.cpp | 52 ++++++++++++++++++++++++++++++++++++++++-- test/testuninitvar.cpp | 12 +++++----- test/testvalueflow.cpp | 23 +++++++++++++++---- 3 files changed, 75 insertions(+), 12 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 7560e2b4f..791c8b9bc 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -7692,6 +7692,52 @@ static void addToErrorPath(ValueFlow::Value& value, const ValueFlow::Value& from }); } +static std::vector findAllUsages(const Variable* var, Token* start) +{ + std::vector result; + const Scope* scope = var->scope(); + if (!scope) + return result; + Token* tok2 = Token::findmatch(start, "%varid%", scope->bodyEnd, var->declarationId()); + while (tok2) { + result.push_back(tok2); + tok2 = Token::findmatch(tok2->next(), "%varid%", scope->bodyEnd, var->declarationId()); + } + return result; +} + +static Token* findStartToken(const Variable* var, Token* start) +{ + std::vector uses = findAllUsages(var, start); + if (uses.empty()) + return start; + Token* first = uses.front(); + if (Token::findmatch(start, "goto|asm|setjmp|longjmp", first)) + return start; + const Scope* scope = first->scope(); + // If there is only one usage or the first usage is in the same scope + if (uses.size() == 1 || scope == var->scope()) + return first->previous(); + // If all uses are in the same scope + if (std::all_of(uses.begin() + 1, uses.end(), [&](const Token* tok) { + return tok->scope() == scope; + })) + return first->previous(); + // Compute the outer scope + while (scope && scope->nestedIn != var->scope()) + scope = scope->nestedIn; + if (!scope) + return start; + Token* tok = const_cast(scope->bodyStart); + if (!tok) + return start; + if (Token::simpleMatch(tok->tokAt(-2), "} else {")) + tok = tok->linkAt(-2); + if (Token::simpleMatch(tok->previous(), ") {")) + return tok->linkAt(-1)->previous(); + return tok; +} + static void valueFlowUninit(TokenList* tokenlist, SymbolDatabase* /*symbolDatabase*/, const Settings* settings) { for (Token *tok = tokenlist->front(); tok; tok = tok->next()) { @@ -7718,6 +7764,8 @@ static void valueFlowUninit(TokenList* tokenlist, SymbolDatabase* /*symbolDataba bool partial = false; + Token* start = findStartToken(var, tok->next()); + std::map partialReads; if (const Scope* scope = var->typeScope()) { if (Token::findsimplematch(scope->bodyStart, "union", scope->bodyEnd)) @@ -7733,7 +7781,7 @@ static void valueFlowUninit(TokenList* tokenlist, SymbolDatabase* /*symbolDataba continue; } MemberExpressionAnalyzer analyzer(memVar.nameToken()->str(), tok, uninitValue, tokenlist, settings); - valueFlowGenericForward(tok->next(), tok->scope()->bodyEnd, analyzer, *settings); + valueFlowGenericForward(start, tok->scope()->bodyEnd, analyzer, *settings); for (auto&& p : *analyzer.partialReads) { Token* tok2 = p.first; @@ -7763,7 +7811,7 @@ static void valueFlowUninit(TokenList* tokenlist, SymbolDatabase* /*symbolDataba if (partial) continue; - valueFlowForward(tok->next(), tok->scope()->bodyEnd, var->nameToken(), uninitValue, tokenlist, settings); + valueFlowForward(start, tok->scope()->bodyEnd, var->nameToken(), uninitValue, tokenlist, settings); } } diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index fbc2128af..67ffcda08 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -3499,7 +3499,7 @@ private: " }\n" "}", "test.cpp"); - ASSERT_EQUALS("", errout.str()); + TODO_ASSERT_EQUALS("", "[test.cpp:6]: (error) Uninitialized variable: i\n", errout.str()); valueFlowUninit("void f() {\n" " int i, y;\n" @@ -3510,7 +3510,7 @@ private: " }\n" "}", "test.cpp"); - ASSERT_EQUALS("", errout.str()); + TODO_ASSERT_EQUALS("", "[test.cpp:6]: (error) Uninitialized variable: i\n", errout.str()); valueFlowUninit("void f() {\n" " int i, y;\n" @@ -3838,7 +3838,7 @@ private: " if (y == 1) { return; }\n" " return x;\n" "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (error) Uninitialized variable: x\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Uninitialized variable: x\n", errout.str()); valueFlowUninit("int f(int x) {\n" " int ret;\n" @@ -3871,7 +3871,7 @@ private: " if (foo) break;\n" " return x;\n" "}"); - ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5]: (error) Uninitialized variable: x\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: x\n", errout.str()); valueFlowUninit("int f() {\n" " int x;\n" @@ -3879,7 +3879,7 @@ private: " if (bar) break;\n" " return x;\n" "}"); - ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5]: (error) Uninitialized variable: x\n", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (error) Uninitialized variable: x\n", errout.str()); // try/catch : don't warn about exception variable valueFlowUninit("void f() {\n" @@ -6662,7 +6662,7 @@ private: " struct AB ab;\n" " while (x) { ab.a = ab.a + 1; }\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Uninitialized variable: ab.a\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Uninitialized variable: ab.a\n", errout.str()); valueFlowUninit("struct AB { int a; };\n" "void f() {\n" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index af178366f..bc03bad77 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -162,6 +162,7 @@ private: TEST_CASE(valueFlowSymbolicStrlen); TEST_CASE(valueFlowSmartPointer); TEST_CASE(valueFlowImpossibleMinMax); + TEST_CASE(valueFlowImpossibleUnknownConstant); } static bool isNotTokValue(const ValueFlow::Value &val) { @@ -5433,18 +5434,19 @@ private: " return x;\n" "}\n"; values = tokenValues(code, "x ; }", ValueFlow::Value::ValueType::UNINIT); - ASSERT_EQUALS(0, values.size()); + ASSERT_EQUALS(1, values.size()); + ASSERT_EQUALS(true, values.front().isUninitValue()); - code = "void f() {\n" + code = "void f(int x) {\n" " int i;\n" - " if (x) {\n" + " if (x > 0) {\n" " int y = -ENOMEM;\n" // assume constant ENOMEM is nonzero since it's negated " if (y != 0) return;\n" " i++;\n" " }\n" "}\n"; values = tokenValues(code, "i ++", ValueFlow::Value::ValueType::UNINIT); - ASSERT_EQUALS(0, values.size()); + TODO_ASSERT_EQUALS(0, 1, values.size()); } void valueFlowConditionExpressions() { @@ -7874,6 +7876,19 @@ private: ASSERT_EQUALS(true, testValueOfXImpossible(code, 3U, "a", -1)); ASSERT_EQUALS(true, testValueOfXImpossible(code, 3U, -1)); } + + void valueFlowImpossibleUnknownConstant() + { + const char* code; + + code = "void f(bool b) {\n" + " if (b) {\n" + " int x = -ENOMEM;\n" // assume constant ENOMEM is nonzero since it's negated + " if (x != 0) return;\n" + " }\n" + "}\n"; + ASSERT_EQUALS(true, testValueOfXImpossible(code, 4U, 0)); + } }; REGISTER_TEST(TestValueFlow)