From e98a4a6f1475db03473d544d576827e49f9a9575 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 19 Mar 2019 09:29:32 +0100 Subject: [PATCH] CheckBufferOverrun: Handle multidimensional arrays --- lib/checkbufferoverrun.cpp | 137 +++++++++++++++++++++++++++++-------- lib/checkbufferoverrun.h | 8 +-- test/testbufferoverrun.cpp | 53 +++++++------- 3 files changed, 137 insertions(+), 61 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 42bf4e151..5e1786ade 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -201,8 +201,15 @@ void CheckBufferOverrun::arrayIndex() continue; } - const Token *indexToken = tok->astOperand2(); - if (!indexToken) + std::vector indexTokens; + for (const Token *tok2 = tok; tok2 && tok2->str() == "["; tok2 = tok2->link()->next()) { + if (!tok2->astOperand2()) { + indexTokens.clear(); + break; + } + indexTokens.emplace_back(tok2->astOperand2()); + } + if (indexTokens.empty()) continue; std::vector dimensions; @@ -212,7 +219,17 @@ void CheckBufferOverrun::arrayIndex() if (array->variable()->isArray() && !array->variable()->dimensions().empty()) { dimensions = array->variable()->dimensions(); - mightBeLarger = (dimensions.size() >= 1 && (dimensions[0].num <= 1 || !dimensions[0].tok)); + if (dimensions.size() >= 1 && (dimensions[0].num <= 1 || !dimensions[0].tok)) { + mightBeLarger = false; + visitAstNodes(tok->astOperand1(), + [&](const Token *child) { + if (child->originalName() == "->") { + mightBeLarger = true; + return ChildrenToVisit::none; + } + return ChildrenToVisit::op1_and_op2; + }); + } } else if (const Token *stringLiteral = array->getValueTokenMinStrSize()) { Dimension dim; dim.tok = nullptr; @@ -236,53 +253,94 @@ void CheckBufferOverrun::arrayIndex() if (dimensions.empty()) continue; - const MathLib::bigint dim = dimensions[0].num; - // Positive index if (!mightBeLarger) { // TODO check arrays with dim 1 also for (int cond = 0; cond < 2; cond++) { - const ValueFlow::Value *value = indexToken->getMaxValue(cond == 1); - if (!value) - continue; - const MathLib::bigint index = value->intvalue; - if (index < dim) - continue; - if (index == dim) { + bool equal = false; + bool overflow = false; + bool allKnown = true; + std::vector indexes; + for (size_t i = 0; i < dimensions.size() && i < indexTokens.size(); ++i) { + const ValueFlow::Value *value = indexTokens[i]->getMaxValue(cond == 1); + indexes.push_back(value); + if (!value) + continue; + if (!value->isKnown()) { + if (!allKnown) + continue; + allKnown = false; + } + if (array->variable()->isArray() && dimensions[i].num == 0) + continue; + if (value->intvalue == dimensions[i].num) + equal = true; + else if (value->intvalue > dimensions[i].num) + overflow = true; + } + if (!overflow && equal) { const Token *parent = tok; while (Token::simpleMatch(parent, "[")) parent = parent->astParent(); if (parent->isUnaryOp("&")) continue; } - arrayIndexError(tok, dimensions, value); + if (overflow || equal) { + arrayIndexError(tok, dimensions, indexes); + break; + } } } // Negative index - const ValueFlow::Value *negativeValue = indexToken->getValueLE(-1, mSettings); - if (negativeValue) { - negativeIndexError(tok, dimensions, negativeValue); + bool neg = false; + std::vector negativeIndexes; + for (size_t i = 0; i < indexTokens.size(); ++i) { + const ValueFlow::Value *negativeValue = indexTokens[i]->getValueLE(-1, mSettings); + negativeIndexes.emplace_back(negativeValue); + if (negativeValue) + neg = true; + } + if (neg) { + negativeIndexError(tok, dimensions, negativeIndexes); } } } -static std::string arrayIndexMessage(const Token *tok, const std::vector &dimensions, const ValueFlow::Value *index) +static std::string stringifyIndexes(const std::string &array, const std::vector &indexValues) +{ + if (indexValues.size() == 1) + return MathLib::toString(indexValues[0]->intvalue); + + std::ostringstream ret; + ret << array; + for (const ValueFlow::Value *index : indexValues) { + ret << "["; + if (index) + ret << index->intvalue; + else + ret << "*"; + ret << "]"; + } + return ret.str(); +} + +static std::string arrayIndexMessage(const Token *tok, const std::vector &dimensions, const std::vector &indexValues, const Token *condition) { std::string array = tok->astOperand1()->expressionString(); for (const Dimension &dim : dimensions) array += "[" + MathLib::toString(dim.num) + "]"; std::ostringstream errmsg; - if (index->condition) - errmsg << ValueFlow::eitherTheConditionIsRedundant(index->condition) - << " or the array '" + array + "' is accessed at index " << index->intvalue << ", which is out of bounds."; + if (condition) + errmsg << ValueFlow::eitherTheConditionIsRedundant(condition) + << " or the array '" + array + "' is accessed at index " << stringifyIndexes(tok->astOperand1()->expressionString(), indexValues) << ", which is out of bounds."; else - errmsg << "Array '" << array << "' accessed at index " << index->intvalue << ", which is out of bounds."; + errmsg << "Array '" << array << "' accessed at index " << stringifyIndexes(tok->astOperand1()->expressionString(), indexValues) << ", which is out of bounds."; return errmsg.str(); } -void CheckBufferOverrun::arrayIndexError(const Token *tok, const std::vector &dimensions, const ValueFlow::Value *index) +void CheckBufferOverrun::arrayIndexError(const Token *tok, const std::vector &dimensions, const std::vector &indexes) { if (!tok) { reportError(tok, Severity::error, "arrayIndexOutOfBounds", "Array 'arr[16]' accessed at index 16, which is out of bounds.", CWE788, false); @@ -290,28 +348,51 @@ void CheckBufferOverrun::arrayIndexError(const Token *tok, const std::vectorerrorSeverity() && !mSettings->isEnabled(Settings::WARNING)) + return; + if (indexValue->condition) + condition = indexValue->condition; + if (!index || !indexValue->errorPath.empty()) + index = indexValue; + } + reportError(getErrorPath(tok, index, "Array index out of bounds"), index->errorSeverity() ? Severity::error : Severity::warning, index->condition ? "arrayIndexOutOfBoundsCond" : "arrayIndexOutOfBounds", - arrayIndexMessage(tok, dimensions, index), + arrayIndexMessage(tok, dimensions, indexes, condition), CWE788, index->isInconclusive()); } -void CheckBufferOverrun::negativeIndexError(const Token *tok, const std::vector &dimensions, const ValueFlow::Value *negativeValue) +void CheckBufferOverrun::negativeIndexError(const Token *tok, const std::vector &dimensions, const std::vector &indexes) { - if (!negativeValue) { + if (!tok) { reportError(tok, Severity::error, "negativeIndex", "Negative array index", CWE786, false); return; } - if (!negativeValue->errorSeverity() && !mSettings->isEnabled(Settings::WARNING)) - return; + const Token *condition = nullptr; + const ValueFlow::Value *negativeValue = nullptr; + for (const ValueFlow::Value *indexValue: indexes) { + if (!indexValue) + continue; + if (!indexValue->errorSeverity() && !mSettings->isEnabled(Settings::WARNING)) + return; + if (indexValue->condition) + condition = indexValue->condition; + if (!negativeValue || !indexValue->errorPath.empty()) + negativeValue = indexValue; + } reportError(getErrorPath(tok, negativeValue, "Negative array index"), negativeValue->errorSeverity() ? Severity::error : Severity::warning, "negativeIndex", - arrayIndexMessage(tok, dimensions, negativeValue), + arrayIndexMessage(tok, dimensions, indexes, condition), CWE786, negativeValue->isInconclusive()); } diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index b7a1fb776..712ea3658 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -70,8 +70,8 @@ public: void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE { CheckBufferOverrun c(nullptr, settings, errorLogger); - c.arrayIndexError(nullptr, std::vector(), nullptr); - c.negativeIndexError(nullptr, std::vector(), nullptr); + c.arrayIndexError(nullptr, std::vector(), std::vector()); + c.negativeIndexError(nullptr, std::vector(), std::vector()); c.arrayIndexThenCheckError(nullptr, "i"); c.bufferOverflowError(nullptr, nullptr); } @@ -79,8 +79,8 @@ public: private: void arrayIndex(); - void arrayIndexError(const Token *tok, const std::vector &dimensions, const ValueFlow::Value *index); - void negativeIndexError(const Token *tok, const std::vector &dimensions, const ValueFlow::Value *negativeValue); + void arrayIndexError(const Token *tok, const std::vector &dimensions, const std::vector &indexes); + void negativeIndexError(const Token *tok, const std::vector &dimensions, const std::vector &indexes); void bufferOverflow(); void bufferOverflowError(const Token *tok, const ValueFlow::Value *value); diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 4640cc2fb..a29ab8c19 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -113,7 +113,7 @@ private: TEST_CASE(array_index_31); // ticket #2120 - out of bounds in subfunction when type is unknown TEST_CASE(array_index_32); TEST_CASE(array_index_33); // ticket #3044 - // TODO multidim TEST_CASE(array_index_34); // ticket #3063 + TEST_CASE(array_index_34); // ticket #3063 TEST_CASE(array_index_35); // ticket #2889 TEST_CASE(array_index_36); // ticket #2960 TEST_CASE(array_index_37); @@ -127,7 +127,7 @@ private: TEST_CASE(array_index_45); // #4207 - calling function with variable number of parameters (...) TEST_CASE(array_index_46); // #4840 - two-statement for loop TEST_CASE(array_index_47); // #5849 - // TODO multidim TEST_CASE(array_index_multidim); + TEST_CASE(array_index_multidim); TEST_CASE(array_index_switch_in_for); TEST_CASE(array_index_for_in_for); // FP: #2634 TEST_CASE(array_index_calculation); @@ -571,7 +571,7 @@ private: " struct ABC x;\n" " x.str[1] = 0;" "}"); - TODO_ASSERT_EQUALS("[test.cpp:9]: (error) Array 'x.str[1]' accessed at index 1, which is out of bounds.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:9]: (error) Array 'x.str[1]' accessed at index 1, which is out of bounds.\n", errout.str()); check("struct foo\n" "{\n" @@ -596,7 +596,7 @@ private: "void f() {\n" " ab.a[2] = 0;\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (error) Array 'ab.a[1]' accessed at index 2, which is out of bounds.\n", errout.str()); } @@ -1134,8 +1134,8 @@ private: " y[0][2][0] = 0;\n" " y[0][0][2] = 0;\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Array 'y[2][2][2]' index y[0][2][0] out of bounds.\n" - "[test.cpp:4]: (error) Array 'y[2][2][2]' index y[0][0][2] out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (error) Array 'y[2][2][2]' accessed at index y[0][2][0], which is out of bounds.\n" + "[test.cpp:4]: (error) Array 'y[2][2][2]' accessed at index y[0][0][2], which is out of bounds.\n", errout.str()); check("struct TEST\n" "{\n" @@ -1155,14 +1155,11 @@ private: " ptest->b[0][19] = 4;\n" "}"); ASSERT_EQUALS("[test.cpp:9]: (error) Array 'test.a[10]' accessed at index 10, which is out of bounds.\n" - "[test.cpp:10]: (error) Array 'test.b[10][5]' index test.b[10][2] out of bounds.\n" - "[test.cpp:11]: (error) Array 'test.b[10][5]' index test.b[0][19] out of bounds.\n" + "[test.cpp:10]: (error) Array 'test.b[10][5]' accessed at index test.b[10][2], which is out of bounds.\n" + "[test.cpp:11]: (error) Array 'test.b[10][5]' accessed at index test.b[0][19], which is out of bounds.\n" "[test.cpp:14]: (error) Array 'ptest->a[10]' accessed at index 10, which is out of bounds.\n" - "[test.cpp:15]: (error) Array 'ptest->b[10][5]' index ptest->b[10][2] out of bounds.\n" - "[test.cpp:16]: (error) Array 'ptest->b[10][5]' index ptest->b[0][19] out of bounds.\n" - "[test.cpp:14]: (error) Array 'ptest.a[10]' accessed at index 10, which is out of bounds.\n" - "[test.cpp:15]: (error) Array 'ptest.b[10][5]' index ptest.b[10][2] out of bounds.\n" - "[test.cpp:16]: (error) Array 'ptest.b[10][5]' index ptest.b[0][19] out of bounds.\n", errout.str()); + "[test.cpp:15]: (error) Array 'ptest->b[10][5]' accessed at index ptest->b[10][2], which is out of bounds.\n" + "[test.cpp:16]: (error) Array 'ptest->b[10][5]' accessed at index ptest->b[0][19], which is out of bounds.\n", errout.str()); check("struct TEST\n" "{\n" @@ -1178,12 +1175,10 @@ private: " ptest->a[9][5] = 4;\n" " ptest->a[0][50] = 4;\n" "}"); - ASSERT_EQUALS("[test.cpp:8]: (error) Array 'test.a[10][5]' index test.a[9][5] out of bounds.\n" - "[test.cpp:9]: (error) Array 'test.a[10][5]' index test.a[0][50] out of bounds.\n" - "[test.cpp:12]: (error) Array 'ptest->a[10][5]' index ptest->a[9][5] out of bounds.\n" - "[test.cpp:13]: (error) Array 'ptest->a[10][5]' index ptest->a[0][50] out of bounds.\n" - "[test.cpp:12]: (error) Array 'ptest.a[10][5]' index ptest.a[9][5] out of bounds.\n" - "[test.cpp:13]: (error) Array 'ptest.a[10][5]' index ptest.a[0][50] out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:8]: (error) Array 'test.a[10][5]' accessed at index test.a[9][5], which is out of bounds.\n" + "[test.cpp:9]: (error) Array 'test.a[10][5]' accessed at index test.a[0][50], which is out of bounds.\n" + "[test.cpp:12]: (error) Array 'ptest->a[10][5]' accessed at index ptest->a[9][5], which is out of bounds.\n" + "[test.cpp:13]: (error) Array 'ptest->a[10][5]' accessed at index ptest->a[0][50], which is out of bounds.\n", errout.str()); } void array_index_35() { // ticket #2889 @@ -1550,67 +1545,67 @@ private: " char a[2][2];\n" " a[2][1] = 'a';\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2]' index a[2][1] out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2]' accessed at index a[2][1], which is out of bounds.\n", errout.str()); check("void f()\n" "{\n" " char a[2][2];\n" " a[1][2] = 'a';\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2]' index a[1][2] out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2]' accessed at index a[1][2], which is out of bounds.\n", errout.str()); check("void f()\n" "{\n" " char a[2][2][2];\n" " a[2][1][1] = 'a';\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2][2]' index a[2][1][1] out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2][2]' accessed at index a[2][1][1], which is out of bounds.\n", errout.str()); check("void f()\n" "{\n" " char a[2][2][2];\n" " a[1][2][1] = 'a';\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2][2]' index a[1][2][1] out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2][2]' accessed at index a[1][2][1], which is out of bounds.\n", errout.str()); check("void f()\n" "{\n" " char a[2][2][2][2];\n" " a[1][2][1][1] = 'a';\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2][2][2]' index a[1][2][1][1] out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2][2][2]' accessed at index a[1][2][1][1], which is out of bounds.\n", errout.str()); check("void f()\n" "{\n" " char a[2][2][2];\n" " a[1][1][2] = 'a';\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2][2]' index a[1][1][2] out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2][2]' accessed at index a[1][1][2], which is out of bounds.\n", errout.str()); check("void f()\n" "{\n" " char a[10][10][10];\n" " a[2*3][4*3][2] = 'a';\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[10][10][10]' index a[6][12][2] out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[10][10][10]' accessed at index a[6][12][2], which is out of bounds.\n", errout.str()); check("void f() {\n" " char a[10][10][10];\n" " a[6][40][10] = 'a';\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Array 'a[10][10][10]' index a[6][40][10] out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (error) Array 'a[10][10][10]' accessed at index a[6][40][10], which is out of bounds.\n", errout.str()); check("void f() {\n" " char a[1][1][1];\n" " a[2][2][2] = 'a';\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Array 'a[1][1][1]' index a[2][2][2] out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (error) Array 'a[1][1][1]' accessed at index a[2][2][2], which is out of bounds.\n", errout.str()); check("void f() {\n" " char a[6][6][6];\n" " a[6][6][2] = 'a';\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (error) Array 'a[6][6][6]' index a[6][6][2] out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (error) Array 'a[6][6][6]' accessed at index a[6][6][2], which is out of bounds.\n", errout.str()); check("void f() {\n" " int a[2][2];\n"