Skip to content

Commit fb8cce6

Browse files
danmardanmar
committed
invalidTestForOverflow: Refactor; move from checkother to checkcondition
1 parent f6f4f27 commit fb8cce6

6 files changed

Lines changed: 89 additions & 88 deletions

File tree

‎lib/checkcondition.cpp‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,3 +1014,62 @@ void CheckCondition::alwaysTrueFalseError(const Token *tok, bool knownResult)
10141014
"knownConditionTrueFalse",
10151015
"Condition '" + expr + "' is always " + (knownResult ? "true" : "false"));
10161016
}
1017+
1018+
void CheckCondition::checkInvalidTestForOverflow()
1019+
{
1020+
if (!_settings->isEnabled("warning"))
1021+
return;
1022+
1023+
const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
1024+
const std::size_t functions = symbolDatabase->functionScopes.size();
1025+
for (std::size_t i = 0; i < functions; ++i) {
1026+
const Scope * scope = symbolDatabase->functionScopes[i];
1027+
1028+
for (const Token* tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) {
1029+
if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2())
1030+
continue;
1031+
1032+
const Token *calcToken, *exprToken;
1033+
if (tok->str() == "<" && tok->astOperand1()->str() == "+") {
1034+
calcToken = tok->astOperand1();
1035+
exprToken = tok->astOperand2();
1036+
} else if (tok->str() == ">" && tok->astOperand2()->str() == "+") {
1037+
calcToken = tok->astOperand2();
1038+
exprToken = tok->astOperand1();
1039+
} else
1040+
continue;
1041+
1042+
// Only warn for signed integer overflows and pointer overflows.
1043+
if (!(calcToken->valueType() && (calcToken->valueType()->pointer || calcToken->valueType()->sign == ValueType::Sign::SIGNED)))
1044+
continue;
1045+
if (!(exprToken->valueType() && (exprToken->valueType()->pointer || exprToken->valueType()->sign == ValueType::Sign::SIGNED)))
1046+
continue;
1047+
1048+
const Token *termToken;
1049+
if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand1(), _settings->library.functionpure))
1050+
termToken = calcToken->astOperand2();
1051+
else if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand2(), _settings->library.functionpure))
1052+
termToken = calcToken->astOperand1();
1053+
else
1054+
continue;
1055+
1056+
if (!termToken)
1057+
continue;
1058+
1059+
// Only warn when termToken is always positive
1060+
if (termToken->valueType() && termToken->valueType()->sign == ValueType::Sign::UNSIGNED)
1061+
invalidTestForOverflow(tok);
1062+
else if (termToken->isNumber() && MathLib::isPositive(termToken->str()))
1063+
invalidTestForOverflow(tok);
1064+
}
1065+
}
1066+
}
1067+
1068+
void CheckCondition::invalidTestForOverflow(const Token* tok)
1069+
{
1070+
std::string errmsg;
1071+
errmsg = "Invalid test for overflow '" +
1072+
(tok ? tok->expressionString() : std::string("x + u < x")) +
1073+
"'. Condition is always false unless there is overflow, and overflow is UB.";
1074+
reportError(tok, Severity::warning, "invalidTestForOverflow", errmsg);
1075+
}

‎lib/checkcondition.h‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ class CPPCHECKLIB CheckCondition : public Check {
5050
checkCondition.clarifyCondition(); // not simplified because ifAssign
5151
checkCondition.oppositeInnerCondition();
5252
checkCondition.checkIncorrectLogicOperator();
53+
checkCondition.checkInvalidTestForOverflow();
5354
}
5455

5556
/** @brief Run checks against the simplified token list */
@@ -97,6 +98,9 @@ class CPPCHECKLIB CheckCondition : public Check {
9798
/** @brief Condition is always true/false */
9899
void alwaysTrueFalse();
99100

101+
/** @brief %Check for invalid test for overflow 'x+100 < x' */
102+
void checkInvalidTestForOverflow();
103+
100104
private:
101105

102106
bool isOverlappingCond(const Token * const cond1, const Token * const cond2, const std::set<std::string> &constFunctions) const;
@@ -122,6 +126,8 @@ class CPPCHECKLIB CheckCondition : public Check {
122126

123127
void alwaysTrueFalseError(const Token *tok, bool knownResult);
124128

129+
void invalidTestForOverflow(const Token* tok);
130+
125131
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
126132
CheckCondition c(0, settings, errorLogger);
127133

@@ -136,6 +142,7 @@ class CPPCHECKLIB CheckCondition : public Check {
136142
c.moduloAlwaysTrueFalseError(0, "1");
137143
c.clarifyConditionError(0, true, false);
138144
c.alwaysTrueFalseError(0, true);
145+
c.invalidTestForOverflow(0);
139146
}
140147

141148
static std::string myName() {
@@ -150,10 +157,11 @@ class CPPCHECKLIB CheckCondition : public Check {
150157
"- Detect matching 'if' and 'else if' conditions\n"
151158
"- Mismatching bitand (a &= 0xf0; a &= 1; => a = 0)\n"
152159
"- Find dead code which is inaccessible due to the counter-conditions check in nested if statements\n"
153-
"- condition that is always true/false\n"
154-
"- mutual exclusion over || always evaluating to true\n"
160+
"- Condition that is always true/false\n"
161+
"- Mutual exclusion over || always evaluating to true\n"
155162
"- Comparisons of modulo results that are always true/false.\n"
156-
"- Known variable values => condition is always true/false\n";
163+
"- Known variable values => condition is always true/false\n"
164+
"- Invalid test for overflow (for example 'ptr+u < ptr'). Condition is always false unless there is overflow, and overflow is UB.\n";
157165
}
158166
};
159167
/// @}

‎lib/checkother.cpp‎

Lines changed: 0 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -2455,62 +2455,3 @@ void CheckOther::unusedLabelError(const Token* tok)
24552455
reportError(tok, Severity::style, "unusedLabel",
24562456
"Label '" + (tok?tok->str():emptyString) + "' is not used.");
24572457
}
2458-
2459-
void CheckOther::checkInvalidTestForOverflow()
2460-
{
2461-
if (!_settings->isEnabled("warning"))
2462-
return;
2463-
2464-
const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
2465-
const std::size_t functions = symbolDatabase->functionScopes.size();
2466-
for (std::size_t i = 0; i < functions; ++i) {
2467-
const Scope * scope = symbolDatabase->functionScopes[i];
2468-
2469-
for (const Token* tok = scope->classStart; tok != scope->classEnd; tok = tok->next()) {
2470-
if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2())
2471-
continue;
2472-
2473-
const Token *calcToken, *exprToken;
2474-
if (Token::Match(tok, "<|<=") && tok->astOperand1()->str() == "+") {
2475-
calcToken = tok->astOperand1();
2476-
exprToken = tok->astOperand2();
2477-
} else if (Token::Match(tok, ">|>=") && tok->astOperand2()->str() == "+") {
2478-
calcToken = tok->astOperand2();
2479-
exprToken = tok->astOperand1();
2480-
} else
2481-
continue;
2482-
2483-
// Only warn for signed integer overflows and pointer overflows.
2484-
if (!(calcToken->valueType() && (calcToken->valueType()->pointer || calcToken->valueType()->sign == ValueType::Sign::SIGNED)))
2485-
continue;
2486-
if (!(exprToken->valueType() && (exprToken->valueType()->pointer || exprToken->valueType()->sign == ValueType::Sign::SIGNED)))
2487-
continue;
2488-
2489-
const Token *termToken;
2490-
if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand1(), _settings->library.functionpure))
2491-
termToken = calcToken->astOperand2();
2492-
else if (isSameExpression(_tokenizer->isCPP(), exprToken, calcToken->astOperand2(), _settings->library.functionpure))
2493-
termToken = calcToken->astOperand1();
2494-
else
2495-
continue;
2496-
2497-
if (!termToken)
2498-
continue;
2499-
2500-
// Only warn when termToken is always positive
2501-
if (termToken->valueType() && termToken->valueType()->sign == ValueType::Sign::UNSIGNED)
2502-
invalidTestForOverflow(tok);
2503-
else if (termToken->isNumber() && MathLib::isPositive(termToken->str()))
2504-
invalidTestForOverflow(tok);
2505-
}
2506-
}
2507-
}
2508-
2509-
void CheckOther::invalidTestForOverflow(const Token* tok)
2510-
{
2511-
std::string errmsg;
2512-
errmsg = "Invalid test for overflow '" +
2513-
(tok ? tok->expressionString() : std::string("x + u < x")) +
2514-
"'. Condition is always false unless there is overflow, and overflow is UB.";
2515-
reportError(tok, Severity::warning, "invalidTestForOverflow", errmsg);
2516-
}

‎lib/checkother.h‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,6 @@ class CPPCHECKLIB CheckOther : public Check {
7070
checkOther.checkZeroDivision();
7171
checkOther.checkInterlockedDecrement();
7272
checkOther.checkUnusedLabel();
73-
checkOther.checkInvalidTestForOverflow();
7473
}
7574

7675
/** @brief Run checks against the simplified token list */
@@ -204,9 +203,6 @@ class CPPCHECKLIB CheckOther : public Check {
204203
/** @brief %Check for unused labels */
205204
void checkUnusedLabel();
206205

207-
/** @brief %Check for invalid test for overflow 'x+100 < x' */
208-
void checkInvalidTestForOverflow();
209-
210206
private:
211207
// Error messages..
212208
void checkComparisonFunctionIsAlwaysTrueOrFalseError(const Token* tok, const std::string &strFunctionName, const std::string &varName, const bool result);
@@ -255,7 +251,6 @@ class CPPCHECKLIB CheckOther : public Check {
255251
void redundantPointerOpError(const Token* tok, const std::string& varname, bool inconclusive);
256252
void raceAfterInterlockedDecrementError(const Token* tok);
257253
void unusedLabelError(const Token* tok);
258-
void invalidTestForOverflow(const Token* tok);
259254

260255
void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
261256
CheckOther c(0, settings, errorLogger);
@@ -310,7 +305,6 @@ class CPPCHECKLIB CheckOther : public Check {
310305
c.commaSeparatedReturnError(0);
311306
c.redundantPointerOpError(0, "varname", false);
312307
c.unusedLabelError(0);
313-
c.invalidTestForOverflow(0);
314308
}
315309

316310
static std::string myName() {
@@ -333,7 +327,6 @@ class CPPCHECKLIB CheckOther : public Check {
333327
// warning
334328
"- either division by zero or useless condition\n"
335329
"- memset() with a value out of range as the 2nd parameter\n"
336-
"- invalid test for overflow\n"
337330

338331
// performance
339332
"- redundant data copying for const variable\n"

‎test/testcondition.cpp‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,8 @@ class TestCondition : public TestFixture {
7979
TEST_CASE(clarifyCondition6); // #3818
8080

8181
TEST_CASE(alwaysTrue);
82+
83+
TEST_CASE(checkInvalidTestForOverflow);
8284
}
8385

8486
void check(const char code[], const char* filename = "test.cpp") {
@@ -1604,6 +1606,23 @@ class TestCondition : public TestFixture {
16041606
"}");
16051607
ASSERT_EQUALS("", errout.str());
16061608
}
1609+
1610+
void checkInvalidTestForOverflow() {
1611+
check("void f(char *p, unsigned int x) {\n"
1612+
" assert((p + x) < p);\n"
1613+
"}");
1614+
ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow '(p+x)<p'. Condition is always false unless there is overflow, and overflow is UB.\n", errout.str());
1615+
1616+
check("void f(signed int x) {\n"
1617+
" assert(x + 100 < x);\n"
1618+
"}");
1619+
ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow 'x+100<x'. Condition is always false unless there is overflow, and overflow is UB.\n", errout.str());
1620+
1621+
check("void f(signed int x) {\n" // unsigned overflow => dont warn
1622+
" assert(x + 100U < x);\n"
1623+
"}");
1624+
ASSERT_EQUALS("", errout.str());
1625+
}
16071626
};
16081627

16091628
REGISTER_TEST(TestCondition)

‎test/testother.cpp‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -157,8 +157,6 @@ class TestOther : public TestFixture {
157157
TEST_CASE(raceAfterInterlockedDecrement);
158158

159159
TEST_CASE(testUnusedLabel);
160-
161-
TEST_CASE(checkInvalidTestForOverflow);
162160
}
163161

164162
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 @@ class TestOther : public TestFixture {
60196017
"}");
60206018
ASSERT_EQUALS("", errout.str());
60216019
}
6022-
6023-
void checkInvalidTestForOverflow() {
6024-
check("void f(char *p, unsigned int x) {\n"
6025-
" assert((p + x) < p);\n"
6026-
"}");
6027-
ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow '(p+x)<p'. Condition is always false unless there is overflow, and overflow is UB.\n", errout.str());
6028-
6029-
check("void f(signed int x) {\n"
6030-
" assert(x + 100 < x);\n"
6031-
"}");
6032-
ASSERT_EQUALS("[test.cpp:2]: (warning) Invalid test for overflow 'x+100<x'. Condition is always false unless there is overflow, and overflow is UB.\n", errout.str());
6033-
6034-
check("void f(signed int x) {\n" // unsigned overflow => dont warn
6035-
" assert(x + 100U < x);\n"
6036-
"}");
6037-
ASSERT_EQUALS("", errout.str());
6038-
}
60396020
};
60406021

60416022
REGISTER_TEST(TestOther)

0 commit comments

Comments
 (0)