Skip to content

Commit 97744da

Browse files
pfultz2pfultz2
committed
Fix 14956: FP arrayIndexOutOfBounds with break in loop
1 parent cb9f2a2 commit 97744da

3 files changed

Lines changed: 132 additions & 18 deletions

File tree

‎lib/valueflow.cpp‎

Lines changed: 43 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4354,6 +4354,20 @@ static bool isBreakScope(const Token* const endToken)
43544354
return Token::findmatch(endToken->link(), "break|goto", endToken);
43554355
}
43564356

4357+
// If this is the body of a loop, the loop always exits through an unconditional break:
4358+
// the last statement is a top-level break and no continue (or goto) can jump back to
4359+
// evaluate the loop condition again
4360+
static bool isUnconditionalBreakScope(const Token* const endToken)
4361+
{
4362+
if (!Token::simpleMatch(endToken, "}"))
4363+
return false;
4364+
if (!Token::simpleMatch(endToken->link(), "{"))
4365+
return false;
4366+
if (!Token::Match(endToken->tokAt(-2), "break ;") || !Token::Match(endToken->tokAt(-3), "{|}|;"))
4367+
return false;
4368+
return !Token::findmatch(endToken->link(), "continue|goto", endToken);
4369+
}
4370+
43574371
ValueFlow::Value ValueFlow::asImpossible(ValueFlow::Value v)
43584372
{
43594373
v.invertRange();
@@ -4901,10 +4915,12 @@ struct ConditionHandler {
49014915
Token* after = top->link()->linkAt(1);
49024916
bool dead_if = deadBranch[0];
49034917
bool dead_else = deadBranch[1];
4918+
bool alwaysBreaks = false;
49044919
const Token* unknownFunction = nullptr;
4905-
if (condTok->astParent() && Token::Match(top->previous(), "while|for ("))
4920+
if (condTok->astParent() && Token::Match(top->previous(), "while|for (")) {
49064921
dead_if = !isBreakScope(after);
4907-
else if (!dead_if)
4922+
alwaysBreaks = isUnconditionalBreakScope(after);
4923+
} else if (!dead_if)
49084924
dead_if = isReturnScope(after, settings.library, &unknownFunction);
49094925

49104926
// If the taken branch might not return (it ends in a call to an unknown,
@@ -4943,12 +4959,15 @@ struct ConditionHandler {
49434959
[](const ValueFlow::Value& v) {
49444960
return v.isPossible() || v.isInconclusive();
49454961
});
4946-
std::copy_if(elseValues.cbegin(),
4947-
elseValues.cend(),
4948-
std::back_inserter(values),
4949-
[](const ValueFlow::Value& v) {
4950-
return v.isPossible() || v.isInconclusive();
4951-
});
4962+
// if the loop body always breaks then the loop condition is evaluated at most once,
4963+
// so the false-condition values from iterating the loop do not apply after the loop
4964+
if (!alwaysBreaks)
4965+
std::copy_if(elseValues.cbegin(),
4966+
elseValues.cend(),
4967+
std::back_inserter(values),
4968+
[](const ValueFlow::Value& v) {
4969+
return v.isPossible() || v.isInconclusive();
4970+
});
49524971
}
49534972

49544973
if (values.empty())
@@ -5453,8 +5472,11 @@ static void valueFlowForLoop(const TokenList &tokenlist, const SymbolDatabase& s
54535472
valueFlowForward(bodyStart, bodyStart->link(), vartok, std::move(lastValues), tokenlist, errorLogger, settings);
54545473
}
54555474
}
5456-
const MathLib::bigint afterValue = executeBody ? lastValue + stepValue : initValue;
5457-
valueFlowForLoopSimplifyAfter(tok, varid, afterValue, tokenlist, errorLogger, settings);
5475+
// if the body always exits through a break the counter does not reach its final value
5476+
if (!executeBody || !isUnconditionalBreakScope(bodyStart->link())) {
5477+
const MathLib::bigint afterValue = executeBody ? lastValue + stepValue : initValue;
5478+
valueFlowForLoopSimplifyAfter(tok, varid, afterValue, tokenlist, errorLogger, settings);
5479+
}
54585480
} else {
54595481
ProgramMemory mem1, mem2, memAfter;
54605482
if (valueFlowForLoop2(tok, mem1, mem2, memAfter, settings)) {
@@ -5485,14 +5507,17 @@ static void valueFlowForLoop(const TokenList &tokenlist, const SymbolDatabase& s
54855507
valueFlowForLoopSimplify(bodyStart, p.first.tok, false, p.second.intvalue, tokenlist, errorLogger, settings);
54865508
}
54875509
}
5488-
for (const auto& p : memAfter) {
5489-
if (!p.second.isIntValue())
5490-
continue;
5491-
if (p.second.isImpossible())
5492-
continue;
5493-
if (p.first.tok->varId() == 0)
5494-
continue;
5495-
valueFlowForLoopSimplifyAfter(tok, p.first.getExpressionId(), p.second.intvalue, tokenlist, errorLogger, settings);
5510+
// if the body always exits through a break the counters do not reach their final values
5511+
if (!isUnconditionalBreakScope(bodyStart->link())) {
5512+
for (const auto& p : memAfter) {
5513+
if (!p.second.isIntValue())
5514+
continue;
5515+
if (p.second.isImpossible())
5516+
continue;
5517+
if (p.first.tok->varId() == 0)
5518+
continue;
5519+
valueFlowForLoopSimplifyAfter(tok, p.first.getExpressionId(), p.second.intvalue, tokenlist, errorLogger, settings);
5520+
}
54965521
}
54975522
}
54985523
}

‎test/testbufferoverrun.cpp‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -164,6 +164,7 @@ class TestBufferOverrun : public TestFixture {
164164
TEST_CASE(array_index_74); // #11088
165165
TEST_CASE(array_index_75);
166166
TEST_CASE(array_index_76);
167+
TEST_CASE(array_index_77); // loop that always exits through a break
167168
TEST_CASE(array_index_multidim);
168169
TEST_CASE(array_index_switch_in_for);
169170
TEST_CASE(array_index_for_in_for); // FP: #2634
@@ -2011,6 +2012,64 @@ class TestBufferOverrun : public TestFixture {
20112012
errout_str());
20122013
}
20132014

2015+
// loop that always exits through a break -> the counter does not reach its final value
2016+
void array_index_77()
2017+
{
2018+
check("void f() {\n"
2019+
" int idx;\n"
2020+
" int arr[3];\n"
2021+
" for (idx = 0; idx < 3; idx++) {\n"
2022+
" break;\n"
2023+
" }\n"
2024+
" arr[idx] = 0;\n"
2025+
"}\n");
2026+
ASSERT_EQUALS("", errout_str());
2027+
2028+
check("void f() {\n" // multiple counters -> handled by valueFlowForLoop2
2029+
" int i, j;\n"
2030+
" int arr[3];\n"
2031+
" for (i = 0, j = 0; i < 3; i++, j++) {\n"
2032+
" break;\n"
2033+
" }\n"
2034+
" arr[i] = 0;\n"
2035+
"}\n");
2036+
ASSERT_EQUALS("", errout_str());
2037+
2038+
check("void f() {\n"
2039+
" int idx = 0;\n"
2040+
" int arr[3];\n"
2041+
" while (idx < 3) {\n"
2042+
" idx++;\n"
2043+
" break;\n"
2044+
" }\n"
2045+
" arr[idx] = 0;\n"
2046+
"}\n");
2047+
ASSERT_EQUALS("", errout_str());
2048+
2049+
check("void f(bool c) {\n" // conditional break -> the loop can run to completion
2050+
" int idx;\n"
2051+
" int arr[3];\n"
2052+
" for (idx = 0; idx < 3; idx++) {\n"
2053+
" if (c)\n"
2054+
" break;\n"
2055+
" }\n"
2056+
" arr[idx] = 0;\n"
2057+
"}\n");
2058+
ASSERT_EQUALS("[test.cpp:8:8]: (error) Array 'arr[3]' accessed at index 3, which is out of bounds. [arrayIndexOutOfBounds]\n", errout_str());
2059+
2060+
check("void f(bool c) {\n" // continue -> the loop condition can be evaluated again
2061+
" int idx;\n"
2062+
" int arr[3];\n"
2063+
" for (idx = 0; idx < 3; idx++) {\n"
2064+
" if (c)\n"
2065+
" continue;\n"
2066+
" break;\n"
2067+
" }\n"
2068+
" arr[idx] = 0;\n"
2069+
"}\n");
2070+
ASSERT_EQUALS("[test.cpp:9:8]: (error) Array 'arr[3]' accessed at index 3, which is out of bounds. [arrayIndexOutOfBounds]\n", errout_str());
2071+
}
2072+
20142073
void array_index_multidim() {
20152074
check("void f()\n"
20162075
"{\n"

‎test/testvalueflow.cpp‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5133,6 +5133,36 @@ class TestValueFlow : public TestFixture {
51335133
++it;
51345134
ASSERT_EQUALS(5, it->intvalue);
51355135
ASSERT(it->isImpossible());
5136+
5137+
code = "void f() {\n" // the loop always exits through the break
5138+
" int x;\n"
5139+
" for (x = 0; x < 3; x++) {\n"
5140+
" break;\n"
5141+
" }\n"
5142+
" a[x] = 0;\n" // <- x is not 3
5143+
"}";
5144+
ASSERT_EQUALS(false, testValueOfX(code, 6U, 3));
5145+
5146+
code = "void f(bool c) {\n" // conditional break -> the loop can run to completion
5147+
" int x;\n"
5148+
" for (x = 0; x < 3; x++) {\n"
5149+
" if (c)\n"
5150+
" break;\n"
5151+
" }\n"
5152+
" a[x] = 0;\n"
5153+
"}";
5154+
ASSERT_EQUALS(true, testValueOfX(code, 7U, 3));
5155+
5156+
code = "void f(bool c) {\n" // continue -> the loop condition can be evaluated again
5157+
" int x;\n"
5158+
" for (x = 0; x < 3; x++) {\n"
5159+
" if (c)\n"
5160+
" continue;\n"
5161+
" break;\n"
5162+
" }\n"
5163+
" a[x] = 0;\n"
5164+
"}";
5165+
ASSERT_EQUALS(true, testValueOfX(code, 8U, 3));
51365166
}
51375167

51385168
void valueFlowSubFunction() {

0 commit comments

Comments
 (0)