From e6632d93e3853d2fb2b84c090e51e190a272cce8 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Wed, 13 Sep 2023 16:27:08 -0500 Subject: [PATCH] Fix 11983: False positive: uninitialized variable (#5443) --- Makefile | 4 +- lib/astutils.cpp | 56 ++++++++++- lib/astutils.h | 8 ++ lib/cppcheck.vcxproj | 1 + lib/findtoken.h | 221 +++++++++++++++++++++++++++++++++++++++++ lib/lib.pri | 1 + lib/programmemory.cpp | 10 +- lib/valueflow.cpp | 94 +----------------- test/testuninitvar.cpp | 21 ++++ tools/dmake.cpp | 1 + 10 files changed, 318 insertions(+), 99 deletions(-) create mode 100644 lib/findtoken.h diff --git a/Makefile b/Makefile index 4828a2f97..edff57e1f 100644 --- a/Makefile +++ b/Makefile @@ -456,7 +456,7 @@ validateRules: ###### Build -$(libcppdir)/valueflow.o: lib/valueflow.cpp lib/analyzer.h lib/astutils.h lib/calculate.h lib/check.h lib/checkuninitvar.h lib/color.h lib/config.h lib/ctu.h lib/errorlogger.h lib/errortypes.h lib/forwardanalyzer.h lib/importproject.h lib/infer.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/programmemory.h lib/reverseanalyzer.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/timer.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/valueptr.h lib/vfvalue.h +$(libcppdir)/valueflow.o: lib/valueflow.cpp lib/analyzer.h lib/astutils.h lib/calculate.h lib/check.h lib/checkuninitvar.h lib/color.h lib/config.h lib/ctu.h lib/errorlogger.h lib/errortypes.h lib/findtoken.h lib/forwardanalyzer.h lib/importproject.h lib/infer.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/programmemory.h lib/reverseanalyzer.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/timer.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/valueptr.h lib/vfvalue.h $(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/valueflow.cpp $(libcppdir)/tokenize.o: lib/tokenize.cpp externals/simplecpp/simplecpp.h lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/preprocessor.h lib/settings.h lib/sourcelocation.h lib/standards.h lib/summaries.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/timer.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vfvalue.h @@ -468,7 +468,7 @@ $(libcppdir)/symboldatabase.o: lib/symboldatabase.cpp lib/astutils.h lib/color.h $(libcppdir)/analyzerinfo.o: lib/analyzerinfo.cpp externals/tinyxml2/tinyxml2.h lib/analyzerinfo.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/path.h lib/platform.h lib/utils.h $(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/analyzerinfo.cpp -$(libcppdir)/astutils.o: lib/astutils.cpp lib/astutils.h lib/check.h lib/checkclass.h lib/config.h lib/errortypes.h lib/importproject.h lib/infer.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/valueptr.h lib/vfvalue.h +$(libcppdir)/astutils.o: lib/astutils.cpp lib/astutils.h lib/check.h lib/checkclass.h lib/config.h lib/errortypes.h lib/findtoken.h lib/importproject.h lib/infer.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/valueptr.h lib/vfvalue.h $(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/astutils.cpp $(libcppdir)/check.o: lib/check.cpp lib/check.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/settings.h lib/standards.h lib/suppressions.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 847e41416..f8f662687 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -22,6 +22,7 @@ #include "config.h" #include "errortypes.h" +#include "findtoken.h" #include "infer.h" #include "library.h" #include "mathlib.h" @@ -2867,7 +2868,14 @@ bool isThisChanged(const Token* start, const Token* end, int indirect, const Set return false; } -bool isExpressionChanged(const Token* expr, const Token* start, const Token* end, const Settings* settings, bool cpp, int depth) +template +bool isExpressionChangedImpl(const Token* expr, + const Token* start, + const Token* end, + const Settings* settings, + bool cpp, + int depth, + Find find) { if (depth < 0) return true; @@ -2888,7 +2896,7 @@ bool isExpressionChanged(const Token* expr, const Token* start, const Token* end } if (tok->exprId() > 0) { - for (const Token* tok2 = start; tok2 != end; tok2 = tok2->next()) { + const Token* result = find(start, end, [&](const Token* tok2) { int indirect = 0; if (const ValueType* vt = tok->valueType()) { indirect = vt->pointer; @@ -2898,13 +2906,55 @@ bool isExpressionChanged(const Token* expr, const Token* start, const Token* end for (int i = 0; i <= indirect; ++i) if (isExpressionChangedAt(tok, tok2, i, global, settings, cpp, depth)) return true; - } + return false; + }); + if (result) + return true; } return false; }); return result; } +struct ExpressionChangedSimpleFind { + template + const Token* operator()(const Token* start, const Token* end, F f) const + { + return findToken(start, end, f); + } +}; + +struct ExpressionChangedSkipDeadCode { + const Library* library; + const std::function(const Token* tok)>* evaluate; + ExpressionChangedSkipDeadCode(const Library* library, + const std::function(const Token* tok)>& evaluate) + : library(library), evaluate(&evaluate) + {} + template + const Token* operator()(const Token* start, const Token* end, F f) const + { + return findTokenSkipDeadCode(library, start, end, f, *evaluate); + } +}; + +bool isExpressionChanged(const Token* expr, const Token* start, const Token* end, const Settings* settings, bool cpp, int depth) +{ + return isExpressionChangedImpl(expr, start, end, settings, cpp, depth, ExpressionChangedSimpleFind{}); +} + +bool isExpressionChangedSkipDeadCode(const Token* expr, + const Token* start, + const Token* end, + const Settings* settings, + bool cpp, + const std::function(const Token* tok)>& evaluate, + int depth) +{ + return isExpressionChangedImpl( + expr, start, end, settings, cpp, depth, ExpressionChangedSkipDeadCode{&settings->library, evaluate}); +} + const Token* getArgumentStart(const Token* ftok) { const Token* tok = ftok; diff --git a/lib/astutils.h b/lib/astutils.h index 95a8896a6..fb621ea81 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -345,6 +345,14 @@ CPPCHECKLIB bool isExpressionChanged(const Token* expr, bool cpp, int depth = 20); +bool isExpressionChangedSkipDeadCode(const Token* expr, + const Token* start, + const Token* end, + const Settings* settings, + bool cpp, + const std::function(const Token* tok)>& evaluate, + int depth = 20); + bool isExpressionChangedAt(const Token* expr, const Token* tok, int indirect, diff --git a/lib/cppcheck.vcxproj b/lib/cppcheck.vcxproj index 6743a9f70..04db6e081 100644 --- a/lib/cppcheck.vcxproj +++ b/lib/cppcheck.vcxproj @@ -128,6 +128,7 @@ + diff --git a/lib/findtoken.h b/lib/findtoken.h new file mode 100644 index 000000000..1210bea67 --- /dev/null +++ b/lib/findtoken.h @@ -0,0 +1,221 @@ +/* + * Cppcheck - A tool for static C/C++ code analysis + * Copyright (C) 2007-2023 Cppcheck team. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +//--------------------------------------------------------------------------- +#ifndef findtokenH +#define findtokenH +//--------------------------------------------------------------------------- + +#include +#include +#include +#include +#include + +#include "config.h" +#include "errortypes.h" +#include "library.h" +#include "smallvector.h" +#include "symboldatabase.h" +#include "token.h" + +inline std::vector evaluateKnownValues(const Token* tok) +{ + if (!tok->hasKnownIntValue()) + return {}; + return {tok->getKnownIntValue()}; +} + +template )> +void findTokensImpl(T* start, const Token* end, const Predicate& pred, Found found) +{ + for (T* tok = start; precedes(tok, end); tok = tok->next()) { + if (pred(tok)) { + if (found(tok)) + break; + } + } +} + +template )> +std::vector findTokens(T* start, const Token* end, const Predicate& pred) +{ + std::vector result; + findTokensImpl(start, end, pred, [&](T* tok) { + result.push_back(tok); + return false; + }); + return result; +} + +template )> +T* findToken(T* start, const Token* end, const Predicate& pred) +{ + T* result = nullptr; + findTokensImpl(start, end, pred, [&](T* tok) { + result = tok; + return true; + }); + return result; +} + +template )> +bool findTokensSkipDeadCodeImpl(const Library* library, + T* start, + const Token* end, + const Predicate& pred, + Found found, + const Evaluate& evaluate) +{ + for (T* tok = start; precedes(tok, end); tok = tok->next()) { + if (pred(tok)) { + if (found(tok)) + return true; + } + if (Token::Match(tok, "if|for|while (") && Token::simpleMatch(tok->next()->link(), ") {")) { + const Token* condTok = getCondTok(tok); + if (!condTok) + continue; + auto result = evaluate(condTok); + if (result.empty()) + continue; + if (findTokensSkipDeadCodeImpl(library, tok->next(), tok->linkAt(1), pred, found, evaluate)) + return true; + T* thenStart = tok->linkAt(1)->next(); + T* elseStart = nullptr; + if (Token::simpleMatch(thenStart->link(), "} else {")) + elseStart = thenStart->link()->tokAt(2); + + int r = result.front(); + if (r == 0) { + if (elseStart) { + if (findTokensSkipDeadCodeImpl(library, elseStart, elseStart->link(), pred, found, evaluate)) + return true; + if (isReturnScope(elseStart->link(), library)) + return true; + tok = elseStart->link(); + } else { + tok = thenStart->link(); + } + } else { + if (findTokensSkipDeadCodeImpl(library, thenStart, thenStart->link(), pred, found, evaluate)) + return true; + if (isReturnScope(thenStart->link(), library)) + return true; + tok = thenStart->link(); + } + } else if (Token::Match(tok->astParent(), "&&|?|%oror%") && astIsLHS(tok)) { + auto result = evaluate(tok); + if (result.empty()) + continue; + const bool cond = result.front() != 0; + T* next = nullptr; + if ((cond && Token::simpleMatch(tok->astParent(), "||")) || + (!cond && Token::simpleMatch(tok->astParent(), "&&"))) { + next = nextAfterAstRightmostLeaf(tok->astParent()); + } else if (Token::simpleMatch(tok->astParent(), "?")) { + T* colon = tok->astParent()->astOperand2(); + if (!cond) { + next = colon; + } else { + if (findTokensSkipDeadCodeImpl(library, tok->astParent()->next(), colon, pred, found, evaluate)) + return true; + next = nextAfterAstRightmostLeaf(colon); + } + } + if (next) + tok = next; + } else if (Token::simpleMatch(tok, "} else {")) { + const Token* condTok = getCondTokFromEnd(tok); + if (!condTok) + continue; + auto result = evaluate(condTok); + if (result.empty()) + continue; + if (isReturnScope(tok->link(), library)) + return true; + int r = result.front(); + if (r != 0) { + tok = tok->linkAt(2); + } + } else if (Token::simpleMatch(tok, "[") && Token::Match(tok->link(), "] (|{")) { + T* afterCapture = tok->link()->next(); + if (Token::simpleMatch(afterCapture, "(") && afterCapture->link()) + tok = afterCapture->link()->next(); + else + tok = afterCapture; + } + } + return false; +} + +template )> +std::vector findTokensSkipDeadCode(const Library* library, + T* start, + const Token* end, + const Predicate& pred, + const Evaluate& evaluate) +{ + std::vector result; + findTokensSkipDeadCodeImpl( + library, + start, + end, + pred, + [&](T* tok) { + result.push_back(tok); + return false; + }, + evaluate); + return result; +} + +template )> +std::vector findTokensSkipDeadCode(const Library* library, T* start, const Token* end, const Predicate& pred) +{ + return findTokensSkipDeadCode(library, start, end, pred, &evaluateKnownValues); +} + +template )> +T* findTokenSkipDeadCode(const Library* library, T* start, const Token* end, const Predicate& pred, const Evaluate& evaluate) +{ + T* result = nullptr; + findTokensSkipDeadCodeImpl( + library, + start, + end, + pred, + [&](T* tok) { + result = tok; + return true; + }, + evaluate); + return result; +} + +template )> +T* findTokenSkipDeadCode(const Library* library, T* start, const Token* end, const Predicate& pred) +{ + return findTokenSkipDeadCode(library, start, end, pred, &evaluateKnownValues); +} + +#endif // findtokenH diff --git a/lib/lib.pri b/lib/lib.pri index ee8669022..d43090f52 100644 --- a/lib/lib.pri +++ b/lib/lib.pri @@ -41,6 +41,7 @@ HEADERS += $${PWD}/analyzer.h \ $${PWD}/ctu.h \ $${PWD}/errorlogger.h \ $${PWD}/errortypes.h \ + $${PWD}/findtoken.h \ $${PWD}/forwardanalyzer.h \ $${PWD}/fwdanalysis.h \ $${PWD}/importproject.h \ diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp index 05c42c461..3d80a452e 100644 --- a/lib/programmemory.cpp +++ b/lib/programmemory.cpp @@ -486,10 +486,18 @@ void ProgramMemoryState::assume(const Token* tok, bool b, bool isEmpty) void ProgramMemoryState::removeModifiedVars(const Token* tok) { + ProgramMemory pm = state; + auto eval = [&](const Token* cond) -> std::vector { + if (conditionIsTrue(cond, pm)) + return {1}; + if (conditionIsFalse(cond, pm)) + return {0}; + return {}; + }; state.erase_if([&](const ExprIdToken& e) { const Token* start = origins[e.getExpressionId()]; const Token* expr = e.tok; - if (!expr || isExpressionChanged(expr, start, tok, settings, true)) { + if (!expr || isExpressionChangedSkipDeadCode(expr, start, tok, settings, true, eval)) { origins.erase(e.getExpressionId()); return true; } diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 0f34206de..b3ec12591 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -84,6 +84,7 @@ #include "config.h" #include "errorlogger.h" #include "errortypes.h" +#include "findtoken.h" #include "forwardanalyzer.h" #include "infer.h" #include "library.h" @@ -7802,99 +7803,6 @@ static void addToErrorPath(ValueFlow::Value& value, const ValueFlow::Value& from }); } -template -bool findTokenSkipDeadCodeImpl(const Library* library, Token* start, const Token* end, Predicate pred, Found found) -{ - for (Token* tok = start; precedes(tok, end); tok = tok->next()) { - if (pred(tok)) { - if (found(tok)) - return true; - } - if (Token::simpleMatch(tok, "if (")) { - const Token* condTok = tok->next()->astOperand2(); - if (!condTok) - continue; - if (!condTok->hasKnownIntValue()) - continue; - if (!Token::simpleMatch(tok->linkAt(1), ") {")) - continue; - if (findTokenSkipDeadCodeImpl(library, tok->next(), tok->linkAt(1), pred, found)) - return true; - Token* thenStart = tok->linkAt(1)->next(); - Token* elseStart = nullptr; - if (Token::simpleMatch(thenStart->link(), "} else {")) - elseStart = thenStart->link()->tokAt(2); - - int r = condTok->values().front().intvalue; - if (r == 0) { - if (elseStart) { - if (findTokenSkipDeadCodeImpl(library, elseStart, elseStart->link(), pred, found)) - return true; - if (isReturnScope(elseStart->link(), library)) - return true; - tok = elseStart->link(); - } else { - tok = thenStart->link(); - } - } else { - if (findTokenSkipDeadCodeImpl(library, thenStart, thenStart->link(), pred, found)) - return true; - if (isReturnScope(thenStart->link(), library)) - return true; - tok = thenStart->link(); - } - } else if (Token::Match(tok->astParent(), "&&|?|%oror%") && astIsLHS(tok) && tok->hasKnownIntValue()) { - const bool cond = tok->values().front().intvalue != 0; - Token* next = nullptr; - if ((cond && Token::simpleMatch(tok->astParent(), "||")) || - (!cond && Token::simpleMatch(tok->astParent(), "&&"))) { - next = nextAfterAstRightmostLeaf(tok->astParent()); - } else if (Token::simpleMatch(tok->astParent(), "?")) { - Token* colon = tok->astParent()->astOperand2(); - if (!cond) { - next = colon; - } else { - if (findTokenSkipDeadCodeImpl(library, tok->astParent()->next(), colon, pred, found)) - return true; - next = nextAfterAstRightmostLeaf(colon); - } - } - if (next) - tok = next; - } else if (Token::simpleMatch(tok, "} else {")) { - const Token* condTok = getCondTokFromEnd(tok); - if (!condTok) - continue; - if (!condTok->hasKnownIntValue()) - continue; - if (isReturnScope(tok->link(), library)) - return true; - int r = condTok->values().front().intvalue; - if (r != 0) { - tok = tok->linkAt(1); - } - } else if (Token::simpleMatch(tok, "[") && Token::Match(tok->link(), "] (|{")) { - Token* afterCapture = tok->link()->next(); - if (Token::simpleMatch(afterCapture, "(") && afterCapture->link()) - tok = afterCapture->link()->next(); - else - tok = afterCapture; - } - } - return false; -} - -template -std::vector findTokensSkipDeadCode(const Library* library, Token* start, const Token* end, Predicate pred) -{ - std::vector result; - findTokenSkipDeadCodeImpl(library, start, end, pred, [&](Token* tok) { - result.push_back(tok); - return false; - }); - return result; -} - static std::vector findAllUsages(const Variable* var, Token* start, // cppcheck-suppress constParameterPointer // FP const Library* library) diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index c0c49b1d4..a62de94e0 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -3682,6 +3682,27 @@ private: " ++c;\n" "}", "test.c"); ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("int do_something();\n" + "int set_st(int *x);\n" + "int bar();\n" + "void foo() {\n" + " int x, y;\n" + " int status = 1;\n" + " if (bar() == 1) {\n" + " status = 0;\n" + " }\n" + " if (status == 1) {\n" + " status = set_st(&x);\n" + " }\n" + " for (int i = 0; status == 1 && i < x; i++) {\n" + " if (do_something() == 0) {\n" + " status = 0;\n" + " }\n" + " }\n" + " if(status == 1 && x > 0){}\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); } void valueFlowUninit_uninitvar2() diff --git a/tools/dmake.cpp b/tools/dmake.cpp index 11d9d035b..0072f211f 100644 --- a/tools/dmake.cpp +++ b/tools/dmake.cpp @@ -326,6 +326,7 @@ int main(int argc, char **argv) libfiles_h.emplace_back("analyzer.h"); libfiles_h.emplace_back("calculate.h"); libfiles_h.emplace_back("config.h"); + libfiles_h.emplace_back("findtoken.h"); libfiles_h.emplace_back("json.h"); libfiles_h.emplace_back("precompiled.h"); libfiles_h.emplace_back("smallvector.h");