Skip to content

Commit 8301fa8

Browse files
pfultz2pfultz2
authored
Fix issue 8144: valueFlowBeforeCondition: struct (cppcheck-opensource#2645)
1 parent 90550d2 commit 8301fa8

9 files changed

Lines changed: 147 additions & 135 deletions

‎lib/astutils.cpp‎

Lines changed: 35 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1085,6 +1085,35 @@ bool isEscapeFunction(const Token* ftok, const Library* library)
10851085
return false;
10861086
}
10871087

1088+
static bool hasReturnFunction(const Token* tok, const Library* library, const Token** unknownFunc)
1089+
{
1090+
if (!tok)
1091+
return false;
1092+
const Token* ftok = tok->str() == "(" ? tok->previous() : nullptr;
1093+
while (Token::simpleMatch(ftok, "("))
1094+
ftok = ftok->astOperand1();
1095+
if (ftok) {
1096+
const Function * function = ftok->function();
1097+
if (function) {
1098+
if (function->isEscapeFunction())
1099+
return true;
1100+
if (function->isAttributeNoreturn())
1101+
return true;
1102+
} else if (library && library->isnoreturn(ftok)) {
1103+
return true;
1104+
} else if (Token::Match(ftok, "exit|abort")) {
1105+
return true;
1106+
}
1107+
if (unknownFunc && !function && library && library->functions.count(library->getFunctionName(ftok)) == 0)
1108+
*unknownFunc = ftok;
1109+
return false;
1110+
} else if (tok->isConstOp()) {
1111+
return hasReturnFunction(tok->astOperand1(), library, unknownFunc) || hasReturnFunction(tok->astOperand2(), library, unknownFunc);
1112+
}
1113+
1114+
return false;
1115+
}
1116+
10881117
bool isReturnScope(const Token* const endToken, const Library* library, const Token** unknownFunc, bool functionScope)
10891118
{
10901119
if (!endToken || endToken->str() != "}")
@@ -1110,21 +1139,12 @@ bool isReturnScope(const Token* const endToken, const Library* library, const To
11101139
if (Token::Match(prev->link()->previous(), "[;{}] {"))
11111140
return isReturnScope(prev, library, unknownFunc, functionScope);
11121141
} else if (Token::simpleMatch(prev, ";")) {
1113-
if (Token::simpleMatch(prev->previous(), ") ;") && Token::Match(prev->linkAt(-1)->tokAt(-2), "[;{}] %name% (")) {
1114-
const Token * ftok = prev->linkAt(-1)->previous();
1115-
const Function * function = ftok->function();
1116-
if (function) {
1117-
if (function->isEscapeFunction())
1118-
return true;
1119-
if (function->isAttributeNoreturn())
1120-
return true;
1121-
} else if (library && library->isnoreturn(ftok)) {
1122-
return true;
1123-
} else if (Token::Match(ftok, "exit|abort")) {
1124-
return true;
1125-
}
1126-
if (unknownFunc && !function && library && library->functions.count(library->getFunctionName(ftok)) == 0)
1127-
*unknownFunc = ftok;
1142+
if (prev->tokAt(-2) && hasReturnFunction(prev->tokAt(-2)->astTop(), library, unknownFunc))
1143+
return true;
1144+
// Unknown symbol
1145+
if (Token::Match(prev->tokAt(-2), ";|}|{ %name% ;") && prev->previous()->isIncompleteVar()) {
1146+
if (unknownFunc)
1147+
*unknownFunc = prev->previous();
11281148
return false;
11291149
}
11301150
if (Token::simpleMatch(prev->previous(), ") ;") && prev->previous()->link() &&

‎lib/checknullpointer.cpp‎

Lines changed: 7 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,13 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown, const Set
195195

196196
// read/write member variable
197197
if (firstOperand && parent->originalName() == "->" && (!parent->astParent() || parent->astParent()->str() != "&")) {
198-
if (!parent->astParent() || parent->astParent()->str() != "(" || parent->astParent() == tok->previous())
198+
if (!parent->astParent())
199+
return true;
200+
if (!Token::Match(parent->astParent()->previous(), "if|while|for|switch ("))
201+
return true;
202+
if (!Token::Match(parent->astParent()->previous(), "%name% ("))
203+
return true;
204+
if (parent->astParent() == tok->previous())
199205
return true;
200206
unknown = true;
201207
return false;
@@ -251,73 +257,6 @@ bool CheckNullPointer::isPointerDeRef(const Token *tok, bool &unknown, const Set
251257
}
252258

253259

254-
void CheckNullPointer::nullPointerLinkedList()
255-
{
256-
257-
if (!mSettings->isEnabled(Settings::WARNING))
258-
return;
259-
260-
const SymbolDatabase* const symbolDatabase = mTokenizer->getSymbolDatabase();
261-
262-
// looping through items in a linked list in a inner loop.
263-
// Here is an example:
264-
// for (const Token *tok = tokens; tok; tok = tok->next) {
265-
// if (tok->str() == "hello")
266-
// tok = tok->next; // <- tok might become a null pointer!
267-
// }
268-
for (const Scope &forScope : symbolDatabase->scopeList) {
269-
const Token* const tok1 = forScope.classDef;
270-
// search for a "for" scope..
271-
if (forScope.type != Scope::eFor || !tok1)
272-
continue;
273-
274-
// is there any dereferencing occurring in the for statement
275-
const Token* end2 = tok1->linkAt(1);
276-
for (const Token *tok2 = tok1->tokAt(2); tok2 != end2; tok2 = tok2->next()) {
277-
// Dereferencing a variable inside the "for" parentheses..
278-
if (Token::Match(tok2, "%var% . %name%")) {
279-
// Is this variable a pointer?
280-
const Variable *var = tok2->variable();
281-
if (!var || !var->isPointer())
282-
continue;
283-
284-
// Variable id for dereferenced variable
285-
const unsigned int varid(tok2->varId());
286-
287-
if (Token::Match(tok2->tokAt(-2), "%varid% ?", varid))
288-
continue;
289-
290-
// Check usage of dereferenced variable in the loop..
291-
// TODO: Move this to ValueFlow
292-
for (const Scope *innerScope : forScope.nestedList) {
293-
if (innerScope->type != Scope::eWhile)
294-
continue;
295-
296-
// TODO: are there false negatives for "while ( %varid% ||"
297-
if (Token::Match(innerScope->classDef->next(), "( %varid% &&|)", varid)) {
298-
// Make sure there is a "break" or "return" inside the loop.
299-
// Without the "break" a null pointer could be dereferenced in the
300-
// for statement.
301-
for (const Token *tok4 = innerScope->bodyStart; tok4; tok4 = tok4->next()) {
302-
if (tok4 == forScope.bodyEnd) {
303-
const ValueFlow::Value v(innerScope->classDef, 0LL);
304-
nullPointerError(tok1, var->name(), &v, false);
305-
break;
306-
}
307-
308-
// There is a "break" or "return" inside the loop.
309-
// TODO: there can be false negatives. There could still be
310-
// execution paths that are not properly terminated
311-
else if (tok4->str() == "break" || tok4->str() == "return")
312-
break;
313-
}
314-
}
315-
}
316-
}
317-
}
318-
}
319-
}
320-
321260
static bool isNullablePointer(const Token* tok, const Settings* settings)
322261
{
323262
if (!tok)
@@ -374,7 +313,6 @@ void CheckNullPointer::nullPointerByDeRefAndChec()
374313

375314
void CheckNullPointer::nullPointer()
376315
{
377-
nullPointerLinkedList();
378316
nullPointerByDeRefAndChec();
379317
}
380318

‎lib/checknullpointer.h‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -137,12 +137,6 @@ class CPPCHECKLIB CheckNullPointer : public Check {
137137
"- undefined null pointer arithmetic\n";
138138
}
139139

140-
/**
141-
* @brief Does one part of the check for nullPointer().
142-
* looping through items in a linked list in a inner loop..
143-
*/
144-
void nullPointerLinkedList();
145-
146140
/**
147141
* @brief Does one part of the check for nullPointer().
148142
* Dereferencing a pointer and then checking if it's NULL..

‎lib/forwardanalyzer.cpp‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -407,8 +407,12 @@ struct ForwardTraversal {
407407
} else {
408408
if (updateTok(tok, &next) == Progress::Break)
409409
return Progress::Break;
410-
if (next)
411-
tok = next;
410+
if (next) {
411+
if (precedes(next, end))
412+
tok = next->previous();
413+
else
414+
return Progress::Break;
415+
}
412416
}
413417
// Prevent infinite recursion
414418
if (tok->next() == start)

‎lib/templatesimplifier.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -147,10 +147,10 @@ TemplateSimplifier::TokenAndName::TokenAndName(Token *token, const std::string &
147147
if ((isFunction() || isClass()) && mNameToken->strAt(-1) == "::") {
148148
const Token * start = mNameToken;
149149

150-
while (Token::Match(start->tokAt(-2), "%name% ::") ||
151-
(Token::simpleMatch(start->tokAt(-2), "> ::") &&
152-
start->tokAt(-2)->findOpeningBracket() &&
153-
Token::Match(start->tokAt(-2)->findOpeningBracket()->previous(), "%name% <"))) {
150+
while (start && (Token::Match(start->tokAt(-2), "%name% ::") ||
151+
(Token::simpleMatch(start->tokAt(-2), "> ::") &&
152+
start->tokAt(-2)->findOpeningBracket() &&
153+
Token::Match(start->tokAt(-2)->findOpeningBracket()->previous(), "%name% <")))) {
154154
if (start->strAt(-2) == ">")
155155
start = start->tokAt(-2)->findOpeningBracket()->previous();
156156
else

‎lib/tokenize.cpp‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1686,6 +1686,8 @@ namespace {
16861686

16871687
void setScopeInfo(Token *tok, std::list<ScopeInfo3> *scopeInfo, bool all = false)
16881688
{
1689+
if (!tok)
1690+
return;
16891691
while (tok->str() == "}" && !scopeInfo->empty() && tok == scopeInfo->back().bodyEnd)
16901692
scopeInfo->pop_back();
16911693
if (!Token::Match(tok, "namespace|class|struct|union %name% {|:|::")) {
@@ -5902,7 +5904,7 @@ bool Tokenizer::simplifyConditions()
59025904
Token::simpleMatch(tok, "|| true ||")) {
59035905
//goto '('
59045906
Token *tok2 = tok;
5905-
while (tok2->previous()) {
5907+
while (tok2 && tok2->previous()) {
59065908
if (tok2->previous()->str() == ")")
59075909
tok2 = tok2->previous()->link();
59085910
else {
@@ -6466,7 +6468,7 @@ void Tokenizer::simplifyFunctionPointers()
64666468
!(Token::Match(tok2, "%name% (") && Token::simpleMatch(tok2->linkAt(1), ") ) (")))
64676469
continue;
64686470

6469-
while (tok->str() != "(")
6471+
while (tok && tok->str() != "(")
64706472
tok = tok->next();
64716473

64726474
// check that the declaration ends

‎lib/valueflow.cpp‎

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,7 @@
102102
#include <map>
103103
#include <set>
104104
#include <stack>
105+
#include <tuple>
105106
#include <vector>
106107

107108
static void bailoutInternal(TokenList *tokenlist, ErrorLogger *errorLogger, const Token *tok, const std::string &what, const std::string &file, int line, const std::string &function)
@@ -1838,6 +1839,29 @@ static void valueFlowReverse(TokenList *tokenlist,
18381839
}
18391840

18401841
if (tok2->str() == "}") {
1842+
const Token* condTok = getCondTokFromEnd(tok2);
1843+
// Evaluate condition of for and while loops first
1844+
if (condTok && condTok->astTop() && Token::Match(condTok->astTop()->previous(), "for|while (")) {
1845+
const Token* startTok = nullptr;
1846+
const Token* endTok = nullptr;
1847+
std::tie(startTok, endTok) = condTok->findExpressionStartEndTokens();
1848+
if (!isVariableChanged(startTok, endTok, varid, false, settings, true)) {
1849+
std::list<ValueFlow::Value> values = {val};
1850+
if (val2.condition) {
1851+
values.push_back(val2);
1852+
}
1853+
const Token *expr = Token::findmatch(tok2, "%varid%", varid);
1854+
valueFlowForward(const_cast<Token*>(startTok),
1855+
endTok,
1856+
expr,
1857+
values,
1858+
false,
1859+
false,
1860+
tokenlist,
1861+
errorLogger,
1862+
settings);
1863+
}
1864+
}
18411865
const Token *vartok = Token::findmatch(tok2->link(), "%varid%", tok2, varid);
18421866
while (Token::Match(vartok, "%name% = %num% ;") && !vartok->tokAt(2)->getValue(num))
18431867
vartok = Token::findmatch(vartok->next(), "%varid%", tok2, varid);
@@ -4089,26 +4113,27 @@ struct ValueFlowConditionHandler {
40894113
// After conditional code..
40904114
if (Token::simpleMatch(top->link(), ") {")) {
40914115
Token *after = top->link()->linkAt(1);
4092-
std::string unknownFunction;
4093-
if (settings->library.isScopeNoReturn(after, &unknownFunction)) {
4094-
if (settings->debugwarnings && !unknownFunction.empty())
4095-
bailout(tokenlist, errorLogger, after, "possible noreturn scope");
4096-
continue;
4097-
}
4098-
4099-
bool dead_if = isReturnScope(after, &settings->library) ||
4116+
const Token* unknownFunction = nullptr;
4117+
bool dead_if = isReturnScope(after, &settings->library, &unknownFunction) ||
41004118
(tok->astParent() && Token::simpleMatch(tok->astParent()->previous(), "while (") &&
41014119
!isBreakScope(after));
41024120
bool dead_else = false;
41034121

4122+
if (!dead_if && unknownFunction) {
4123+
if (settings->debugwarnings)
4124+
bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope");
4125+
continue;
4126+
}
4127+
41044128
if (Token::simpleMatch(after, "} else {")) {
41054129
after = after->linkAt(2);
4106-
if (Token::simpleMatch(after->tokAt(-2), ") ; }")) {
4130+
unknownFunction = nullptr;
4131+
dead_else = isReturnScope(after, &settings->library, &unknownFunction);
4132+
if (!dead_else && unknownFunction) {
41074133
if (settings->debugwarnings)
4108-
bailout(tokenlist, errorLogger, after, "possible noreturn scope");
4134+
bailout(tokenlist, errorLogger, unknownFunction, "possible noreturn scope");
41094135
continue;
41104136
}
4111-
dead_else = isReturnScope(after, &settings->library);
41124137
}
41134138

41144139
if (dead_if && dead_else)

0 commit comments

Comments
 (0)