From b3b7ecc7ea15b806bdfcb8eab13fc6feb1fd7f78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 5 Jan 2021 11:38:19 +0100 Subject: [PATCH] Removed integerOverflowOptimization checking and merged functionality into invalidTestForOverflow --- lib/checkcondition.cpp | 123 +++++++++++++++++++++++++++-------------- lib/checkcondition.h | 7 ++- lib/checktype.cpp | 91 ------------------------------ lib/checktype.h | 6 -- test/testcondition.cpp | 71 +++++++++++++++++++++--- test/testtype.cpp | 58 ------------------- 6 files changed, 148 insertions(+), 208 deletions(-) diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 0da1288fc..ab43412b5 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -39,6 +39,7 @@ #include // CWE ids used +static const struct CWE uncheckedErrorConditionCWE(391U); static const struct CWE CWE398(398U); // Indicator of Poor Code Quality static const struct CWE CWE570(570U); // Expression is Always False static const struct CWE CWE571(571U); // Expression is Always True @@ -1494,63 +1495,103 @@ void CheckCondition::alwaysTrueFalseError(const Token *tok, const ValueFlow::Val void CheckCondition::checkInvalidTestForOverflow() { + // Interesting blogs: + // https://www.airs.com/blog/archives/120 + // https://kristerw.blogspot.com/2016/02/how-undefined-signed-overflow-enables.html + // https://research.checkpoint.com/2020/optout-compiler-undefined-behavior-optimizations/ + + // x + c < x -> false + // x + c <= x -> false + // x + c > x -> true + // x + c >= x -> true + + // x + y < x -> y < 0 + + if (!mSettings->isEnabled(Settings::WARNING)) return; - const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); - for (const Scope * scope : symbolDatabase->functionScopes) { + for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) { + if (!Token::Match(tok, "<|<=|>=|>") || !tok->isBinaryOp()) + continue; - for (const Token* tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) { - if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2()) + const Token *lhsTokens[2] = {tok->astOperand1(), tok->astOperand2()}; + for (const Token *lhs: lhsTokens) { + std::string cmp = tok->str(); + if (lhs == tok->astOperand2()) + cmp[0] = (cmp[0] == '<') ? '>' : '<'; + + if (!Token::Match(lhs, "[+-]") || !lhs->isBinaryOp()) continue; - const Token *calcToken, *exprToken; - bool result; - if (Token::Match(tok, "<|>=") && tok->astOperand1()->str() == "+") { - calcToken = tok->astOperand1(); - exprToken = tok->astOperand2(); - result = (tok->str() == ">="); - } else if (Token::Match(tok, ">|<=") && tok->astOperand2()->str() == "+") { - calcToken = tok->astOperand2(); - exprToken = tok->astOperand1(); - result = (tok->str() == "<="); - } else + const bool isSignedInteger = lhs->valueType() && lhs->valueType()->isIntegral() && lhs->valueType()->sign == ValueType::Sign::SIGNED; + const bool isPointer = lhs->valueType() && lhs->valueType()->pointer > 0; + if (!isSignedInteger && !isPointer) 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 *exprTokens[2] = {lhs->astOperand1(), lhs->astOperand2()}; + for (const Token *expr: exprTokens) { + if (lhs->str() == "-" && expr == lhs->astOperand2()) + continue; // TODO? - const Token *termToken; - if (isSameExpression(mTokenizer->isCPP(), true, exprToken, calcToken->astOperand1(), mSettings->library, true, false)) - termToken = calcToken->astOperand2(); - else if (isSameExpression(mTokenizer->isCPP(), true, exprToken, calcToken->astOperand2(), mSettings->library, true, false)) - termToken = calcToken->astOperand1(); - else - continue; + if (expr->hasKnownIntValue()) + continue; - if (!termToken) - continue; + if (!isSameExpression(mTokenizer->isCPP(), + true, + expr, + lhs->astSibling(), + mSettings->library, + true, + false)) + continue; - // Only warn when termToken is always positive - if (termToken->valueType() && termToken->valueType()->sign == ValueType::Sign::UNSIGNED) - invalidTestForOverflow(tok, result); - else if (termToken->isNumber() && MathLib::isPositive(termToken->str())) - invalidTestForOverflow(tok, result); + const Token * const other = expr->astSibling(); + + // x [+-] c cmp x + if ((other->isNumber() && other->getKnownIntValue() > 0) || + (!other->isNumber() && other->valueType() && other->valueType()->isIntegral() && other->valueType()->sign == ValueType::Sign::UNSIGNED)) { + bool result; + if (lhs->str() == "+") + result = (cmp == ">" || cmp == ">="); + else + result = (cmp == "<" || cmp == "<="); + invalidTestForOverflow(tok, lhs->valueType(), result ? "true" : "false"); + continue; + } + + // x + y cmp x + if (lhs->str() == "+" && other->varId() > 0) { + const std::string result = other->str() + cmp + "0"; + invalidTestForOverflow(tok, lhs->valueType(), result); + continue; + } + + // x - y cmp x + if (lhs->str() == "-" && other->varId() > 0) { + std::string cmp2 = cmp; + cmp2[0] = (cmp[0] == '<') ? '>' : '<'; + const std::string result = other->str() + cmp2 + "0"; + invalidTestForOverflow(tok, lhs->valueType(), result); + continue; + } + } } } } -void CheckCondition::invalidTestForOverflow(const Token* tok, bool result) +void CheckCondition::invalidTestForOverflow(const Token* tok, const ValueType *valueType, const std::string &replace) { - const std::string errmsg = "Invalid test for overflow '" + - (tok ? tok->expressionString() : std::string("x + u < x")) + - "'. Condition is always " + - std::string(result ? "true" : "false") + - " unless there is overflow, and overflow is undefined behaviour."; - reportError(tok, Severity::warning, "invalidTestForOverflow", errmsg, (result ? CWE571 : CWE570), false); + const std::string expr = (tok ? tok->expressionString() : std::string("x + c < x")); + const std::string overflow = (valueType && valueType->pointer) ? "pointer overflow" : "signed integer overflow"; + + std::string errmsg = + "Invalid test for overflow '" + expr + "'; " + overflow + " is undefined behavior."; + if (replace == "false" || replace == "true") + errmsg += " Some mainstream compilers remove such overflow tests when optimising the code and assume it's always " + replace + "."; + else + errmsg += " Some mainstream compilers removes handling of overflows when optimising the code and change the code to '" + replace + "'."; + reportError(tok, Severity::warning, "invalidTestForOverflow", errmsg, uncheckedErrorConditionCWE, false); } diff --git a/lib/checkcondition.h b/lib/checkcondition.h index c3ec666bd..b2e49cca0 100644 --- a/lib/checkcondition.h +++ b/lib/checkcondition.h @@ -35,6 +35,7 @@ class Settings; class Token; class Tokenizer; class ErrorLogger; +class ValueType; /// @addtogroup Checks /// @{ @@ -155,7 +156,7 @@ private: void alwaysTrueFalseError(const Token *tok, const ValueFlow::Value *value); - void invalidTestForOverflow(const Token* tok, bool result); + void invalidTestForOverflow(const Token* tok, const ValueType *valueType, const std::string &replace); void pointerAdditionResultNotNullError(const Token *tok, const Token *calc); void duplicateConditionalAssignError(const Token *condTok, const Token* assignTok); @@ -179,7 +180,7 @@ private: c.moduloAlwaysTrueFalseError(nullptr, "1"); c.clarifyConditionError(nullptr, true, false); c.alwaysTrueFalseError(nullptr, nullptr); - c.invalidTestForOverflow(nullptr, false); + c.invalidTestForOverflow(nullptr, nullptr, "false"); c.pointerAdditionResultNotNullError(nullptr, nullptr); c.duplicateConditionalAssignError(nullptr, nullptr); } @@ -202,7 +203,7 @@ private: "- 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" - "- Invalid test for overflow (for example 'ptr+u < ptr'). Condition is always false unless there is overflow, and overflow is undefined behaviour.\n"; + "- Invalid test for overflow. Some mainstream compilers remove such overflow tests when optimising code.\n"; } }; /// @} diff --git a/lib/checktype.cpp b/lib/checktype.cpp index 1258eb5b3..6bd7c344f 100644 --- a/lib/checktype.cpp +++ b/lib/checktype.cpp @@ -223,97 +223,6 @@ void CheckType::integerOverflowError(const Token *tok, const ValueFlow::Value &v value.isInconclusive()); } -//--------------------------------------------------------------------------------- -// Checking for code patterns that might be optimised differently by the compilers -//--------------------------------------------------------------------------------- - -void CheckType::checkIntegerOverflowOptimisations() -{ - // Interesting blogs: - // https://www.airs.com/blog/archives/120 - // https://kristerw.blogspot.com/2016/02/how-undefined-signed-overflow-enables.html - // https://research.checkpoint.com/2020/optout-compiler-undefined-behavior-optimizations/ - - // x + c < x -> false - // x + c <= x -> false - // x + c > x -> true - // x + c >= x -> true - - // x + y < x -> y < 0 - - - if (!mSettings->isEnabled(Settings::PORTABILITY)) - return; - - for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) { - if (!Token::Match(tok, "<|<=|>=|>") || !tok->isBinaryOp()) - continue; - - const Token *lhsTokens[2] = {tok->astOperand1(), tok->astOperand2()}; - for (const Token *lhs: lhsTokens) { - std::string cmp = tok->str(); - if (lhs == tok->astOperand2()) - cmp[0] = (cmp[0] == '<') ? '>' : '<'; - - if (!Token::Match(lhs, "[+-]") || !lhs->isBinaryOp() || !lhs->valueType() || !lhs->valueType()->isIntegral() || lhs->valueType()->sign != ValueType::Sign::SIGNED) - continue; - - const Token *exprTokens[2] = {lhs->astOperand1(), lhs->astOperand2()}; - for (const Token *expr: exprTokens) { - if (lhs->str() == "-" && expr == lhs->astOperand2()) - continue; // TODO? - - if (expr->hasKnownIntValue()) - continue; - - if (!isSameExpression(mTokenizer->isCPP(), - false, - expr, - lhs->astSibling(), - mSettings->library, - false, - false)) - continue; - - const Token * const other = expr->astSibling(); - - // x [+-] c cmp x - if (other->isNumber() && other->getKnownIntValue() > 0) { - bool result; - if (lhs->str() == "+") - result = (cmp == ">" || cmp == ">="); - else - result = (cmp == "<" || cmp == "<="); - integerOverflowOptimisationError(tok, result ? "true" : "false"); - } - - // x + y cmp x - if (lhs->str() == "+" && other->varId() > 0) { - const std::string result = other->str() + cmp + "0"; - integerOverflowOptimisationError(tok, result); - } - - // x - y cmp x - if (lhs->str() == "-" && other->varId() > 0) { - std::string cmp2 = cmp; - cmp2[0] = (cmp[0] == '<') ? '>' : '<'; - const std::string result = other->str() + cmp2 + "0"; - integerOverflowOptimisationError(tok, result); - } - } - } - } -} - -void CheckType::integerOverflowOptimisationError(const Token *tok, const std::string &replace) -{ - const std::string expr = tok ? tok->expressionString() : "x+c= p);\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow '(p+x)>=p'. Condition is always true unless there is overflow, and overflow is undefined behaviour.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow '(p+x)>=p'; pointer overflow is undefined behavior. Some mainstream compilers remove such overflow tests when optimising the code and assume it's always true.\n", errout.str()); check("void f(char *p, unsigned int x) {\n" " assert(p > (p + x));\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow 'p>(p+x)'. Condition is always false unless there is overflow, and overflow is undefined behaviour.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow 'p>(p+x)'; pointer overflow is undefined behavior. Some mainstream compilers remove such overflow tests when optimising the code and assume it's always false.\n", errout.str()); check("void f(char *p, unsigned int x) {\n" " assert(p <= (p + x));\n" "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow 'p<=(p+x)'. Condition is always true unless there is overflow, and overflow is undefined behaviour.\n", errout.str()); - - check("void f(signed int x) {\n" - " assert(x + 100 < x);\n" - "}"); - ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow 'x+100 don't warn " assert(x + 100U < x);\n" "}"); ASSERT_EQUALS("", errout.str()); + + + // x + c < x + +#define MSG(EXPR, RESULT) "[test.cpp:1]: (warning) Invalid test for overflow '" EXPR "'; signed integer overflow is undefined behavior. Some mainstream compilers remove such overflow tests when optimising the code and assume it's always " RESULT ".\n" + + check("int f(int x) { return x + 10 > x; }"); + ASSERT_EQUALS(MSG("x+10>x", "true"), errout.str()); + + check("int f(int x) { return x + 10 >= x; }"); + ASSERT_EQUALS(MSG("x+10>=x", "true"), errout.str()); + + check("int f(int x) { return x + 10 < x; }"); + ASSERT_EQUALS(MSG("x+10 x; }"); + ASSERT_EQUALS(MSG("x-10>x", "false"), errout.str()); + + check("int f(int x) { return x - 10 >= x; }"); + ASSERT_EQUALS(MSG("x-10>=x", "false"), errout.str()); + + check("int f(int x) { return x - 10 < x; }"); + ASSERT_EQUALS(MSG("x-10 x; }"); + ASSERT_EQUALS(MSG("x+y>x", "y>0"), errout.str()); + + check("int f(int x, int y) { return x + y >= x; }"); + ASSERT_EQUALS(MSG("x+y>=x", "y>=0"), errout.str()); + + // x - y < x + check("int f(int x, int y) { return x - y < x; }"); + ASSERT_EQUALS(MSG("x-y0"), errout.str()); + + check("int f(int x, int y) { return x - y <= x; }"); + ASSERT_EQUALS(MSG("x-y<=x", "y>=0"), errout.str()); + + check("int f(int x, int y) { return x - y > x; }"); + ASSERT_EQUALS(MSG("x-y>x", "y<0"), errout.str()); + + check("int f(int x, int y) { return x - y >= x; }"); + ASSERT_EQUALS(MSG("x-y>=x", "y<=0"), errout.str()); } void checkConditionIsAlwaysTrueOrFalseInsideIfWhile() { diff --git a/test/testtype.cpp b/test/testtype.cpp index 790dd119b..4a121a09a 100644 --- a/test/testtype.cpp +++ b/test/testtype.cpp @@ -35,7 +35,6 @@ private: void run() OVERRIDE { TEST_CASE(checkTooBigShift_Unix32); TEST_CASE(checkIntegerOverflow); - TEST_CASE(checkIntegerOverflowOptimisations); TEST_CASE(signConversion); TEST_CASE(longCastAssign); TEST_CASE(longCastReturn); @@ -249,63 +248,6 @@ private: ASSERT_EQUALS("", errout.str()); } - void checkIntegerOverflowOptimisations() { - Settings settings; - settings.addEnabled("warning"); - - // x + c < x - - check("int f(int x) { return x + 10 > x; }", &settings); - ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+10>x' will be optimised into 'true'. Signed integer overflow is undefined behavior.\n", errout.str()); - - check("int f(int x) { return x + 10 >= x; }", &settings); - ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+10>=x' will be optimised into 'true'. Signed integer overflow is undefined behavior.\n", errout.str()); - - check("int f(int x) { return x + 10 < x; }", &settings); - ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+10 x; }", &settings); - ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-10>x' will be optimised into 'false'. Signed integer overflow is undefined behavior.\n", errout.str()); - - check("int f(int x) { return x - 10 >= x; }", &settings); - ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-10>=x' will be optimised into 'false'. Signed integer overflow is undefined behavior.\n", errout.str()); - - check("int f(int x) { return x - 10 < x; }", &settings); - ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-10 x; }", &settings); - ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+y>x' will be optimised into 'y>0'. Signed integer overflow is undefined behavior.\n", errout.str()); - - check("int f(int x, int y) { return x + y >= x; }", &settings); - ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x+y>=x' will be optimised into 'y>=0'. Signed integer overflow is undefined behavior.\n", errout.str()); - - // x - y < x - check("int f(int x, int y) { return x - y < x; }", &settings); - ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-y0'. Signed integer overflow is undefined behavior.\n", errout.str()); - - check("int f(int x, int y) { return x - y <= x; }", &settings); - ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-y<=x' will be optimised into 'y>=0'. Signed integer overflow is undefined behavior.\n", errout.str()); - - check("int f(int x, int y) { return x - y > x; }", &settings); - ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-y>x' will be optimised into 'y<0'. Signed integer overflow is undefined behavior.\n", errout.str()); - - check("int f(int x, int y) { return x - y >= x; }", &settings); - ASSERT_EQUALS("[test.cpp:1]: (portability) There is a danger that 'x-y>=x' will be optimised into 'y<=0'. Signed integer overflow is undefined behavior.\n", errout.str()); - } - void signConversion() { check("x = -4 * (unsigned)y;"); ASSERT_EQUALS("[test.cpp:1]: (warning) Expression '-4' has a negative value. That is converted to an unsigned value and used in an unsigned calculation.\n", errout.str());