From d7d165a0d2f320cbc060efe6270e4b4fb2f617fb Mon Sep 17 00:00:00 2001 From: Jeewoong Kim Date: Sat, 5 Sep 2026 08:21:30 +0900 Subject: [PATCH 1/3] Fix buffer size value flow for global new arrays --- lib/valueflow.cpp | 71 ++++++++++++++++++++++++++++++++++++++ test/testbufferoverrun.cpp | 23 ++++++++++++ 2 files changed, 94 insertions(+) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 325a55e98e5..d3cb671d3c9 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -7064,6 +7064,77 @@ static void valueFlowDynamicBufferSize(const TokenList& tokenlist, const SymbolD return sizeValue; }; + std::map globalBufferSizes; + + // Get buffer sizes for global pointers initialized with new + for (const Variable* var : symboldatabase.variableList()) { + if (!var || !var->isGlobal() || !var->isPointer() || var->isExtern()) + continue; + + const Token* nameTok = var->nameToken(); + if (!Token::Match(nameTok, "%var% ; %var% =")) + continue; + + const Token* initLhs = nameTok->tokAt(2); + if (!initLhs || initLhs->variable() != var) + continue; + + const Token* assignTok = initLhs->next(); + const Token* rhs = assignTok->astOperand2(); + while (rhs && rhs->isCast()) + rhs = rhs->astOperand2() ? rhs->astOperand2() : rhs->astOperand1(); + + if (!rhs || !rhs->isCpp() || rhs->str() != "new") + continue; + + const MathLib::bigint sizeValue = getBufferSizeFromNew(rhs); + if (sizeValue < 0) + continue; + + ValueFlow::Value value(sizeValue); + value.errorPath.emplace_back(assignTok, "Assign " + initLhs->str() + ", buffer with size " + MathLib::toString(sizeValue)); + value.valueType = ValueFlow::Value::ValueType::BUFFER_SIZE; + value.setKnown(); + globalBufferSizes.emplace(var, std::move(value)); + } + + // Remove initial buffer sizes if the pointers are changed later + for (const Token* tok = tokenlist.front(); tok && !globalBufferSizes.empty(); tok = tok->next()) { + const Variable* var = tok->variable(); + if (!var) + continue; + + const auto it = globalBufferSizes.find(var); + if (it == globalBufferSizes.end()) + continue; + + const Token* nameTok = var->nameToken(); + const Token* initLhs = Token::Match(nameTok, "%var% ; %var% =") ? nameTok->tokAt(2) : nullptr; + if (tok == nameTok || tok == initLhs) + continue; + + if (isVariableChanged(tok, 0, settings)) + globalBufferSizes.erase(it); + } + + // Propagate the buffer size through main() + for (const auto& entry : globalBufferSizes) { + const Variable* var = entry.first; + const ValueFlow::Value& value = entry.second; + const Token* nameTok = var->nameToken(); + const Token* initLhs = nameTok->tokAt(2); + + for (const Scope* functionScope : symboldatabase.functionScopes) { + if (functionScope->className != "main") + continue; + if (!functionScope->bodyStart || !functionScope->bodyEnd) + continue; + + valueFlowForward(const_cast(functionScope->bodyStart->next()), functionScope->bodyEnd, initLhs, value, tokenlist, errorLogger, settings); + break; + } + } + for (const Scope *functionScope : symboldatabase.functionScopes) { for (const Token *tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) { if (!Token::Match(tok, "[;{}] %var% =")) diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 7b36dbac035..cc049d9bdc5 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -3078,6 +3078,29 @@ class TestBufferOverrun : public TestFixture { " delete[] z;\n" "}\n"); ASSERT_EQUALS("[test.cpp:4:10]: (error) Array 'z[5]' accessed at index 7, which is out of bounds. [arrayIndexOutOfBounds]\n", errout_str()); + + // #14934 + check("int *a = new int[2];\n" + "int main() {\n" + " return a[5];\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3:13]: (error) Array 'a[2]' accessed at index 5, which is out of bounds. [arrayIndexOutOfBounds]\n", errout_str()); + + check("int *a = new int[2];\n" + "int main() {\n" + " a = new int[10];\n" + " return a[5];\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + check("int *a = new int[2];\n" + "void reset();\n" + "int main() {\n" + " reset();\n" + " return a[5];\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + } void buffer_overrun_2_struct() { From 726e0b18f5f4ea8c5e424f4a9e9f40ac8211718c Mon Sep 17 00:00:00 2001 From: Jeewoong Kim Date: Wed, 23 Sep 2026 09:59:45 +0900 Subject: [PATCH 2/3] valueflow: restrict global buffer sizes to static pointers Replace the main()-specific propagation with tracking for static global pointers that are not modified or escaped. Also handle allocation functions such as malloc in addition to new. --- lib/valueflow.cpp | 112 +++++++++++++++++++++++++++---------- test/testbufferoverrun.cpp | 32 ++++++++--- 2 files changed, 107 insertions(+), 37 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index d3cb671d3c9..c268a8f3d8d 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -7064,41 +7064,45 @@ static void valueFlowDynamicBufferSize(const TokenList& tokenlist, const SymbolD return sizeValue; }; + // Get buffer sizes for static global pointers std::map globalBufferSizes; - // Get buffer sizes for global pointers initialized with new for (const Variable* var : symboldatabase.variableList()) { - if (!var || !var->isGlobal() || !var->isPointer() || var->isExtern()) + if (!var || !var->isGlobal() || !var->isStatic() || var->isExtern() || !var->isPointer()) continue; const Token* nameTok = var->nameToken(); - if (!Token::Match(nameTok, "%var% ; %var% =")) + if (!nameTok) continue; - const Token* initLhs = nameTok->tokAt(2); - if (!initLhs || initLhs->variable() != var) + const Token* assignTok = nameTok->astParent(); + if (!assignTok || !assignTok->isAssignmentOp() || assignTok->astOperand1() != nameTok) continue; - const Token* assignTok = initLhs->next(); const Token* rhs = assignTok->astOperand2(); while (rhs && rhs->isCast()) rhs = rhs->astOperand2() ? rhs->astOperand2() : rhs->astOperand1(); - if (!rhs || !rhs->isCpp() || rhs->str() != "new") + if (!rhs) continue; - const MathLib::bigint sizeValue = getBufferSizeFromNew(rhs); + const bool isNew = rhs->isCpp() && (rhs->str() == "new" || (rhs->str() == "(" && Token::Match(rhs->astOperand1(), "::| operatornew"))); + if (!isNew && !Token::Match(rhs->previous(), "%name% (")) + continue; + + const MathLib::bigint sizeValue = isNew ? getBufferSizeFromNew(rhs) : getBufferSizeFromAllocFunc(rhs->previous()); if (sizeValue < 0) continue; ValueFlow::Value value(sizeValue); - value.errorPath.emplace_back(assignTok, "Assign " + initLhs->str() + ", buffer with size " + MathLib::toString(sizeValue)); + value.errorPath.emplace_back(assignTok, "Assign " + var->name() + ", buffer with size " + MathLib::toString(sizeValue)); value.valueType = ValueFlow::Value::ValueType::BUFFER_SIZE; value.setKnown(); + globalBufferSizes.emplace(var, std::move(value)); } - // Remove initial buffer sizes if the pointers are changed later + // Remove buffer sizes if the pointer is modified or escapes for (const Token* tok = tokenlist.front(); tok && !globalBufferSizes.empty(); tok = tok->next()) { const Variable* var = tok->variable(); if (!var) @@ -7108,31 +7112,83 @@ static void valueFlowDynamicBufferSize(const TokenList& tokenlist, const SymbolD if (it == globalBufferSizes.end()) continue; - const Token* nameTok = var->nameToken(); - const Token* initLhs = Token::Match(nameTok, "%var% ; %var% =") ? nameTok->tokAt(2) : nullptr; - if (tok == nameTok || tok == initLhs) + // Ignore the initialization itself + if (tok == var->nameToken()) continue; - if (isVariableChanged(tok, 0, settings)) + const Token* parent = tok->astParent(); + if (!parent) { globalBufferSizes.erase(it); - } + continue; + } - // Propagate the buffer size through main() - for (const auto& entry : globalBufferSizes) { - const Variable* var = entry.first; - const ValueFlow::Value& value = entry.second; - const Token* nameTok = var->nameToken(); - const Token* initLhs = nameTok->tokAt(2); + bool invalidate = false; + // Direct modification of the pointer object or its lifetime. + if (Token::Match(parent, "++|--|&") && !parent->astOperand2()) { + invalidate = true; + } else if (Token::simpleMatch(parent, "delete")) { + invalidate = true; + } else { + // Follow expressions that preserve or derive the pointer value. + const Token* expr = tok; + while (expr->astParent()) { + const Token* exprParent = expr->astParent(); + if (exprParent->isCast()) { + expr = exprParent; + continue; + } - for (const Scope* functionScope : symboldatabase.functionScopes) { - if (functionScope->className != "main") - continue; - if (!functionScope->bodyStart || !functionScope->bodyEnd) - continue; + if (Token::Match(exprParent, "+|-") && exprParent->valueType() && exprParent->valueType()->pointer > 0) { + expr = exprParent; + continue; + } - valueFlowForward(const_cast(functionScope->bodyStart->next()), functionScope->bodyEnd, initLhs, value, tokenlist, errorLogger, settings); - break; + // Follow a pointer value through the result of a conditional expression. + if (Token::simpleMatch(exprParent, ":") && exprParent->astParent() && Token::simpleMatch(exprParent->astParent(), "?") && exprParent == exprParent->astParent()->astOperand2()) { + expr = exprParent->astParent(); + continue; + } + + // &p[index] creates a pointer alias, whereas p[index] itself does not. + if (Token::simpleMatch(exprParent, "[") && expr == exprParent->astOperand1() && exprParent->astParent() && exprParent->astParent()->isUnaryOp("&")) { + expr = exprParent->astParent(); + continue; + } + + break; + } + + int argn = -1; + if (getTokenArgumentFunction(expr, argn)) { + invalidate = true; + } else { + const Token* context = expr->astParent(); + // Reassigning the pointer, or storing the pointer value elsewhere, + // invalidates the whole-TU buffer-size assumption. + if (context && context->isAssignmentOp()) { + invalidate = true; + } else if (Token::simpleMatch(context, "return")) { + invalidate = true; + } else if (context && isLikelyStreamRead(context)) { + invalidate = true; + } + } } + + if (invalidate) + globalBufferSizes.erase(it); + } + + // Set buffer sizes for stable static global pointers + for (const Token* tok = tokenlist.front(); tok; tok = tok->next()) { + if (!tok->variable()) + continue; + + const auto it = globalBufferSizes.find(tok->variable()); + if (it == globalBufferSizes.end()) + continue; + + setTokenValue(const_cast(tok), it->second, settings); } for (const Scope *functionScope : symboldatabase.functionScopes) { diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index cc049d9bdc5..009de90c86d 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -3079,28 +3079,42 @@ class TestBufferOverrun : public TestFixture { "}\n"); ASSERT_EQUALS("[test.cpp:4:10]: (error) Array 'z[5]' accessed at index 7, which is out of bounds. [arrayIndexOutOfBounds]\n", errout_str()); - // #14934 - check("int *a = new int[2];\n" - "int main() {\n" + // Global dynamic buffers with internal linkage + check("static int *a = new int[2];\n" + "int f() {\n" " return a[5];\n" "}\n"); ASSERT_EQUALS("[test.cpp:3:13]: (error) Array 'a[2]' accessed at index 5, which is out of bounds. [arrayIndexOutOfBounds]\n", errout_str()); - check("int *a = new int[2];\n" - "int main() {\n" + check("static int *a = (int *)malloc(2 * sizeof(int));\n" + "int f() {\n" + " return a[5];\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3:13]: (error) Array 'a[2]' accessed at index 5, which is out of bounds. [arrayIndexOutOfBounds]\n", errout_str()); + + check("static int *a = new int[2];\n" + "int f() {\n" " a = new int[10];\n" " return a[5];\n" "}\n"); ASSERT_EQUALS("", errout_str()); - check("int *a = new int[2];\n" - "void reset();\n" - "int main() {\n" - " reset();\n" + check("void use(int *);\n" + "static int *a = new int[2];\n" + "int f() {\n" + " use(a);\n" " return a[5];\n" "}\n"); ASSERT_EQUALS("", errout_str()); + check("void unrelated();\n" + "static int *a = new int[2];\n" + "int f() {\n" + " unrelated();\n" + " return a[5];\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5:13]: (error) Array 'a[2]' accessed at index 5, which is out of bounds. [arrayIndexOutOfBounds]\n", errout_str()); + } void buffer_overrun_2_struct() { From d781b29136511893a14ebf440abea66f53891a0b Mon Sep 17 00:00:00 2001 From: Jeewoong Kim Date: Wed, 23 Sep 2026 11:47:55 +0900 Subject: [PATCH 3/3] valueflow: remove redundant token check --- lib/valueflow.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index c268a8f3d8d..921162674da 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -7144,7 +7144,7 @@ static void valueFlowDynamicBufferSize(const TokenList& tokenlist, const SymbolD } // Follow a pointer value through the result of a conditional expression. - if (Token::simpleMatch(exprParent, ":") && exprParent->astParent() && Token::simpleMatch(exprParent->astParent(), "?") && exprParent == exprParent->astParent()->astOperand2()) { + if (Token::simpleMatch(exprParent, ":") && Token::simpleMatch(exprParent->astParent(), "?") && exprParent == exprParent->astParent()->astOperand2()) { expr = exprParent->astParent(); continue; }