diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 717f2369f..978458ba2 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1014,3 +1014,62 @@ void CheckCondition::alwaysTrueFalseError(const Token *tok, bool knownResult) "knownConditionTrueFalse", "Condition '" + expr + "' is always " + (knownResult ? "true" : "false")); } + +void CheckCondition::checkInvalidTestForOverflow() +{ + if (!_settings->isEnabled("warning")) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + const std::size_t functions = symbolDatabase->functionScopes.size(); + for (std::size_t i = 0; i < functions; ++i) { + const Scope * scope = symbolDatabase->functionScopes[i]; + + for (const Token* tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) { + if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2()) + continue; + + const Token *calcToken, *exprToken; + if (tok->str() == "<" && tok->astOperand1()->str() == "+") { + calcToken = tok->astOperand1(); + exprToken = tok->astOperand2(); + } else if (tok->str() == ">" && tok->astOperand2()->str() == "+") { + calcToken = tok->astOperand2(); + exprToken = tok->astOperand1(); + } else + continue; + + // Only warn for signed integer overflows and pointer overflows. + if (!(calcToken->valueType() && (calcToken->valueType()->pointer || calcToken->valueType()->sign == ValueType::Sign::SIGNED))) + continue; + if (!(exprToken->valueType() && (exprToken->valueType()->pointer || exprToken->valueType()->sign == ValueType::Sign::SIGNED))) + continue; + + const Token *termToken; + if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand1(), _settings->library.functionpure)) + termToken = calcToken->astOperand2(); + else if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand2(), _settings->library.functionpure)) + termToken = calcToken->astOperand1(); + else + continue; + + if (!termToken) + continue; + + // Only warn when termToken is always positive + if (termToken->valueType() && termToken->valueType()->sign == ValueType::Sign::UNSIGNED) + invalidTestForOverflow(tok); + else if (termToken->isNumber() && MathLib::isPositive(termToken->str())) + invalidTestForOverflow(tok); + } + } +} + +void CheckCondition::invalidTestForOverflow(const Token* tok) +{ + std::string errmsg; + errmsg = "Invalid test for overflow '" + + (tok ? tok->expressionString() : std::string("x + u < x")) + + "'. Condition is always false unless there is overflow, and overflow is UB."; + reportError(tok, Severity::warning, "invalidTestForOverflow", errmsg); +} diff --git a/lib/checkcondition.h b/lib/checkcondition.h index 8ed091c14..099ba6316 100644 --- a/lib/checkcondition.h +++ b/lib/checkcondition.h @@ -50,6 +50,7 @@ public: checkCondition.clarifyCondition(); // not simplified because ifAssign checkCondition.oppositeInnerCondition(); checkCondition.checkIncorrectLogicOperator(); + checkCondition.checkInvalidTestForOverflow(); } /** @brief Run checks against the simplified token list */ @@ -97,6 +98,9 @@ public: /** @brief Condition is always true/false */ void alwaysTrueFalse(); + /** @brief %Check for invalid test for overflow 'x+100 < x' */ + void checkInvalidTestForOverflow(); + private: bool isOverlappingCond(const Token * const cond1, const Token * const cond2, const std::set &constFunctions) const; @@ -122,6 +126,8 @@ private: void alwaysTrueFalseError(const Token *tok, bool knownResult); + void invalidTestForOverflow(const Token* tok); + void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckCondition c(0, settings, errorLogger); @@ -136,6 +142,7 @@ private: c.moduloAlwaysTrueFalseError(0, "1"); c.clarifyConditionError(0, true, false); c.alwaysTrueFalseError(0, true); + c.invalidTestForOverflow(0); } static std::string myName() { @@ -150,10 +157,11 @@ private: "- Detect matching 'if' and 'else if' conditions\n" "- Mismatching bitand (a &= 0xf0; a &= 1; => a = 0)\n" "- Find dead code which is inaccessible due to the counter-conditions check in nested if statements\n" - "- condition that is always true/false\n" - "- mutual exclusion over || always evaluating to true\n" + "- Condition that is always true/false\n" + "- Mutual exclusion over || always evaluating to true\n" "- Comparisons of modulo results that are always true/false.\n" - "- Known variable values => condition is always true/false\n"; + "- Known variable values => condition is always true/false\n" + "- Invalid test for overflow (for example 'ptr+u < ptr'). Condition is always false unless there is overflow, and overflow is UB.\n"; } }; /// @} diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 672d07d0d..0ceec88ab 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2455,62 +2455,3 @@ void CheckOther::unusedLabelError(const Token* tok) reportError(tok, Severity::style, "unusedLabel", "Label '" + (tok?tok->str():emptyString) + "' is not used."); } - -void CheckOther::checkInvalidTestForOverflow() -{ - if (!_settings->isEnabled("warning")) - return; - - const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); - const std::size_t functions = symbolDatabase->functionScopes.size(); - for (std::size_t i = 0; i < functions; ++i) { - const Scope * scope = symbolDatabase->functionScopes[i]; - - for (const Token* tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) { - if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2()) - continue; - - const Token *calcToken, *exprToken; - if (Token::Match(tok, "<|<=") && tok->astOperand1()->str() == "+") { - calcToken = tok->astOperand1(); - exprToken = tok->astOperand2(); - } else if (Token::Match(tok, ">|>=") && tok->astOperand2()->str() == "+") { - calcToken = tok->astOperand2(); - exprToken = tok->astOperand1(); - } else - continue; - - // Only warn for signed integer overflows and pointer overflows. - if (!(calcToken->valueType() && (calcToken->valueType()->pointer || calcToken->valueType()->sign == ValueType::Sign::SIGNED))) - continue; - if (!(exprToken->valueType() && (exprToken->valueType()->pointer || exprToken->valueType()->sign == ValueType::Sign::SIGNED))) - continue; - - const Token *termToken; - if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand1(), _settings->library.functionpure)) - termToken = calcToken->astOperand2(); - else if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand2(), _settings->library.functionpure)) - termToken = calcToken->astOperand1(); - else - continue; - - if (!termToken) - continue; - - // Only warn when termToken is always positive - if (termToken->valueType() && termToken->valueType()->sign == ValueType::Sign::UNSIGNED) - invalidTestForOverflow(tok); - else if (termToken->isNumber() && MathLib::isPositive(termToken->str())) - invalidTestForOverflow(tok); - } - } -} - -void CheckOther::invalidTestForOverflow(const Token* tok) -{ - std::string errmsg; - errmsg = "Invalid test for overflow '" + - (tok ? tok->expressionString() : std::string("x + u < x")) + - "'. Condition is always false unless there is overflow, and overflow is UB."; - reportError(tok, Severity::warning, "invalidTestForOverflow", errmsg); -} diff --git a/lib/checkother.h b/lib/checkother.h index 1ccff97ec..731147172 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -70,7 +70,6 @@ public: checkOther.checkZeroDivision(); checkOther.checkInterlockedDecrement(); checkOther.checkUnusedLabel(); - checkOther.checkInvalidTestForOverflow(); } /** @brief Run checks against the simplified token list */ @@ -204,9 +203,6 @@ public: /** @brief %Check for unused labels */ void checkUnusedLabel(); - /** @brief %Check for invalid test for overflow 'x+100 < x' */ - void checkInvalidTestForOverflow(); - private: // Error messages.. void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result); @@ -255,7 +251,6 @@ private: void redundantPointerOpError(const Token* tok, const std::string& varname, bool inconclusive); void raceAfterInterlockedDecrementError(const Token* tok); void unusedLabelError(const Token* tok); - void invalidTestForOverflow(const Token* tok); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckOther c(0, settings, errorLogger); @@ -310,7 +305,6 @@ private: c.commaSeparatedReturnError(0); c.redundantPointerOpError(0, "varname", false); c.unusedLabelError(0); - c.invalidTestForOverflow(0); } static std::string myName() { @@ -333,7 +327,6 @@ private: // warning "- either division by zero or useless condition\n" "- memset() with a value out of range as the 2nd parameter\n" - "- invalid test for overflow\n" // performance "- redundant data copying for const variable\n" diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 1f642c6a3..1df4f6493 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -79,6 +79,8 @@ private: TEST_CASE(clarifyCondition6); // #3818 TEST_CASE(alwaysTrue); + + TEST_CASE(checkInvalidTestForOverflow); } void check(const char code[], const char* filename = "test.cpp") { @@ -1604,6 +1606,23 @@ private: "}"); ASSERT_EQUALS("", errout.str()); } + + void checkInvalidTestForOverflow() { + check("void f(char *p, unsigned int x) {\n" + " assert((p + x) < p);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow '(p+x) dont warn + " assert(x + 100U < x);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestCondition) diff --git a/test/testother.cpp b/test/testother.cpp index b0f6dcd10..b7b18964d 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -157,8 +157,6 @@ private: TEST_CASE(raceAfterInterlockedDecrement); TEST_CASE(testUnusedLabel); - - TEST_CASE(checkInvalidTestForOverflow); } void check(const char code[], const char *filename = nullptr, bool experimental = false, bool inconclusive = true, bool runSimpleChecks=true, Settings* settings = 0) { @@ -6019,23 +6017,6 @@ private: "}"); ASSERT_EQUALS("", errout.str()); } - - void checkInvalidTestForOverflow() { - check("void f(char *p, unsigned int x) {\n" - " assert((p + x) < p);\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow '(p+x) dont warn - " assert(x + 100U < x);\n" - "}"); - ASSERT_EQUALS("", errout.str()); - } }; REGISTER_TEST(TestOther)