From 97744da13ce5097a3b52a1ae9b3e541ad91db2e1 Mon Sep 17 00:00:00 2001 From: Paul Date: Sun, 2 Aug 2026 17:23:17 -0500 Subject: [PATCH 1/3] Fix 14956: FP arrayIndexOutOfBounds with break in loop --- lib/valueflow.cpp | 61 +++++++++++++++++++++++++++----------- test/testbufferoverrun.cpp | 59 ++++++++++++++++++++++++++++++++++++ test/testvalueflow.cpp | 30 +++++++++++++++++++ 3 files changed, 132 insertions(+), 18 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 63543e1a8f5..3c17d33df85 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -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::Match(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(); @@ -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, @@ -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()) @@ -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)) { @@ -5485,14 +5507,17 @@ 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); + } } } } diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 37ab7083902..872fe76c090 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -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 @@ -2011,6 +2012,64 @@ 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", errout_str()); + } + void array_index_multidim() { check("void f()\n" "{\n" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index c98680b8180..c9fc3697ad5 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -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() { From 9df78811b65905e05c4b50848f765ad5ff618ecf Mon Sep 17 00:00:00 2001 From: Paul Date: Sun, 2 Aug 2026 17:33:57 -0500 Subject: [PATCH 2/3] Format --- lib/valueflow.cpp | 7 ++++++- test/testbufferoverrun.cpp | 8 ++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 3c17d33df85..81459954cf4 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -5516,7 +5516,12 @@ static void valueFlowForLoop(const TokenList &tokenlist, const SymbolDatabase& s continue; if (p.first.tok->varId() == 0) continue; - valueFlowForLoopSimplifyAfter(tok, p.first.getExpressionId(), p.second.intvalue, tokenlist, errorLogger, settings); + valueFlowForLoopSimplifyAfter(tok, + p.first.getExpressionId(), + p.second.intvalue, + tokenlist, + errorLogger, + settings); } } } diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 872fe76c090..43a7b1812f4 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -2055,7 +2055,9 @@ class TestBufferOverrun : public TestFixture { " }\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()); + 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" @@ -2067,7 +2069,9 @@ class TestBufferOverrun : public TestFixture { " }\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", errout_str()); + ASSERT_EQUALS( + "[test.cpp:9:8]: (error) Array 'arr[3]' accessed at index 3, which is out of bounds. [arrayIndexOutOfBounds]\n", + errout_str()); } void array_index_multidim() { From caabbba75de77152cca6ec4c21caf78116ceca06 Mon Sep 17 00:00:00 2001 From: Paul Date: Mon, 3 Aug 2026 09:57:58 -0500 Subject: [PATCH 3/3] Use simpleMatch --- lib/valueflow.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 81459954cf4..33acc506168 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -4363,7 +4363,7 @@ static bool isUnconditionalBreakScope(const Token* const endToken) return false; if (!Token::simpleMatch(endToken->link(), "{")) return false; - if (!Token::Match(endToken->tokAt(-2), "break ;") || !Token::Match(endToken->tokAt(-3), "{|}|;")) + if (!Token::simpleMatch(endToken->tokAt(-2), "break ;") || !Token::Match(endToken->tokAt(-3), "{|}|;")) return false; return !Token::findmatch(endToken->link(), "continue|goto", endToken); }