Skip to content

Commit f7d0c25

Browse files
chrchr-githubchrchr-github
authored
Fix #13109 FP arrayIndexOutOfBounds with infinite loop and break in body (#6986)
1 parent 0cbf73c commit f7d0c25

5 files changed

Lines changed: 137 additions & 10 deletions

File tree

‎lib/astutils.cpp‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2833,6 +2833,41 @@ const Token* findExpression(const Token* start, const nonneg int exprid)
28332833
return nullptr;
28342834
}
28352835

2836+
const Token* findEscapeStatement(const Scope* scope, const Library* library)
2837+
{
2838+
if (!scope)
2839+
return nullptr;
2840+
for (const Token* tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) {
2841+
const Scope* escapeScope = tok->scope();
2842+
if (!escapeScope->isExecutable()) { // skip type definitions
2843+
tok = escapeScope->bodyEnd;
2844+
continue;
2845+
}
2846+
if (const Token* lambdaEnd = findLambdaEndToken(tok)) { // skip lambdas
2847+
tok = lambdaEnd;
2848+
continue;
2849+
}
2850+
if (!tok->isName())
2851+
continue;
2852+
if (isEscapeFunction(tok, library))
2853+
return tok;
2854+
if (!tok->isKeyword())
2855+
continue;
2856+
if (Token::Match(tok, "goto|return|throw")) // TODO: check try/catch, labels?
2857+
return tok;
2858+
if (!Token::Match(tok, "break|continue"))
2859+
continue;
2860+
const bool isBreak = tok->str()[0] == 'b';
2861+
while (escapeScope && escapeScope != scope) {
2862+
if (escapeScope->isLoopScope() || (isBreak && escapeScope->type == Scope::ScopeType::eSwitch))
2863+
return nullptr;
2864+
escapeScope = escapeScope->nestedIn;
2865+
}
2866+
return tok;
2867+
}
2868+
return nullptr;
2869+
}
2870+
28362871
// Thread-unsafe memoization
28372872
template<class F, class R=decltype(std::declval<F>()())>
28382873
static std::function<R()> memoize(F f)

‎lib/astutils.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,9 @@ const Token* findExpression(nonneg int exprid,
118118
const std::function<bool(const Token*)>& pred);
119119
const Token* findExpression(const Token* start, nonneg int exprid);
120120

121+
/** Does code execution escape from the given scope? */
122+
const Token* findEscapeStatement(const Scope* scope, const Library* library);
123+
121124
std::vector<const Token*> astFlatten(const Token* tok, const char* op);
122125
std::vector<Token*> astFlatten(Token* tok, const char* op);
123126

‎lib/valueflow.cpp‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5128,6 +5128,12 @@ static void valueFlowForLoopSimplify(Token* const bodyStart,
51285128
if (isVariableChanged(bodyStart, bodyEnd, expr->varId(), globalvar, settings))
51295129
return;
51305130

5131+
if (const Token* escape = findEscapeStatement(bodyStart->scope(), &settings.library)) {
5132+
if (settings.debugwarnings)
5133+
bailout(tokenlist, errorLogger, escape, "For loop variable bailout on escape statement");
5134+
return;
5135+
}
5136+
51315137
for (Token *tok2 = bodyStart->next(); tok2 != bodyEnd; tok2 = tok2->next()) {
51325138
if (tok2->varId() == expr->varId()) {
51335139
const Token * parent = tok2->astParent();
@@ -5189,20 +5195,10 @@ static void valueFlowForLoopSimplify(Token* const bodyStart,
51895195

51905196
if (Token::simpleMatch(tok2, ") {")) {
51915197
if (vartok->varId() && Token::findmatch(tok2->link(), "%varid%", tok2, vartok->varId())) {
5192-
if (Token::findmatch(tok2, "continue|break|return", tok2->linkAt(1), vartok->varId())) {
5193-
if (settings.debugwarnings)
5194-
bailout(tokenlist, errorLogger, tok2, "For loop variable bailout on conditional continue|break|return");
5195-
break;
5196-
}
51975198
if (settings.debugwarnings)
51985199
bailout(tokenlist, errorLogger, tok2, "For loop variable skipping conditional scope");
51995200
tok2 = tok2->linkAt(1);
52005201
if (Token::simpleMatch(tok2, "} else {")) {
5201-
if (Token::findmatch(tok2, "continue|break|return", tok2->linkAt(2), vartok->varId())) {
5202-
if (settings.debugwarnings)
5203-
bailout(tokenlist, errorLogger, tok2, "For loop variable bailout on conditional continue|break|return");
5204-
break;
5205-
}
52065202
tok2 = tok2->linkAt(2);
52075203
}
52085204
}

‎test/testbufferoverrun.cpp‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2579,6 +2579,19 @@ class TestBufferOverrun : public TestFixture {
25792579
" return buf[0];\n"
25802580
"}\n");
25812581
ASSERT_EQUALS("[test.cpp:5]: (error) Array 'buf[10]' accessed at index 10, which is out of bounds.\n", errout_str());
2582+
2583+
check("void f() {\n"
2584+
" const int a[10] = {};\n"
2585+
" for (int n = 0; 1; ++n) {\n"
2586+
" if (a[n] < 1) {\n"
2587+
" switch (a[n]) {\n"
2588+
" case 0:\n"
2589+
" break;\n"
2590+
" }\n"
2591+
" }\n"
2592+
" }\n"
2593+
"}\n");
2594+
ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[10]' accessed at index 9998, which is out of bounds.\n", errout_str());
25822595
}
25832596

25842597
void array_index_for_neq() {

‎test/testvalueflow.cpp‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4584,6 +4584,86 @@ class TestValueFlow : public TestFixture {
45844584
ASSERT_EQUALS(true, values.empty());
45854585
values = tokenValues(code, "[ f . b");
45864586
ASSERT_EQUALS(true, values.empty());
4587+
4588+
code = "void f() {\n" // #13109
4589+
" const int a[10] = {};\n"
4590+
" for (int n = 0; 1; ++n) {\n"
4591+
" (void)a[n];\n"
4592+
" break;\n"
4593+
" }\n"
4594+
"}\n";
4595+
values = tokenValues(code, "n ]");
4596+
ASSERT_EQUALS(2, values.size());
4597+
auto it = values.begin();
4598+
ASSERT_EQUALS(-1, it->intvalue);
4599+
ASSERT(it->isImpossible());
4600+
++it;
4601+
ASSERT_EQUALS(0, it->intvalue);
4602+
ASSERT(it->isPossible());
4603+
4604+
code = "void f() {\n"
4605+
" const int a[10] = {};\n"
4606+
" for (int n = 0; 1; ++n) {\n"
4607+
" if (a[n] < 1)\n"
4608+
" break;\n"
4609+
" }\n"
4610+
"}\n";
4611+
values = tokenValues(code, "n ]");
4612+
ASSERT_EQUALS(2, values.size());
4613+
it = values.begin();
4614+
ASSERT_EQUALS(-1, it->intvalue);
4615+
ASSERT(it->isImpossible());
4616+
++it;
4617+
ASSERT_EQUALS(0, it->intvalue);
4618+
ASSERT(it->isPossible());
4619+
4620+
code = "void f() {\n"
4621+
" const int a[10] = {};\n"
4622+
" for (int n = 0; 1; ++n) {\n"
4623+
" if (a[n] < 1)\n"
4624+
" throw 0;\n"
4625+
" }\n"
4626+
"}\n";
4627+
values = tokenValues(code, "n ]");
4628+
ASSERT_EQUALS(2, values.size());
4629+
it = values.begin();
4630+
ASSERT_EQUALS(-1, it->intvalue);
4631+
ASSERT(it->isImpossible());
4632+
++it;
4633+
ASSERT_EQUALS(0, it->intvalue);
4634+
ASSERT(it->isPossible());
4635+
4636+
code = "void f() {\n"
4637+
" const int a[10] = {};\n"
4638+
" for (int n = 0; 1; ++n) {\n"
4639+
" (void)a[n];\n"
4640+
" exit(1);\n"
4641+
" }\n"
4642+
"}\n";
4643+
values = tokenValues(code, "n ]");
4644+
ASSERT_EQUALS(2, values.size());
4645+
it = values.begin();
4646+
ASSERT_EQUALS(-1, it->intvalue);
4647+
ASSERT(it->isImpossible());
4648+
++it;
4649+
ASSERT_EQUALS(0, it->intvalue);
4650+
ASSERT(it->isPossible());
4651+
4652+
code = "void f() {\n"
4653+
" const int a[10] = {};\n"
4654+
" for (int n = 0; 1; ++n) {\n"
4655+
" if (a[n] < 1)\n"
4656+
" exit(1);\n"
4657+
" }\n"
4658+
"}\n";
4659+
values = tokenValues(code, "n ]");
4660+
ASSERT_EQUALS(2, values.size());
4661+
it = values.begin();
4662+
ASSERT_EQUALS(-1, it->intvalue);
4663+
ASSERT(it->isImpossible());
4664+
++it;
4665+
ASSERT_EQUALS(0, it->intvalue);
4666+
ASSERT(it->isPossible());
45874667
}
45884668

45894669
void valueFlowSubFunction() {

0 commit comments

Comments
 (0)