Skip to content

Fix 14956: FP arrayIndexOutOfBounds with break in loop #8772

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pfultz2 wants to merge 3 commits into
base: main
Choose a base branch
from
+141 −18
Open

Fix 14956: FP arrayIndexOutOfBounds with break in loop #8772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
97744da
Fix 14956: FP arrayIndexOutOfBounds with break in loop
pfultz2 Aug 2, 2026
9df7881
Format
pfultz2 Aug 2, 2026
caabbba
Use simpleMatch
pfultz2 Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 48 additions & 18 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4354,6 +4354,20 @@ static bool isBreakScope(const Token* const endToken)
return Token::findmatch(endToken->link(), "break|goto", endToken);
}

// If this is the body of a loop, the loop always exits through an unconditional break:
// the last statement is a top-level break and no continue (or goto) can jump back to
// evaluate the loop condition again
static bool isUnconditionalBreakScope(const Token* const endToken)
{
if (!Token::simpleMatch(endToken, "}"))
return false;
if (!Token::simpleMatch(endToken->link(), "{"))
return false;
if (!Token::simpleMatch(endToken->tokAt(-2), "break ;") || !Token::Match(endToken->tokAt(-3), "{|}|;"))
return false;
return !Token::findmatch(endToken->link(), "continue|goto", endToken);
}

ValueFlow::Value ValueFlow::asImpossible(ValueFlow::Value v)
{
v.invertRange();
Expand Down Expand Up @@ -4901,10 +4915,12 @@ struct ConditionHandler {
Token* after = top->link()->linkAt(1);
bool dead_if = deadBranch[0];
bool dead_else = deadBranch[1];
bool alwaysBreaks = false;
const Token* unknownFunction = nullptr;
if (condTok->astParent() && Token::Match(top->previous(), "while|for ("))
if (condTok->astParent() && Token::Match(top->previous(), "while|for (")) {
dead_if = !isBreakScope(after);
else if (!dead_if)
alwaysBreaks = isUnconditionalBreakScope(after);
} else if (!dead_if)
dead_if = isReturnScope(after, settings.library, &unknownFunction);

// If the taken branch might not return (it ends in a call to an unknown,
Expand Down Expand Up @@ -4943,12 +4959,15 @@ struct ConditionHandler {
[](const ValueFlow::Value& v) {
return v.isPossible() || v.isInconclusive();
});
std::copy_if(elseValues.cbegin(),
elseValues.cend(),
std::back_inserter(values),
[](const ValueFlow::Value& v) {
return v.isPossible() || v.isInconclusive();
});
// if the loop body always breaks then the loop condition is evaluated at most once,
// so the false-condition values from iterating the loop do not apply after the loop
if (!alwaysBreaks)
std::copy_if(elseValues.cbegin(),
elseValues.cend(),
std::back_inserter(values),
[](const ValueFlow::Value& v) {
return v.isPossible() || v.isInconclusive();
});
}

if (values.empty())
Expand Down Expand Up @@ -5453,8 +5472,11 @@ static void valueFlowForLoop(const TokenList &tokenlist, const SymbolDatabase& s
valueFlowForward(bodyStart, bodyStart->link(), vartok, std::move(lastValues), tokenlist, errorLogger, settings);
}
}
const MathLib::bigint afterValue = executeBody ? lastValue + stepValue : initValue;
valueFlowForLoopSimplifyAfter(tok, varid, afterValue, tokenlist, errorLogger, settings);
// if the body always exits through a break the counter does not reach its final value
if (!executeBody || !isUnconditionalBreakScope(bodyStart->link())) {
const MathLib::bigint afterValue = executeBody ? lastValue + stepValue : initValue;
valueFlowForLoopSimplifyAfter(tok, varid, afterValue, tokenlist, errorLogger, settings);
}
} else {
ProgramMemory mem1, mem2, memAfter;
if (valueFlowForLoop2(tok, mem1, mem2, memAfter, settings)) {
Expand Down Expand Up @@ -5485,14 +5507,22 @@ static void valueFlowForLoop(const TokenList &tokenlist, const SymbolDatabase& s
valueFlowForLoopSimplify(bodyStart, p.first.tok, false, p.second.intvalue, tokenlist, errorLogger, settings);
}
}
for (const auto& p : memAfter) {
if (!p.second.isIntValue())
continue;
if (p.second.isImpossible())
continue;
if (p.first.tok->varId() == 0)
continue;
valueFlowForLoopSimplifyAfter(tok, p.first.getExpressionId(), p.second.intvalue, tokenlist, errorLogger, settings);
// if the body always exits through a break the counters do not reach their final values
if (!isUnconditionalBreakScope(bodyStart->link())) {
for (const auto& p : memAfter) {
if (!p.second.isIntValue())
continue;
if (p.second.isImpossible())
continue;
if (p.first.tok->varId() == 0)
continue;
valueFlowForLoopSimplifyAfter(tok,
p.first.getExpressionId(),
p.second.intvalue,
tokenlist,
errorLogger,
settings);
}
}
}
}
Expand Down
63 changes: 63 additions & 0 deletions test/testbufferoverrun.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ class TestBufferOverrun : public TestFixture {
TEST_CASE(array_index_74); // #11088
TEST_CASE(array_index_75);
TEST_CASE(array_index_76);
TEST_CASE(array_index_77); // loop that always exits through a break
TEST_CASE(array_index_multidim);
TEST_CASE(array_index_switch_in_for);
TEST_CASE(array_index_for_in_for); // FP: #2634
Expand Down Expand Up @@ -2011,6 +2012,68 @@ class TestBufferOverrun : public TestFixture {
errout_str());
}

// loop that always exits through a break -> the counter does not reach its final value
void array_index_77()
{
check("void f() {\n"
" int idx;\n"
" int arr[3];\n"
" for (idx = 0; idx < 3; idx++) {\n"
" break;\n"
" }\n"
" arr[idx] = 0;\n"
"}\n");
ASSERT_EQUALS("", errout_str());

check("void f() {\n" // multiple counters -> handled by valueFlowForLoop2
" int i, j;\n"
" int arr[3];\n"
" for (i = 0, j = 0; i < 3; i++, j++) {\n"
" break;\n"
" }\n"
" arr[i] = 0;\n"
"}\n");
ASSERT_EQUALS("", errout_str());

check("void f() {\n"
" int idx = 0;\n"
" int arr[3];\n"
" while (idx < 3) {\n"
" idx++;\n"
" break;\n"
" }\n"
" arr[idx] = 0;\n"
"}\n");
ASSERT_EQUALS("", errout_str());

check("void f(bool c) {\n" // conditional break -> the loop can run to completion
" int idx;\n"
" int arr[3];\n"
" for (idx = 0; idx < 3; idx++) {\n"
" if (c)\n"
" break;\n"
" }\n"
" arr[idx] = 0;\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:8:8]: (error) Array 'arr[3]' accessed at index 3, which is out of bounds. [arrayIndexOutOfBounds]\n",
errout_str());

check("void f(bool c) {\n" // continue -> the loop condition can be evaluated again
" int idx;\n"
" int arr[3];\n"
" for (idx = 0; idx < 3; idx++) {\n"
" if (c)\n"
" continue;\n"
" break;\n"
" }\n"
" arr[idx] = 0;\n"
"}\n");
ASSERT_EQUALS(
"[test.cpp:9:8]: (error) Array 'arr[3]' accessed at index 3, which is out of bounds. [arrayIndexOutOfBounds]\n",

@aadanen aadanen Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! I am new so sorry if this is a silly question.

I thought that if we want to return an error, we must be guaranteed that it is actually an error. In this input it looks like the array access is only out of bounds if c is true. Since we don't know anything about c, I thought we couldn't report any errors. I would expect the correct output for this testcase to be no errors.

errout_str());
}

void array_index_multidim() {
check("void f()\n"
"{\n"
Expand Down
30 changes: 30 additions & 0 deletions test/testvalueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5133,6 +5133,36 @@ class TestValueFlow : public TestFixture {
++it;
ASSERT_EQUALS(5, it->intvalue);
ASSERT(it->isImpossible());

code = "void f() {\n" // the loop always exits through the break
" int x;\n"
" for (x = 0; x < 3; x++) {\n"
" break;\n"
" }\n"
" a[x] = 0;\n" // <- x is not 3
"}";
ASSERT_EQUALS(false, testValueOfX(code, 6U, 3));

code = "void f(bool c) {\n" // conditional break -> the loop can run to completion
" int x;\n"
" for (x = 0; x < 3; x++) {\n"
" if (c)\n"
" break;\n"
" }\n"
" a[x] = 0;\n"
"}";
ASSERT_EQUALS(true, testValueOfX(code, 7U, 3));

code = "void f(bool c) {\n" // continue -> the loop condition can be evaluated again
" int x;\n"
" for (x = 0; x < 3; x++) {\n"
" if (c)\n"
" continue;\n"
" break;\n"
" }\n"
" a[x] = 0;\n"
"}";
ASSERT_EQUALS(true, testValueOfX(code, 8U, 3));
}

void valueFlowSubFunction() {
Expand Down
Loading