From 8c9c46835a26282ab1b21f621a2c72984c098643 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Sat, 30 Oct 2021 15:13:58 -0500 Subject: [PATCH] Fix for 6597: false negative: uninitialized variable usage not detected (ValueFlow , multi variables) (#3535) --- Makefile | 2 +- lib/astutils.cpp | 2 + lib/programmemory.cpp | 33 ++++--- lib/valueflow.cpp | 4 + lib/valueflow.h | 6 ++ test/testuninitvar.cpp | 141 +++++++++++++++++++++++++-- tools/extracttests.py | 5 +- tools/generate_and_run_more_tests.sh | 10 +- tools/run_more_tests.sh | 50 ++++++---- 9 files changed, 204 insertions(+), 49 deletions(-) diff --git a/Makefile b/Makefile index 8d2c2dd0a..fe3199775 100644 --- a/Makefile +++ b/Makefile @@ -546,7 +546,7 @@ $(libcppdir)/platform.o: lib/platform.cpp externals/tinyxml2/tinyxml2.h lib/conf $(libcppdir)/preprocessor.o: lib/preprocessor.cpp externals/simplecpp/simplecpp.h lib/color.h lib/config.h lib/errorlogger.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/path.h lib/platform.h lib/preprocessor.h lib/settings.h lib/standards.h lib/suppressions.h lib/timer.h lib/utils.h $(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CPPFILESDIR) $(CXXFLAGS) $(UNDEF_STRICT_ANSI) -c -o $(libcppdir)/preprocessor.o $(libcppdir)/preprocessor.cpp -$(libcppdir)/programmemory.o: lib/programmemory.cpp lib/astutils.h lib/calculate.h lib/config.h lib/errortypes.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/programmemory.h lib/settings.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/timer.h lib/token.h lib/utils.h lib/valueflow.h +$(libcppdir)/programmemory.o: lib/programmemory.cpp lib/astutils.h lib/calculate.h lib/config.h lib/errortypes.h lib/importproject.h lib/infer.h lib/library.h lib/mathlib.h lib/platform.h lib/programmemory.h lib/settings.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/timer.h lib/token.h lib/utils.h lib/valueflow.h lib/valueptr.h $(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CPPFILESDIR) $(CXXFLAGS) $(UNDEF_STRICT_ANSI) -c -o $(libcppdir)/programmemory.o $(libcppdir)/programmemory.cpp $(libcppdir)/reverseanalyzer.o: lib/reverseanalyzer.cpp lib/analyzer.h lib/astutils.h lib/config.h lib/errortypes.h lib/forwardanalyzer.h lib/importproject.h lib/library.h lib/mathlib.h lib/platform.h lib/reverseanalyzer.h lib/settings.h lib/standards.h lib/suppressions.h lib/symboldatabase.h lib/templatesimplifier.h lib/timer.h lib/token.h lib/utils.h lib/valueflow.h lib/valueptr.h diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 83161c87f..5bed78446 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1080,6 +1080,8 @@ static bool isUsedAsBool_internal(const Token * const tok, bool checkingParent) } } else if (isForLoopCondition(tok)) return true; + else if (Token::simpleMatch(parent, "?") && astIsLHS(tok)) + return true; return isUsedAsBool_internal(parent, true); } diff --git a/lib/programmemory.cpp b/lib/programmemory.cpp index 333f0ab54..f52ae65a2 100644 --- a/lib/programmemory.cpp +++ b/lib/programmemory.cpp @@ -3,11 +3,13 @@ #include "astutils.h" #include "calculate.h" #include "errortypes.h" +#include "infer.h" #include "mathlib.h" #include "settings.h" #include "symboldatabase.h" #include "token.h" #include "valueflow.h" +#include "valueptr.h" #include #include #include @@ -215,11 +217,9 @@ void programMemoryParseCondition(ProgramMemory& pm, const Token* tok, const Toke programMemoryParseCondition(pm, tok->astOperand1(), endTok, settings, then); } } else if (tok->exprId() > 0) { - if (then && !astIsPointer(tok) && !astIsBool(tok)) - return; if (endTok && isExpressionChanged(tok, tok->next(), endTok, settings, true)) return; - pm.setIntValue(tok->exprId(), then); + pm.setIntValue(tok->exprId(), 0, then); const Token* containerTok = settings->library.getContainerFromYield(tok, Library::Container::Yield::EMPTY); if (containerTok) pm.setContainerSizeValue(containerTok->exprId(), 0, then); @@ -637,14 +637,18 @@ static ValueFlow::Value execute(const Token* expr, ProgramMemory& pm) if (!lhs.isUninitValue() && !rhs.isUninitValue()) return evaluate(expr->str(), lhs, rhs); if (expr->isComparisonOp()) { - if (rhs.isIntValue() && !rhs.isImpossible()) { - ValueFlow::Value v = inferCondition(expr->str(), expr->astOperand1(), rhs.intvalue); - if (v.isKnown()) - return v; - } else if (lhs.isIntValue() && !lhs.isImpossible()) { - ValueFlow::Value v = inferCondition(expr->str(), lhs.intvalue, expr->astOperand2()); - if (v.isKnown()) - return v; + if (rhs.isIntValue()) { + std::vector result = + infer(makeIntegralInferModel(), expr->str(), expr->astOperand1()->values(), {rhs}); + if (result.empty()) + return unknown; + return result.front(); + } else if (lhs.isIntValue()) { + std::vector result = + infer(makeIntegralInferModel(), expr->str(), {lhs}, expr->astOperand2()->values()); + if (result.empty()) + return unknown; + return result.front(); } } } @@ -674,7 +678,12 @@ static ValueFlow::Value execute(const Token* expr, ProgramMemory& pm) return execute(expr->astOperand1(), pm); } if (expr->exprId() > 0 && pm.hasValue(expr->exprId())) { - return pm.values.at(expr->exprId()); + ValueFlow::Value result = pm.values.at(expr->exprId()); + if (result.isImpossible() && result.isIntValue() && result.intvalue == 0 && isUsedAsBool(expr)) { + result.intvalue = !result.intvalue; + result.setKnown(); + } + return result; } return unknown; diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index aec2ea6e3..3b0964c87 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -5465,6 +5465,10 @@ struct IntegralInferModel : InferModel { } }; +ValuePtr makeIntegralInferModel() { + return IntegralInferModel{}; +} + ValueFlow::Value inferCondition(const std::string& op, const Token* varTok, MathLib::bigint val) { if (!varTok) diff --git a/lib/valueflow.h b/lib/valueflow.h index cc226eeb8..b6a1b1c11 100644 --- a/lib/valueflow.h +++ b/lib/valueflow.h @@ -35,6 +35,7 @@ #include class ErrorLogger; +struct InferModel; class Settings; class SymbolDatabase; class Token; @@ -42,6 +43,9 @@ class TokenList; class ValueType; class Variable; +template +class ValuePtr; + namespace ValueFlow { struct increment { template @@ -484,6 +488,8 @@ const Token *parseCompareInt(const Token *tok, ValueFlow::Value &true_value, Val ValueFlow::Value inferCondition(std::string op, MathLib::bigint val, const Token* varTok); ValueFlow::Value inferCondition(const std::string& op, const Token* varTok, MathLib::bigint val); +ValuePtr makeIntegralInferModel(); + std::vector getLifetimeTokens(const Token* tok, bool escape = false, ValueFlow::Value::ErrorPath errorPath = ValueFlow::Value::ErrorPath{}); diff --git a/test/testuninitvar.cpp b/test/testuninitvar.cpp index a88a51bc9..cc289800a 100644 --- a/test/testuninitvar.cpp +++ b/test/testuninitvar.cpp @@ -63,6 +63,7 @@ private: TEST_CASE(uninitvar5); // #3861 TEST_CASE(uninitvar2_func); // function calls TEST_CASE(uninitvar2_value); // value flow + TEST_CASE(valueFlowUninit2_value); TEST_CASE(uninitStructMember); // struct members TEST_CASE(uninitvar2_while); TEST_CASE(uninitvar2_4494); // #4494 @@ -3308,13 +3309,6 @@ private: "}"); ASSERT_EQUALS("", errout.str()); - checkUninitVar("void f(int x) {\n" - " int i;\n" - " if (!x) i = 0;\n" - " if (!x || i>0) {}\n" // <- error - "}"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Uninitialized variable: i\n", "", errout.str()); - checkUninitVar("void f(int x) {\n" " int i;\n" " if (x) i = 0;\n" @@ -3396,6 +3390,127 @@ private: TODO_ASSERT_EQUALS("error", "", errout.str()); } + void valueFlowUninit2_value() + { + valueFlowUninit("void f() {\n" + " int i;\n" + " if (x) {\n" + " int y = -ENOMEM;\n" // assume constant ENOMEM is nonzero since it's negated + " if (y != 0) return;\n" + " i++;\n" + " }\n" + "}", + "test.cpp"); + ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("void f() {\n" + " int i, y;\n" + " if (x) {\n" + " y = -ENOMEM;\n" // assume constant ENOMEM is nonzero since it's negated + " if (y != 0) return;\n" + " i++;\n" + " }\n" + "}", + "test.cpp"); + ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("void f() {\n" + " int i, y;\n" + " if (x) y = -ENOMEM;\n" // assume constant ENOMEM is nonzero since it's negated + " else y = get_value(i);\n" + " if (y != 0) return;\n" // <- condition is always true if i is uninitialized + " i++;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("void f(int x) {\n" + " int i;\n" + " if (!x) i = 0;\n" + " if (!x || i>0) {}\n" // <- error + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (error) Uninitialized variable: i\n", errout.str()); + + valueFlowUninit("void f(int x) {\n" + " int i;\n" + " if (x) i = 0;\n" + " if (!x || i>0) {}\n" // <- no error + "}"); + ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("void f(int x) {\n" + " int i;\n" + " if (!x) { }\n" + " else i = 0;\n" + " if (x || i>0) {}\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:5]: (error) Uninitialized variable: i\n", errout.str()); + + valueFlowUninit("void f(int x) {\n" + " int i;\n" + " if (x) { }\n" + " else i = 0;\n" + " if (x || i>0) {}\n" // <- no error + "}"); + ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("int f(int x) {\n" + " int y;\n" + " if (x) y = do_something();\n" + " if (!x) return 0;\n" + " return y;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // extracttests.start: int y; + valueFlowUninit("int f(int x) {\n" // FP with ?: + " int a;\n" + " if (x)\n" + " a = y;\n" + " return x ? 2*a : 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("int f(int x) {\n" + " int a;\n" + " if (x)\n" + " a = y;\n" + " return y ? 2*a : 3*a;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:5]: (error) Uninitialized variable: a\n", errout.str()); + + valueFlowUninit("void f() {\n" // Don't crash + " int a;\n" + " dostuff(\"ab\" cd \"ef\", x?a:z);\n" // <- No AST is created for ?: + "}"); + + // Unknown => bail out.. + valueFlowUninit("void f(int x) {\n" + " int i;\n" + " if (a(x)) i = 0;\n" + " if (b(x)) return;\n" + " i++;\n" // <- no error if b(x) is always true when a(x) is false + "}"); + ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("void f(int x) {\n" + " int i;\n" + " if (x) i = 0;\n" + " while (condition) {\n" + " if (x) i++;\n" // <- no error + " }\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + valueFlowUninit("void f(int x) {\n" + " int i;\n" + " if (x) i = 0;\n" + " while (condition) {\n" + " i++;\n" + " }\n" + "}"); + TODO_ASSERT_EQUALS("error", "", errout.str()); + } + void uninitStructMember() { // struct members checkUninitVar("struct AB { int a; int b; };\n" "void f(void) {\n" @@ -4954,6 +5069,18 @@ private: " return out;\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + // #6597 + valueFlowUninit("int f(int b) {\n" + " int a;\n" + " if (!b)\n" + " a = 1;\n" + " if (b)\n" + " return a;\n" + " else\n" + " return -1;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:6]: (error) Uninitialized variable: a\n", errout.str()); } void valueFlowUninitBreak() { // Do not show duplicate warnings about the same uninitialized value diff --git a/tools/extracttests.py b/tools/extracttests.py index 16a526ad3..f2b12ad1c 100755 --- a/tools/extracttests.py +++ b/tools/extracttests.py @@ -355,10 +355,7 @@ if filename is not None: if not os.path.exists(codedir): os.mkdir(codedir) - testfile = filename - if testfile.find('/'): - testfile = testfile[testfile.rfind('/'):] - testfile = testfile[:testfile.find('.')] + testfile = os.path.splitext(os.path.basename(filename))[0] for node in e.nodes: if onlyTP and node['expected'] == '': diff --git a/tools/generate_and_run_more_tests.sh b/tools/generate_and_run_more_tests.sh index 8955189ef..9034aa4ec 100755 --- a/tools/generate_and_run_more_tests.sh +++ b/tools/generate_and_run_more_tests.sh @@ -3,17 +3,19 @@ # cd ~/cppcheck # tools/generate_and_run_more_tests.sh +DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" + set -e echo testleakautovar -tools/run_more_tests.sh test/testleakautovar.cpp +$DIR/run_more_tests.sh $DIR/../test/testleakautovar.cpp echo testmemleak -tools/run_more_tests.sh test/testmemleak.cpp +$DIR/run_more_tests.sh $DIR/../test/testmemleak.cpp echo testnullpointer -tools/run_more_tests.sh test/testnullpointer.cpp +$DIR/run_more_tests.sh $DIR/../test/testnullpointer.cpp echo testuninitvar -tools/run_more_tests.sh test/testuninitvar.cpp +$DIR/run_more_tests.sh $DIR/../test/testuninitvar.cpp diff --git a/tools/run_more_tests.sh b/tools/run_more_tests.sh index e16f50ec2..2a5e17c42 100755 --- a/tools/run_more_tests.sh +++ b/tools/run_more_tests.sh @@ -1,96 +1,104 @@ #!/bin/bash # Script Used by generate_and_run_more_tests.sh +DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )" + set -e -python tools/extracttests.py --code=test1 $1 +CPPCHECK=$(pwd)/cppcheck + +if test -f ./bin/cppcheck; then + CPPCHECK=$(pwd)/bin/cppcheck +fi + +python $DIR/extracttests.py --code=$(pwd)/test1 $1 cd test1 -../cppcheck -q --template=cppcheck1 . 2> 1.txt +$CPPCHECK -q --template=cppcheck1 . 2> 1.txt # (!x) => (x==0) sed -ri 's/([(&][ ]*)\!([a-z]+)([ ]*[&)])/\1\2==0\3/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (x==0) => (0==x) sed -ri 's/([(&][ ]*)([a-z]+)[ ]*==[ ]*0([ ]*[&)])/\10==\2\3/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (0==x) => (!x) sed -ri 's/([(&][ ]*)0[ ]*==[ ]*([a-z]+)([ ]*[&)])/\1!\2\3/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # if (x) => (x!=0) sed -ri 's/(if[ ]*\([ ]*[a-z]+)([ ]*[&)])/\1!=0\2/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # while (x) => (x!=0) sed -ri 's/(while[ ]*\([ ]*[a-z]+)([ ]*[&)])/\1!=0\2/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (x!=0) => (0!=x) sed -ri 's/([(&][ ]*)([a-z]+)[ ]*!=[ ]*0([ ]*[&)])/\10!=\2\3/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (0!=x) => (x) sed -ri 's/([(&][ ]*)0[ ]*!=[ ]*([a-z]+[ ]*[&)])/\1\2/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (x < 0) => (0 > x) sed -ri 's/([(&][ ]*)([a-z]+)[ ]*<[ ]*(\-?[0-9]+)([ ]*[&)])/\1\3>\2\4/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (x <= 0) => (0 >= x) sed -ri 's/([(&][ ]*)([a-z]+)[ ]*<=[ ]*(\-?[0-9]+)([ ]*[&)])/\1\3>=\2\4/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (x > 0) => (0 < x) sed -ri 's/([(&][ ]*)([a-z]+)[ ]*<=[ ]*(\-?[0-9]+)([ ]*[&)])/\1\3>=\2\4/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (x >= 0) => (0 <= x) sed -ri 's/([(&][ ]*)([a-z]+)[ ]*<=[ ]*(\-?[0-9]+)([ ]*[&)])/\1\3>=\2\4/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (x == 123) => (123 == x) sed -ri 's/([(&][ ]*)([a-z]+)[ ]*==[ ]*(\-?[0-9]+)([ ]*[&)])/\1\3==\2\4/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (x != 123) => (123 != x) sed -ri 's/([(&][ ]*)([a-z]+)[ ]*\!=[ ]*(\-?[0-9]+)([ ]*[&)])/\1\3!=\2\4/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (0 < x) => (x > 0) sed -ri 's/([(&][ ]*)(\-?[0-9]+)[ ]*<[ ]*([a-z]+)([ ]*[&)])/\1\3>\2\4/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (0 <= x) => (x >= 0) sed -ri 's/([(&][ ]*)(\-?[0-9]+)[ ]*<=[ ]*([a-z]+)([ ]*[&)])/\1\3>=\2\4/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (0 > x) => (x < 0) sed -ri 's/([(&][ ]*)(\-?[0-9]+)[ ]*<=[ ]*([a-z]+)([ ]*[&)])/\1\3>=\2\4/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (0 >= x) => (x <= 0) sed -ri 's/([(&][ ]*)(\-?[0-9]+)[ ]*<=[ ]*([a-z]+)([ ]*[&)])/\1\3>=\2\4/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (123 == x) => (x == 123) sed -ri 's/([(&][ ]*)(\-?[0-9]+)[ ]*==[ ]*([a-z]+)([ ]*[&)])/\1\3==\2\4/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt # (123 != x) => (x <= 123) sed -ri 's/([(&][ ]*)(\-?[0-9]+)[ ]*\!=[ ]*([a-z]+)([ ]*[&)])/\1\3!=\2\4/' *.cpp -../cppcheck -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt +$CPPCHECK -q --template=cppcheck1 . 2> 2.txt && diff 1.txt 2.txt cd ..