From f7d0c25fdd7f1b5115e0c37774d75c66e80cd4be Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Wed, 4 Dec 2024 10:59:00 +0100 Subject: [PATCH] Fix #13109 FP arrayIndexOutOfBounds with infinite loop and break in body (#6986) --- lib/astutils.cpp | 35 +++++++++++++++++ lib/astutils.h | 3 ++ lib/valueflow.cpp | 16 +++----- test/testbufferoverrun.cpp | 13 +++++++ test/testvalueflow.cpp | 80 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 137 insertions(+), 10 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index cc6a0e21748..469a3fbaecf 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -2833,6 +2833,41 @@ const Token* findExpression(const Token* start, const nonneg int exprid) return nullptr; } +const Token* findEscapeStatement(const Scope* scope, const Library* library) +{ + if (!scope) + return nullptr; + for (const Token* tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) { + const Scope* escapeScope = tok->scope(); + if (!escapeScope->isExecutable()) { // skip type definitions + tok = escapeScope->bodyEnd; + continue; + } + if (const Token* lambdaEnd = findLambdaEndToken(tok)) { // skip lambdas + tok = lambdaEnd; + continue; + } + if (!tok->isName()) + continue; + if (isEscapeFunction(tok, library)) + return tok; + if (!tok->isKeyword()) + continue; + if (Token::Match(tok, "goto|return|throw")) // TODO: check try/catch, labels? + return tok; + if (!Token::Match(tok, "break|continue")) + continue; + const bool isBreak = tok->str()[0] == 'b'; + while (escapeScope && escapeScope != scope) { + if (escapeScope->isLoopScope() || (isBreak && escapeScope->type == Scope::ScopeType::eSwitch)) + return nullptr; + escapeScope = escapeScope->nestedIn; + } + return tok; + } + return nullptr; +} + // Thread-unsafe memoization template()())> static std::function memoize(F f) diff --git a/lib/astutils.h b/lib/astutils.h index f5ffc67da1b..f0f50c11b27 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -118,6 +118,9 @@ const Token* findExpression(nonneg int exprid, const std::function& pred); const Token* findExpression(const Token* start, nonneg int exprid); +/** Does code execution escape from the given scope? */ +const Token* findEscapeStatement(const Scope* scope, const Library* library); + std::vector astFlatten(const Token* tok, const char* op); std::vector astFlatten(Token* tok, const char* op); diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index af495611b10..d522fb8e619 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -5128,6 +5128,12 @@ static void valueFlowForLoopSimplify(Token* const bodyStart, if (isVariableChanged(bodyStart, bodyEnd, expr->varId(), globalvar, settings)) return; + if (const Token* escape = findEscapeStatement(bodyStart->scope(), &settings.library)) { + if (settings.debugwarnings) + bailout(tokenlist, errorLogger, escape, "For loop variable bailout on escape statement"); + return; + } + for (Token *tok2 = bodyStart->next(); tok2 != bodyEnd; tok2 = tok2->next()) { if (tok2->varId() == expr->varId()) { const Token * parent = tok2->astParent(); @@ -5189,20 +5195,10 @@ static void valueFlowForLoopSimplify(Token* const bodyStart, if (Token::simpleMatch(tok2, ") {")) { if (vartok->varId() && Token::findmatch(tok2->link(), "%varid%", tok2, vartok->varId())) { - if (Token::findmatch(tok2, "continue|break|return", tok2->linkAt(1), vartok->varId())) { - if (settings.debugwarnings) - bailout(tokenlist, errorLogger, tok2, "For loop variable bailout on conditional continue|break|return"); - break; - } if (settings.debugwarnings) bailout(tokenlist, errorLogger, tok2, "For loop variable skipping conditional scope"); tok2 = tok2->linkAt(1); if (Token::simpleMatch(tok2, "} else {")) { - if (Token::findmatch(tok2, "continue|break|return", tok2->linkAt(2), vartok->varId())) { - if (settings.debugwarnings) - bailout(tokenlist, errorLogger, tok2, "For loop variable bailout on conditional continue|break|return"); - break; - } tok2 = tok2->linkAt(2); } } diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index ae3ead9bec3..6b02b07fc50 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -2579,6 +2579,19 @@ class TestBufferOverrun : public TestFixture { " return buf[0];\n" "}\n"); ASSERT_EQUALS("[test.cpp:5]: (error) Array 'buf[10]' accessed at index 10, which is out of bounds.\n", errout_str()); + + check("void f() {\n" + " const int a[10] = {};\n" + " for (int n = 0; 1; ++n) {\n" + " if (a[n] < 1) {\n" + " switch (a[n]) {\n" + " case 0:\n" + " break;\n" + " }\n" + " }\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[10]' accessed at index 9998, which is out of bounds.\n", errout_str()); } void array_index_for_neq() { diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index 1b49c465b76..5b4b4613221 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -4584,6 +4584,86 @@ class TestValueFlow : public TestFixture { ASSERT_EQUALS(true, values.empty()); values = tokenValues(code, "[ f . b"); ASSERT_EQUALS(true, values.empty()); + + code = "void f() {\n" // #13109 + " const int a[10] = {};\n" + " for (int n = 0; 1; ++n) {\n" + " (void)a[n];\n" + " break;\n" + " }\n" + "}\n"; + values = tokenValues(code, "n ]"); + ASSERT_EQUALS(2, values.size()); + auto it = values.begin(); + ASSERT_EQUALS(-1, it->intvalue); + ASSERT(it->isImpossible()); + ++it; + ASSERT_EQUALS(0, it->intvalue); + ASSERT(it->isPossible()); + + code = "void f() {\n" + " const int a[10] = {};\n" + " for (int n = 0; 1; ++n) {\n" + " if (a[n] < 1)\n" + " break;\n" + " }\n" + "}\n"; + values = tokenValues(code, "n ]"); + ASSERT_EQUALS(2, values.size()); + it = values.begin(); + ASSERT_EQUALS(-1, it->intvalue); + ASSERT(it->isImpossible()); + ++it; + ASSERT_EQUALS(0, it->intvalue); + ASSERT(it->isPossible()); + + code = "void f() {\n" + " const int a[10] = {};\n" + " for (int n = 0; 1; ++n) {\n" + " if (a[n] < 1)\n" + " throw 0;\n" + " }\n" + "}\n"; + values = tokenValues(code, "n ]"); + ASSERT_EQUALS(2, values.size()); + it = values.begin(); + ASSERT_EQUALS(-1, it->intvalue); + ASSERT(it->isImpossible()); + ++it; + ASSERT_EQUALS(0, it->intvalue); + ASSERT(it->isPossible()); + + code = "void f() {\n" + " const int a[10] = {};\n" + " for (int n = 0; 1; ++n) {\n" + " (void)a[n];\n" + " exit(1);\n" + " }\n" + "}\n"; + values = tokenValues(code, "n ]"); + ASSERT_EQUALS(2, values.size()); + it = values.begin(); + ASSERT_EQUALS(-1, it->intvalue); + ASSERT(it->isImpossible()); + ++it; + ASSERT_EQUALS(0, it->intvalue); + ASSERT(it->isPossible()); + + code = "void f() {\n" + " const int a[10] = {};\n" + " for (int n = 0; 1; ++n) {\n" + " if (a[n] < 1)\n" + " exit(1);\n" + " }\n" + "}\n"; + values = tokenValues(code, "n ]"); + ASSERT_EQUALS(2, values.size()); + it = values.begin(); + ASSERT_EQUALS(-1, it->intvalue); + ASSERT(it->isImpossible()); + ++it; + ASSERT_EQUALS(0, it->intvalue); + ASSERT(it->isPossible()); } void valueFlowSubFunction() {