From 9d1755f449a75d87bab9fb62b4729a4d5c479889 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 19 Mar 2019 13:13:29 +0100 Subject: [PATCH] Revert "CheckBufferOverrun: Handle multidimensional arrays" This reverts commit e98a4a6f1475db03473d544d576827e49f9a9575. --- lib/checkbufferoverrun.cpp | 137 ++++++++----------------------------- lib/checkbufferoverrun.h | 8 +-- test/testbufferoverrun.cpp | 53 +++++++------- 3 files changed, 61 insertions(+), 137 deletions(-) diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 5e1786ade..42bf4e151 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -201,15 +201,8 @@ void CheckBufferOverrun::arrayIndex() continue; } - 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()) + const Token *indexToken = tok->astOperand2(); + if (!indexToken) continue; std::vector dimensions; @@ -219,17 +212,7 @@ void CheckBufferOverrun::arrayIndex() if (array->variable()->isArray() && !array->variable()->dimensions().empty()) { dimensions = array->variable()->dimensions(); - 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; - }); - } + mightBeLarger = (dimensions.size() >= 1 && (dimensions[0].num <= 1 || !dimensions[0].tok)); } else if (const Token *stringLiteral = array->getValueTokenMinStrSize()) { Dimension dim; dim.tok = nullptr; @@ -253,94 +236,53 @@ 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++) { - 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 ValueFlow::Value *value = indexToken->getMaxValue(cond == 1); + if (!value) + continue; + const MathLib::bigint index = value->intvalue; + if (index < dim) + continue; + if (index == dim) { const Token *parent = tok; while (Token::simpleMatch(parent, "[")) parent = parent->astParent(); if (parent->isUnaryOp("&")) continue; } - if (overflow || equal) { - arrayIndexError(tok, dimensions, indexes); - break; - } + arrayIndexError(tok, dimensions, value); } } // Negative index - 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); + const ValueFlow::Value *negativeValue = indexToken->getValueLE(-1, mSettings); + if (negativeValue) { + negativeIndexError(tok, dimensions, negativeValue); } } } -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) +static std::string arrayIndexMessage(const Token *tok, const std::vector &dimensions, const ValueFlow::Value *index) { std::string array = tok->astOperand1()->expressionString(); for (const Dimension &dim : dimensions) array += "[" + MathLib::toString(dim.num) + "]"; std::ostringstream errmsg; - if (condition) - errmsg << ValueFlow::eitherTheConditionIsRedundant(condition) - << " or the array '" + array + "' is accessed at index " << stringifyIndexes(tok->astOperand1()->expressionString(), indexValues) << ", which is out of bounds."; + if (index->condition) + errmsg << ValueFlow::eitherTheConditionIsRedundant(index->condition) + << " or the array '" + array + "' is accessed at index " << index->intvalue << ", which is out of bounds."; else - errmsg << "Array '" << array << "' accessed at index " << stringifyIndexes(tok->astOperand1()->expressionString(), indexValues) << ", which is out of bounds."; + errmsg << "Array '" << array << "' accessed at index " << index->intvalue << ", which is out of bounds."; return errmsg.str(); } -void CheckBufferOverrun::arrayIndexError(const Token *tok, const std::vector &dimensions, const std::vector &indexes) +void CheckBufferOverrun::arrayIndexError(const Token *tok, const std::vector &dimensions, const ValueFlow::Value *index) { if (!tok) { reportError(tok, Severity::error, "arrayIndexOutOfBounds", "Array 'arr[16]' accessed at index 16, which is out of bounds.", CWE788, false); @@ -348,51 +290,28 @@ 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, indexes, condition), + arrayIndexMessage(tok, dimensions, index), CWE788, index->isInconclusive()); } -void CheckBufferOverrun::negativeIndexError(const Token *tok, const std::vector &dimensions, const std::vector &indexes) +void CheckBufferOverrun::negativeIndexError(const Token *tok, const std::vector &dimensions, const ValueFlow::Value *negativeValue) { - if (!tok) { + if (!negativeValue) { reportError(tok, Severity::error, "negativeIndex", "Negative array index", CWE786, false); 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; - } + if (!negativeValue->errorSeverity() && !mSettings->isEnabled(Settings::WARNING)) + return; reportError(getErrorPath(tok, negativeValue, "Negative array index"), negativeValue->errorSeverity() ? Severity::error : Severity::warning, "negativeIndex", - arrayIndexMessage(tok, dimensions, indexes, condition), + arrayIndexMessage(tok, dimensions, negativeValue), CWE786, negativeValue->isInconclusive()); } diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 712ea3658..b7a1fb776 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(), std::vector()); - c.negativeIndexError(nullptr, std::vector(), std::vector()); + c.arrayIndexError(nullptr, std::vector(), nullptr); + c.negativeIndexError(nullptr, std::vector(), nullptr); 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 std::vector &indexes); - void negativeIndexError(const Token *tok, const std::vector &dimensions, const std::vector &indexes); + 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 bufferOverflow(); void bufferOverflowError(const Token *tok, const ValueFlow::Value *value); diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index a29ab8c19..4640cc2fb 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 - TEST_CASE(array_index_34); // ticket #3063 + // TODO multidim 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 - TEST_CASE(array_index_multidim); + // TODO 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;" "}"); - ASSERT_EQUALS("[test.cpp:9]: (error) Array 'x.str[1]' accessed at index 1, which is out of bounds.\n", errout.str()); + TODO_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("[test.cpp:3]: (error) Array 'ab.a[1]' accessed at index 2, which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("", 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]' 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()); + 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()); check("struct TEST\n" "{\n" @@ -1155,11 +1155,14 @@ 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]' 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: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: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]' 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()); + "[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()); check("struct TEST\n" "{\n" @@ -1175,10 +1178,12 @@ private: " ptest->a[9][5] = 4;\n" " ptest->a[0][50] = 4;\n" "}"); - 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()); + 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()); } void array_index_35() { // ticket #2889 @@ -1545,67 +1550,67 @@ private: " char a[2][2];\n" " a[2][1] = 'a';\n" "}"); - ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2]' accessed at index a[2][1], which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2]' index a[2][1] 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]' accessed at index a[1][2], which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2]' index a[1][2] 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]' accessed at index a[2][1][1], which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2][2]' index a[2][1][1] 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]' accessed at index a[1][2][1], which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2][2]' index a[1][2][1] 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]' accessed at index a[1][2][1][1], which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2][2][2]' index a[1][2][1][1] 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]' accessed at index a[1][1][2], which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[2][2][2]' index a[1][1][2] 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]' accessed at index a[6][12][2], which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'a[10][10][10]' index a[6][12][2] 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]' accessed at index a[6][40][10], which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (error) Array 'a[10][10][10]' index a[6][40][10] 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]' accessed at index a[2][2][2], which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (error) Array 'a[1][1][1]' index a[2][2][2] 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]' accessed at index a[6][6][2], which is out of bounds.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (error) Array 'a[6][6][6]' index a[6][6][2] out of bounds.\n", errout.str()); check("void f() {\n" " int a[2][2];\n"