From e52017750ebf4d4a82b9672bc5cf24778d61516b Mon Sep 17 00:00:00 2001 From: Rikard Falkeborn Date: Thu, 24 Jan 2019 23:48:48 +0100 Subject: [PATCH] Use valueflow in unsigned less than zero checker The unsigned less than zero checker looked for patterns like "<= 0". Switching to use valueflow improves the checker in a few aspects. First, it removes false positives where instead of 0, the code is using 0L, 0U, etc. Instead of having to hard code the different variants of 0, valueflow handles this automatically. This fixes FPs on the form uint32_t value = 0xFUL; void f() { if (value < 0u) { value = 0u; } } where 0u was previously not recognized by the checker. This fixes #8836. Morover, it makes it possible to handle templates properly. In commit fa076598ade8a751ad85d5375bc976439e32c117, all warnings inside templates were made inconclusive, since the checker had no idea if "0" came from a template parameter or not. This makes it possible to not warn for the following case which was reported as a FP in #3233 template void foo(unsigned int x) { if (x <= n); } foo<0>(); but give a warning for the following case template void foo(unsigned int x) { if (x <= 0); } Previously, both these cases gave inconclusive warnings. Finally, it makes it possible to give warnings for the following code: void f(unsigned x) { int y = 0; if (x <= y) {} } Also, previously, the checker for unsigned variables larger than 0, the checker used the string of the astoperand. This meant that for code like the following: void f(unsigned x, unsigned y) { if (x -y >= 0) {} } cppcheck would output [unsigned-expression-positive.c] (style) Unsigned variable '-' can't be negative so it is unnecessary to test it. using expressionString() instead gives a better error message [unsigned-expression-positive.c] (style) Unsigned expression 'x-z' can't be negative so it is unnecessary to test it. --- lib/checkother.cpp | 82 +++++++++------------- lib/checkother.h | 16 ++--- test/testother.cpp | 168 ++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 188 insertions(+), 78 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 3af2200d17d..f469c13cd43 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2012,10 +2012,6 @@ void CheckOther::checkSignOfUnsignedVariable() if (!mSettings->isEnabled(Settings::STYLE)) return; - const bool inconclusive = mTokenizer->codeWithTemplates(); - if (inconclusive && !mSettings->inconclusive) - return; - const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); for (const Scope * scope : symbolDatabase->functionScopes) { @@ -2024,80 +2020,64 @@ void CheckOther::checkSignOfUnsignedVariable() if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2()) continue; - if (Token::Match(tok, "<|<= 0") && tok->next() == tok->astOperand2()) { + const ValueFlow::Value *v1 = tok->astOperand1()->getValue(0); + const ValueFlow::Value *v2 = tok->astOperand2()->getValue(0); + + if (Token::Match(tok, "<|<=") && v2 && v2->isKnown()) { const ValueType* vt = tok->astOperand1()->valueType(); if (vt && vt->pointer) - pointerLessThanZeroError(tok, inconclusive); + pointerLessThanZeroError(tok, v2); if (vt && vt->sign == ValueType::UNSIGNED) - unsignedLessThanZeroError(tok, tok->astOperand1()->expressionString(), inconclusive); - } else if (Token::Match(tok->previous(), "0 >|>=") && tok->previous() == tok->astOperand1()) { + unsignedLessThanZeroError(tok, v2, tok->astOperand1()->expressionString()); + } else if (Token::Match(tok, ">|>=") && v1 && v1->isKnown()) { const ValueType* vt = tok->astOperand2()->valueType(); if (vt && vt->pointer) - pointerLessThanZeroError(tok, inconclusive); + pointerLessThanZeroError(tok, v1); if (vt && vt->sign == ValueType::UNSIGNED) - unsignedLessThanZeroError(tok, tok->astOperand2()->expressionString(), inconclusive); - } else if (Token::simpleMatch(tok, ">= 0") && tok->next() == tok->astOperand2()) { + unsignedLessThanZeroError(tok, v1, tok->astOperand2()->expressionString()); + } else if (Token::simpleMatch(tok, ">=") && v2 && v2->isKnown()) { const ValueType* vt = tok->astOperand1()->valueType(); if (vt && vt->pointer) - pointerPositiveError(tok, inconclusive); + pointerPositiveError(tok, v2); if (vt && vt->sign == ValueType::UNSIGNED) - unsignedPositiveError(tok, tok->astOperand1()->str(), inconclusive); - } else if (Token::simpleMatch(tok->previous(), "0 <=") && tok->previous() == tok->astOperand1()) { + unsignedPositiveError(tok, v2, tok->astOperand1()->expressionString()); + } else if (Token::simpleMatch(tok, "<=") && v1 && v1->isKnown()) { const ValueType* vt = tok->astOperand2()->valueType(); if (vt && vt->pointer) - pointerPositiveError(tok, inconclusive); + pointerPositiveError(tok, v1); if (vt && vt->sign == ValueType::UNSIGNED) - unsignedPositiveError(tok, tok->astOperand2()->str(), inconclusive); + unsignedPositiveError(tok, v1, tok->astOperand2()->expressionString()); } } } } -void CheckOther::unsignedLessThanZeroError(const Token *tok, const std::string &varname, bool inconclusive) +void CheckOther::unsignedLessThanZeroError(const Token *tok, const ValueFlow::Value * v, const std::string &varname) { - if (inconclusive) { - reportError(tok, Severity::style, "unsignedLessThanZero", - "$symbol:" + varname + "\n" - "Checking if unsigned expression '$symbol' is less than zero. This might be a false warning.\n" - "Checking if unsigned expression '$symbol' is less than zero. An unsigned " - "variable will never be negative so it is either pointless or an error to check if it is. " - "It's not known if the used constant is a template parameter or not and therefore " - "this message might be a false warning.", CWE570, true); - } else { - reportError(tok, Severity::style, "unsignedLessThanZero", - "$symbol:" + varname + "\n" - "Checking if unsigned expression '$symbol' is less than zero.\n" - "The unsigned expression '$symbol' will never be negative so it " - "is either pointless or an error to check if it is.", CWE570, false); - } + reportError(getErrorPath(tok, v, "Unsigned less than zero"), Severity::style, "unsignedLessThanZero", + "$symbol:" + varname + "\n" + "Checking if unsigned expression '$symbol' is less than zero.\n" + "The unsigned expression '$symbol' will never be negative so it " + "is either pointless or an error to check if it is.", CWE570, false); } -void CheckOther::pointerLessThanZeroError(const Token *tok, bool inconclusive) +void CheckOther::pointerLessThanZeroError(const Token *tok, const ValueFlow::Value *v) { - reportError(tok, Severity::style, "pointerLessThanZero", - "A pointer can not be negative so it is either pointless or an error to check if it is.", CWE570, inconclusive); + reportError(getErrorPath(tok, v, "Pointer less than zero"), Severity::style, "pointerLessThanZero", + "A pointer can not be negative so it is either pointless or an error to check if it is.", CWE570, false); } -void CheckOther::unsignedPositiveError(const Token *tok, const std::string &varname, bool inconclusive) +void CheckOther::unsignedPositiveError(const Token *tok, const ValueFlow::Value * v, const std::string &varname) { - if (inconclusive) { - reportError(tok, Severity::style, "unsignedPositive", - "$symbol:" + varname + "\n" - "Unsigned variable '$symbol' can't be negative so it is unnecessary to test it.\n" - "The unsigned variable '$symbol' can't be negative so it is unnecessary to test it. " - "It's not known if the used constant is a " - "template parameter or not and therefore this message might be a false warning", CWE570, true); - } else { - reportError(tok, Severity::style, "unsignedPositive", - "$symbol:" + varname + "\n" - "Unsigned variable '$symbol' can't be negative so it is unnecessary to test it.", CWE570, false); - } + reportError(getErrorPath(tok, v, "Unsigned positive"), Severity::style, "unsignedPositive", + "$symbol:" + varname + "\n" + "Unsigned expression '$symbol' can't be negative so it is unnecessary to test it.", CWE570, false); } -void CheckOther::pointerPositiveError(const Token *tok, bool inconclusive) +void CheckOther::pointerPositiveError(const Token *tok, const ValueFlow::Value * v) { - reportError(tok, Severity::style, "pointerPositive", - "A pointer can not be negative so it is either pointless or an error to check if it is not.", CWE570, inconclusive); + reportError(getErrorPath(tok, v, "Pointer positive"), Severity::style, "pointerPositive", + "A pointer can not be negative so it is either pointless or an error to check if it is not.", CWE570, false); } /* check if a constructor in given class scope takes a reference */ diff --git a/lib/checkother.h b/lib/checkother.h index e76bfd5a023..4d3802cac49 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -252,10 +252,10 @@ class CPPCHECKLIB CheckOther : public Check { void duplicateExpressionTernaryError(const Token *tok, ErrorPath errors); void duplicateBreakError(const Token *tok, bool inconclusive); void unreachableCodeError(const Token* tok, bool inconclusive); - void unsignedLessThanZeroError(const Token *tok, const std::string &varname, bool inconclusive); - void pointerLessThanZeroError(const Token *tok, bool inconclusive); - void unsignedPositiveError(const Token *tok, const std::string &varname, bool inconclusive); - void pointerPositiveError(const Token *tok, bool inconclusive); + void unsignedLessThanZeroError(const Token *tok, const ValueFlow::Value *v, const std::string &varname); + void pointerLessThanZeroError(const Token *tok, const ValueFlow::Value *v); + void unsignedPositiveError(const Token *tok, const ValueFlow::Value *v, const std::string &varname); + void pointerPositiveError(const Token *tok, const ValueFlow::Value *v); void SuspiciousSemicolonError(const Token *tok); void negativeBitwiseShiftError(const Token *tok, int op); void redundantCopyError(const Token *tok, const std::string &varname); @@ -318,10 +318,10 @@ class CPPCHECKLIB CheckOther : public Check { c.duplicateExpressionTernaryError(nullptr, errorPath); c.duplicateBreakError(nullptr, false); c.unreachableCodeError(nullptr, false); - c.unsignedLessThanZeroError(nullptr, "varname", false); - c.unsignedPositiveError(nullptr, "varname", false); - c.pointerLessThanZeroError(nullptr, false); - c.pointerPositiveError(nullptr, false); + c.unsignedLessThanZeroError(nullptr, nullptr, "varname"); + c.unsignedPositiveError(nullptr, nullptr, "varname"); + c.pointerLessThanZeroError(nullptr, nullptr); + c.pointerPositiveError(nullptr, nullptr); c.SuspiciousSemicolonError(nullptr); c.incompleteArrayFillError(nullptr, "buffer", "memset", false); c.varFuncNullUBError(nullptr); diff --git a/test/testother.cpp b/test/testother.cpp index 751974c0cf3..7af93253564 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -224,7 +224,7 @@ class TestOther : public TestFixture { TEST_CASE(constArgument); } - void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) { + void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, bool verbose=false, Settings* settings = 0) { // Clear the error buffer.. errout.str(""); @@ -239,6 +239,7 @@ class TestOther : public TestFixture { settings->standards.cpp = Standards::CPPLatest; settings->inconclusive = inconclusive; settings->experimental = experimental; + settings->verbose = verbose; // Tokenize.. Tokenizer tokenizer(settings, this); @@ -256,7 +257,7 @@ class TestOther : public TestFixture { } void check(const char code[], Settings *s) { - check(code,"test.cpp",false,true,true,s); + check(code,"test.cpp",false,true,true,false,s); } void checkP(const char code[], const char *filename = "test.cpp") { @@ -305,6 +306,7 @@ class TestOther : public TestFixture { false, // experimental false, // inconclusive true, // runSimpleChecks + false, // verbose &settings); } @@ -312,7 +314,7 @@ class TestOther : public TestFixture { static Settings settings; settings.platformType = Settings::Win32A; - check(code, nullptr, false, false, true, &settings); + check(code, nullptr, false, false, true, false, &settings); } void emptyBrackets() { @@ -2512,7 +2514,7 @@ class TestOther : public TestFixture { check("void foo() {\n" " exit(0);\n" " break;\n" - "}", nullptr, false, false, false, &settings); + "}", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("[test.cpp:3]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary.\n", errout.str()); check("class NeonSession {\n" @@ -2521,16 +2523,16 @@ class TestOther : public TestFixture { "void NeonSession::exit()\n" "{\n" " SAL_INFO(\"ucb.ucp.webdav\", \"neon commands cannot be aborted\");\n" - "}", nullptr, false, false, false, &settings); + "}", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("", errout.str()); check("void NeonSession::exit()\n" "{\n" " SAL_INFO(\"ucb.ucp.webdav\", \"neon commands cannot be aborted\");\n" - "}", nullptr, false, false, false, &settings); + "}", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("", errout.str()); - check("void foo() { xResAccess->exit(); }", nullptr, false, false, false, &settings); + check("void foo() { xResAccess->exit(); }", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("", errout.str()); check("void foo(int a)\n" @@ -2773,20 +2775,20 @@ class TestOther : public TestFixture { check("void foo() {\n" " (beat < 100) ? (void)0 : exit(0);\n" " bar();\n" - "}", nullptr, false, false, false, &settings); + "}", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("", errout.str()); check("void foo() {\n" " (beat < 100) ? exit(0) : (void)0;\n" " bar();\n" - "}", nullptr, false, false, false, &settings); + "}", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("", errout.str()); // #8261 check("void foo() {\n" " (beat < 100) ? (void)0 : throw(0);\n" " bar();\n" - "}", nullptr, false, false, false, &settings); + "}", nullptr, false, false, false, false, &settings); ASSERT_EQUALS("", errout.str()); } @@ -3696,6 +3698,7 @@ class TestOther : public TestFixture { false, // experimental false, // inconclusive false, // runSimpleChecks + false, // verbose nullptr // settings ); ASSERT_EQUALS("", errout.str()); @@ -3885,7 +3888,7 @@ class TestOther : public TestFixture { check("void foo() {\n" " if ((mystrcmp(a, b) == 0) || (mystrcmp(a, b) == 0)) {}\n" - "}", "test.cpp", false, false, true, &settings); + "}", "test.cpp", false, false, true, false, &settings); ASSERT_EQUALS("[test.cpp:2]: (style) Same expression on both sides of '||'.\n", errout.str()); check("void GetValue() { return rand(); }\n" @@ -4782,18 +4785,31 @@ class TestOther : public TestFixture { " for(unsigned char i = 10; i >= 0; i--)" " printf(\"%u\", i);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'i' can't be negative so it is unnecessary to test it.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'i' can't be negative so it is unnecessary to test it.\n", errout.str()); check( "void foo(bool b) {\n" " for(unsigned int i = 10; b || i >= 0; i--)" " printf(\"%u\", i);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'i' can't be negative so it is unnecessary to test it.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'i' can't be negative so it is unnecessary to test it.\n", errout.str()); + + { + const char code[] = + "bool foo(unsigned int x) {\n" + " if (x < 0)" + " return true;\n" + " return false;\n" + "}"; + check(code, nullptr, false, false, true, false); + ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + check(code, nullptr, false, false, true, true); + ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + } check( "bool foo(unsigned int x) {\n" - " if (x < 0)" + " if (x < 0u)" " return true;\n" " return false;\n" "}"); @@ -4807,6 +4823,30 @@ class TestOther : public TestFixture { "}"); ASSERT_EQUALS("", errout.str()); + { + const char code[] = + "bool foo(unsigned x) {\n" + " int y = 0;\n" + " if (x < y)" + " return true;\n" + " return false;\n" + "}"; + check(code, nullptr, false, false, true, false); + ASSERT_EQUALS("[test.cpp:3]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + check(code, nullptr, false, false, true, true); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + } + check( + "bool foo(unsigned x) {\n" + " int y = 0;\n" + " if (b)\n" + " y = 1;\n" + " if (x < y)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check( "bool foo(unsigned int x) {\n" " if (0 > x)" @@ -4815,6 +4855,14 @@ class TestOther : public TestFixture { "}"); ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + check( + "bool foo(unsigned int x) {\n" + " if (0UL > x)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + check( "bool foo(int x) {\n" " if (0 > x)" @@ -4829,7 +4877,23 @@ class TestOther : public TestFixture { " return true;\n" " return false;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + + check( + "bool foo(unsigned int x, unsigned y) {\n" + " if (x - y >= 0)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x-y' can't be negative so it is unnecessary to test it.\n", errout.str()); + + check( + "bool foo(unsigned int x) {\n" + " if (x >= 0ull)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); check( "bool foo(int x) {\n" @@ -4839,6 +4903,29 @@ class TestOther : public TestFixture { "}"); ASSERT_EQUALS("", errout.str()); + check( + "bool foo(unsigned int x) {\n" + " if (0 <= x)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + + check( + "bool foo(unsigned int x) {\n" + " if (0ll <= x)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + + check( + "bool foo(int x) {\n" + " if (0 <= x)" + " return true;\n" + " return false;\n" + "}"); + ASSERT_EQUALS("", errout.str()); check( "bool foo(unsigned int x, bool y) {\n" @@ -4878,7 +4965,7 @@ class TestOther : public TestFixture { " return true;\n" " return false;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); check( "bool foo(int x, bool y) {\n" @@ -4927,7 +5014,7 @@ class TestOther : public TestFixture { " return true;\n" " return false;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); check( "bool foo(int x, bool y) {\n" @@ -4976,7 +5063,7 @@ class TestOther : public TestFixture { " return true;\n" " return false;\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned variable 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Unsigned expression 'x' can't be negative so it is unnecessary to test it.\n", errout.str()); check( "bool foo(int x, bool y) {\n" @@ -4996,8 +5083,25 @@ class TestOther : public TestFixture { check(code, nullptr, false, false); ASSERT_EQUALS("", errout.str()); check(code, nullptr, false, true); - ASSERT_EQUALS("[test.cpp:2]: (style, inconclusive) Checking if unsigned expression 'x' is less than zero. This might be a false warning.\n", errout.str()); + ASSERT_EQUALS("", errout.str()); } + + check( + "template void foo(unsigned int x) {\n" + "if (x <= 0);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (style) Checking if unsigned expression 'x' is less than zero.\n", errout.str()); + + // #8836 + check( + "uint32_t value = 0xFUL;\n" + "void f() {\n" + " if (value < 0u)\n" + " {\n" + " value = 0u;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Checking if unsigned expression 'value' is less than zero.\n", errout.str()); } void checkSignOfPointer() { @@ -5008,6 +5112,18 @@ class TestOther : public TestFixture { "}"); ASSERT_EQUALS("[test.cpp:2]: (style) A pointer can not be negative so it is either pointless or an error to check if it is not.\n", errout.str()); + { + const char code[] = + "bool foo(int* x) {\n" + " int y = 0;\n" + " if (x >= y)" + " bar();\n" + "}"; + check(code, nullptr, false, false, true, false); + ASSERT_EQUALS("[test.cpp:3]: (style) A pointer can not be negative so it is either pointless or an error to check if it is not.\n", errout.str()); + check(code, nullptr, false, false, true, true); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) A pointer can not be negative so it is either pointless or an error to check if it is not.\n", errout.str()); + } check( "bool foo(int* x) {\n" " if (*x >= 0)" @@ -5022,6 +5138,20 @@ class TestOther : public TestFixture { "}"); ASSERT_EQUALS("[test.cpp:2]: (style) A pointer can not be negative so it is either pointless or an error to check if it is.\n", errout.str()); + { + const char code[] = + "bool foo(int* x) {\n" + " unsigned y = 0u;\n" + " if (x < y)" + " bar();\n" + "}"; + + check(code, nullptr, false, false, true, false); + ASSERT_EQUALS("[test.cpp:3]: (style) A pointer can not be negative so it is either pointless or an error to check if it is.\n", errout.str()); + check(code, nullptr, false, false, true, true); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) A pointer can not be negative so it is either pointless or an error to check if it is.\n", errout.str()); + } + check( "bool foo(int* x) {\n" " if (*x < 0)"