Skip to content

Commit b3b7ecc

Browse files
danmardanmar
committed
Removed integerOverflowOptimization checking and merged functionality into invalidTestForOverflow
1 parent abd6c00 commit b3b7ecc

6 files changed

Lines changed: 148 additions & 208 deletions

File tree

‎lib/checkcondition.cpp‎

Lines changed: 82 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
#include <utility>
4040

4141
// CWE ids used
42+
static const struct CWE uncheckedErrorConditionCWE(391U);
4243
static const struct CWE CWE398(398U); // Indicator of Poor Code Quality
4344
static const struct CWE CWE570(570U); // Expression is Always False
4445
static const struct CWE CWE571(571U); // Expression is Always True
@@ -1494,63 +1495,103 @@ void CheckCondition::alwaysTrueFalseError(const Token *tok, const ValueFlow::Val
14941495

14951496
void CheckCondition::checkInvalidTestForOverflow()
14961497
{
1498+
// Interesting blogs:
1499+
// https://www.airs.com/blog/archives/120
1500+
// https://kristerw.blogspot.com/2016/02/how-undefined-signed-overflow-enables.html
1501+
// https://research.checkpoint.com/2020/optout-compiler-undefined-behavior-optimizations/
1502+
1503+
// x + c < x -> false
1504+
// x + c <= x -> false
1505+
// x + c > x -> true
1506+
// x + c >= x -> true
1507+
1508+
// x + y < x -> y < 0
1509+
1510+
14971511
if (!mSettings->isEnabled(Settings::WARNING))
14981512
return;
14991513

1500-
const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
1501-
for (const Scope * scope : symbolDatabase->functionScopes) {
1514+
for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) {
1515+
if (!Token::Match(tok, "<|<=|>=|>") || !tok->isBinaryOp())
1516+
continue;
15021517

1503-
for (const Token* tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) {
1504-
if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2())
1505-
continue;
1518+
const Token *lhsTokens[2] = {tok->astOperand1(), tok->astOperand2()};
1519+
for (const Token *lhs: lhsTokens) {
1520+
std::string cmp = tok->str();
1521+
if (lhs == tok->astOperand2())
1522+
cmp[0] = (cmp[0] == '<') ? '>' : '<';
15061523

1507-
const Token *calcToken, *exprToken;
1508-
bool result;
1509-
if (Token::Match(tok, "<|>=") && tok->astOperand1()->str() == "+") {
1510-
calcToken = tok->astOperand1();
1511-
exprToken = tok->astOperand2();
1512-
result = (tok->str() == ">=");
1513-
} else if (Token::Match(tok, ">|<=") && tok->astOperand2()->str() == "+") {
1514-
calcToken = tok->astOperand2();
1515-
exprToken = tok->astOperand1();
1516-
result = (tok->str() == "<=");
1517-
} else
1524+
if (!Token::Match(lhs, "[+-]") || !lhs->isBinaryOp())
15181525
continue;
15191526

1520-
// Only warn for signed integer overflows and pointer overflows.
1521-
if (!(calcToken->valueType() && (calcToken->valueType()->pointer || calcToken->valueType()->sign == ValueType::Sign::SIGNED)))
1522-
continue;
1523-
if (!(exprToken->valueType() && (exprToken->valueType()->pointer || exprToken->valueType()->sign == ValueType::Sign::SIGNED)))
1527+
const bool isSignedInteger = lhs->valueType() && lhs->valueType()->isIntegral() && lhs->valueType()->sign == ValueType::Sign::SIGNED;
1528+
const bool isPointer = lhs->valueType() && lhs->valueType()->pointer > 0;
1529+
if (!isSignedInteger && !isPointer)
15241530
continue;
15251531

1526-
const Token *termToken;
1527-
if (isSameExpression(mTokenizer->isCPP(), true, exprToken, calcToken->astOperand1(), mSettings->library, true, false))
1528-
termToken = calcToken->astOperand2();
1529-
else if (isSameExpression(mTokenizer->isCPP(), true, exprToken, calcToken->astOperand2(), mSettings->library, true, false))
1530-
termToken = calcToken->astOperand1();
1531-
else
1532-
continue;
1532+
const Token *exprTokens[2] = {lhs->astOperand1(), lhs->astOperand2()};
1533+
for (const Token *expr: exprTokens) {
1534+
if (lhs->str() == "-" && expr == lhs->astOperand2())
1535+
continue; // TODO?
15331536

1534-
if (!termToken)
1535-
continue;
1537+
if (expr->hasKnownIntValue())
1538+
continue;
1539+
1540+
if (!isSameExpression(mTokenizer->isCPP(),
1541+
true,
1542+
expr,
1543+
lhs->astSibling(),
1544+
mSettings->library,
1545+
true,
1546+
false))
1547+
continue;
1548+
1549+
const Token * const other = expr->astSibling();
1550+
1551+
// x [+-] c cmp x
1552+
if ((other->isNumber() && other->getKnownIntValue() > 0) ||
1553+
(!other->isNumber() && other->valueType() && other->valueType()->isIntegral() && other->valueType()->sign == ValueType::Sign::UNSIGNED)) {
1554+
bool result;
1555+
if (lhs->str() == "+")
1556+
result = (cmp == ">" || cmp == ">=");
1557+
else
1558+
result = (cmp == "<" || cmp == "<=");
1559+
invalidTestForOverflow(tok, lhs->valueType(), result ? "true" : "false");
1560+
continue;
1561+
}
15361562

1537-
// Only warn when termToken is always positive
1538-
if (termToken->valueType() && termToken->valueType()->sign == ValueType::Sign::UNSIGNED)
1539-
invalidTestForOverflow(tok, result);
1540-
else if (termToken->isNumber() && MathLib::isPositive(termToken->str()))
1541-
invalidTestForOverflow(tok, result);
1563+
// x + y cmp x
1564+
if (lhs->str() == "+" && other->varId() > 0) {
1565+
const std::string result = other->str() + cmp + "0";
1566+
invalidTestForOverflow(tok, lhs->valueType(), result);
1567+
continue;
1568+
}
1569+
1570+
// x - y cmp x
1571+
if (lhs->str() == "-" && other->varId() > 0) {
1572+
std::string cmp2 = cmp;
1573+
cmp2[0] = (cmp[0] == '<') ? '>' : '<';
1574+
const std::string result = other->str() + cmp2 + "0";
1575+
invalidTestForOverflow(tok, lhs->valueType(), result);
1576+
continue;
1577+
}
1578+
}
15421579
}
15431580
}
15441581
}
15451582

1546-
void CheckCondition::invalidTestForOverflow(const Token* tok, bool result)
1583+
void CheckCondition::invalidTestForOverflow(const Token* tok, const ValueType *valueType, const std::string &replace)
15471584
{
1548-
const std::string errmsg = "Invalid test for overflow '" +
1549-
(tok ? tok->expressionString() : std::string("x + u < x")) +
1550-
"'. Condition is always " +
1551-
std::string(result ? "true" : "false") +
1552-
" unless there is overflow, and overflow is undefined behaviour.";
1553-
reportError(tok, Severity::warning, "invalidTestForOverflow", errmsg, (result ? CWE571 : CWE570), false);
1585+
const std::string expr = (tok ? tok->expressionString() : std::string("x + c < x"));
1586+
const std::string overflow = (valueType && valueType->pointer) ? "pointer overflow" : "signed integer overflow";
1587+
1588+
std::string errmsg =
1589+
"Invalid test for overflow '" + expr + "'; " + overflow + " is undefined behavior.";
1590+
if (replace == "false" || replace == "true")
1591+
errmsg += " Some mainstream compilers remove such overflow tests when optimising the code and assume it's always " + replace + ".";
1592+
else
1593+
errmsg += " Some mainstream compilers removes handling of overflows when optimising the code and change the code to '" + replace + "'.";
1594+
reportError(tok, Severity::warning, "invalidTestForOverflow", errmsg, uncheckedErrorConditionCWE, false);
15541595
}
15551596

15561597

‎lib/checkcondition.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ class Settings;
3535
class Token;
3636
class Tokenizer;
3737
class ErrorLogger;
38+
class ValueType;
3839

3940
/// @addtogroup Checks
4041
/// @{
@@ -155,7 +156,7 @@ class CPPCHECKLIB CheckCondition : public Check {
155156

156157
void alwaysTrueFalseError(const Token *tok, const ValueFlow::Value *value);
157158

158-
void invalidTestForOverflow(const Token* tok, bool result);
159+
void invalidTestForOverflow(const Token* tok, const ValueType *valueType, const std::string &replace);
159160
void pointerAdditionResultNotNullError(const Token *tok, const Token *calc);
160161

161162
void duplicateConditionalAssignError(const Token *condTok, const Token* assignTok);
@@ -179,7 +180,7 @@ class CPPCHECKLIB CheckCondition : public Check {
179180
c.moduloAlwaysTrueFalseError(nullptr, "1");
180181
c.clarifyConditionError(nullptr, true, false);
181182
c.alwaysTrueFalseError(nullptr, nullptr);
182-
c.invalidTestForOverflow(nullptr, false);
183+
c.invalidTestForOverflow(nullptr, nullptr, "false");
183184
c.pointerAdditionResultNotNullError(nullptr, nullptr);
184185
c.duplicateConditionalAssignError(nullptr, nullptr);
185186
}
@@ -202,7 +203,7 @@ class CPPCHECKLIB CheckCondition : public Check {
202203
"- Mutual exclusion over || always evaluating to true\n"
203204
"- Comparisons of modulo results that are always true/false.\n"
204205
"- Known variable values => condition is always true/false\n"
205-
"- Invalid test for overflow (for example 'ptr+u < ptr'). Condition is always false unless there is overflow, and overflow is undefined behaviour.\n";
206+
"- Invalid test for overflow. Some mainstream compilers remove such overflow tests when optimising code.\n";
206207
}
207208
};
208209
/// @}

‎lib/checktype.cpp‎

Lines changed: 0 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -223,97 +223,6 @@ void CheckType::integerOverflowError(const Token *tok, const ValueFlow::Value &v
223223
value.isInconclusive());
224224
}
225225

226-
//---------------------------------------------------------------------------------
227-
// Checking for code patterns that might be optimised differently by the compilers
228-
//---------------------------------------------------------------------------------
229-
230-
void CheckType::checkIntegerOverflowOptimisations()
231-
{
232-
// Interesting blogs:
233-
// https://www.airs.com/blog/archives/120
234-
// https://kristerw.blogspot.com/2016/02/how-undefined-signed-overflow-enables.html
235-
// https://research.checkpoint.com/2020/optout-compiler-undefined-behavior-optimizations/
236-
237-
// x + c < x -> false
238-
// x + c <= x -> false
239-
// x + c > x -> true
240-
// x + c >= x -> true
241-
242-
// x + y < x -> y < 0
243-
244-
245-
if (!mSettings->isEnabled(Settings::PORTABILITY))
246-
return;
247-
248-
for (const Token *tok = mTokenizer->tokens(); tok; tok = tok->next()) {
249-
if (!Token::Match(tok, "<|<=|>=|>") || !tok->isBinaryOp())
250-
continue;
251-
252-
const Token *lhsTokens[2] = {tok->astOperand1(), tok->astOperand2()};
253-
for (const Token *lhs: lhsTokens) {
254-
std::string cmp = tok->str();
255-
if (lhs == tok->astOperand2())
256-
cmp[0] = (cmp[0] == '<') ? '>' : '<';
257-
258-
if (!Token::Match(lhs, "[+-]") || !lhs->isBinaryOp() || !lhs->valueType() || !lhs->valueType()->isIntegral() || lhs->valueType()->sign != ValueType::Sign::SIGNED)
259-
continue;
260-
261-
const Token *exprTokens[2] = {lhs->astOperand1(), lhs->astOperand2()};
262-
for (const Token *expr: exprTokens) {
263-
if (lhs->str() == "-" && expr == lhs->astOperand2())
264-
continue; // TODO?
265-
266-
if (expr->hasKnownIntValue())
267-
continue;
268-
269-
if (!isSameExpression(mTokenizer->isCPP(),
270-
false,
271-
expr,
272-
lhs->astSibling(),
273-
mSettings->library,
274-
false,
275-
false))
276-
continue;
277-
278-
const Token * const other = expr->astSibling();
279-
280-
// x [+-] c cmp x
281-
if (other->isNumber() && other->getKnownIntValue() > 0) {
282-
bool result;
283-
if (lhs->str() == "+")
284-
result = (cmp == ">" || cmp == ">=");
285-
else
286-
result = (cmp == "<" || cmp == "<=");
287-
integerOverflowOptimisationError(tok, result ? "true" : "false");
288-
}
289-
290-
// x + y cmp x
291-
if (lhs->str() == "+" && other->varId() > 0) {
292-
const std::string result = other->str() + cmp + "0";
293-
integerOverflowOptimisationError(tok, result);
294-
}
295-
296-
// x - y cmp x
297-
if (lhs->str() == "-" && other->varId() > 0) {
298-
std::string cmp2 = cmp;
299-
cmp2[0] = (cmp[0] == '<') ? '>' : '<';
300-
const std::string result = other->str() + cmp2 + "0";
301-
integerOverflowOptimisationError(tok, result);
302-
}
303-
}
304-
}
305-
}
306-
}
307-
308-
void CheckType::integerOverflowOptimisationError(const Token *tok, const std::string &replace)
309-
{
310-
const std::string expr = tok ? tok->expressionString() : "x+c<x";
311-
const std::string errmsg =
312-
"There is a danger that '" + expr + "' will be optimised into '" + replace + "'. Signed integer overflow is undefined behavior.\n"
313-
"There is a danger that '" + expr + "' will be optimised into '" + replace + "'. Your code could work differently depending on what compiler/flags/version/etc is used. Signed integer overflow is undefined behavior and assuming that it will never happen; the result of '" + expr + "' is always '" + replace + "'.";
314-
reportError(tok, Severity::portability, "integerOverflowOptimization", errmsg, CWE190, false);
315-
}
316-
317226
//---------------------------------------------------------------------------
318227
// Checking for sign conversion when operand can be negative
319228
//---------------------------------------------------------------------------

‎lib/checktype.h‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,6 @@ class CPPCHECKLIB CheckType : public Check {
5454
CheckType checkType(tokenizer, settings, errorLogger);
5555
checkType.checkTooBigBitwiseShift();
5656
checkType.checkIntegerOverflow();
57-
checkType.checkIntegerOverflowOptimisations();
5857
checkType.checkSignConversion();
5958
checkType.checkLongCast();
6059
checkType.checkFloatToIntegerOverflow();
@@ -66,9 +65,6 @@ class CPPCHECKLIB CheckType : public Check {
6665
/** @brief %Check for integer overflow */
6766
void checkIntegerOverflow();
6867

69-
/** @brief Check for overflow code patterns that will be optimized */
70-
void checkIntegerOverflowOptimisations();
71-
7268
/** @brief %Check for dangerous sign conversion */
7369
void checkSignConversion();
7470

@@ -85,7 +81,6 @@ class CPPCHECKLIB CheckType : public Check {
8581
void tooBigBitwiseShiftError(const Token *tok, int lhsbits, const ValueFlow::Value &rhsbits);
8682
void tooBigSignedBitwiseShiftError(const Token *tok, int lhsbits, const ValueFlow::Value &rhsbits);
8783
void integerOverflowError(const Token *tok, const ValueFlow::Value &value);
88-
void integerOverflowOptimisationError(const Token *tok, const std::string &replace);
8984
void signConversionError(const Token *tok, const ValueFlow::Value *negativeValue, const bool constvalue);
9085
void longCastAssignError(const Token *tok);
9186
void longCastReturnError(const Token *tok);
@@ -113,7 +108,6 @@ class CPPCHECKLIB CheckType : public Check {
113108
return "Type checks\n"
114109
"- bitwise shift by too many bits (only enabled when --platform is used)\n"
115110
"- signed integer overflow (only enabled when --platform is used)\n"
116-
"- expression that can be optimised 'out' because signed integer overflow is undefined behavior.\n"
117111
"- dangerous sign conversion, when signed value can be negative\n"
118112
"- possible loss of information when assigning int result to long variable\n"
119113
"- possible loss of information when returning int result as long return value\n"

0 commit comments

Comments
 (0)