From fbdfb6080931e6a75445a8bb3eb0ba11d6f5192c Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Mon, 7 Mar 2022 19:43:40 +0100 Subject: [PATCH] Warn about buffer overruns when allocating memory with new (#3879) * Warn about buffer overruns when allocating memory with new * Format * Avoid FP --- lib/valueflow.cpp | 107 ++++++++++++++++++++++++++----------- test/testbufferoverrun.cpp | 83 +++++++++++++++++++++++++--- 2 files changed, 150 insertions(+), 40 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index f1310f3ee..8dbc93268 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -7842,6 +7842,78 @@ struct ContainerConditionHandler : ConditionHandler { static void valueFlowDynamicBufferSize(TokenList* tokenlist, SymbolDatabase* symboldatabase, const Settings* settings) { + auto getBufferSizeFromAllocFunc = [&](const Token* funcTok) -> MathLib::bigint { + MathLib::bigint sizeValue = -1; + const Library::AllocFunc* allocFunc = settings->library.getAllocFuncInfo(funcTok); + if (!allocFunc) + allocFunc = settings->library.getReallocFuncInfo(funcTok); + if (!allocFunc || allocFunc->bufferSize == Library::AllocFunc::BufferSize::none) + return sizeValue; + + const std::vector args = getArguments(funcTok); + + const Token* const arg1 = (args.size() >= allocFunc->bufferSizeArg1) ? args[allocFunc->bufferSizeArg1 - 1] : nullptr; + const Token* const arg2 = (args.size() >= allocFunc->bufferSizeArg2) ? args[allocFunc->bufferSizeArg2 - 1] : nullptr; + + switch (allocFunc->bufferSize) { + case Library::AllocFunc::BufferSize::none: + break; + case Library::AllocFunc::BufferSize::malloc: + if (arg1 && arg1->hasKnownIntValue()) + sizeValue = arg1->getKnownIntValue(); + break; + case Library::AllocFunc::BufferSize::calloc: + if (arg1 && arg2 && arg1->hasKnownIntValue() && arg2->hasKnownIntValue()) + sizeValue = arg1->getKnownIntValue() * arg2->getKnownIntValue(); + break; + case Library::AllocFunc::BufferSize::strdup: + if (arg1 && arg1->hasKnownValue()) { + const ValueFlow::Value& value = arg1->values().back(); + if (value.isTokValue() && value.tokvalue->tokType() == Token::eString) + sizeValue = Token::getStrLength(value.tokvalue) + 1; // Add one for the null terminator + } + break; + } + return sizeValue; + }; + + auto getBufferSizeFromNew = [&](const Token* newTok) -> MathLib::bigint { + MathLib::bigint sizeValue = -1, numElem = -1; + + if (newTok && newTok->astOperand1()) { // number of elements + const Token* bracTok = nullptr, *typeTok = nullptr; + if (newTok->astOperand1()->str() == "[") + bracTok = newTok->astOperand1(); + else if (newTok->astOperand1()->str() == "(") { + if (newTok->astOperand1()->astOperand1() && newTok->astOperand1()->astOperand1()->str() == "[") + bracTok = newTok->astOperand1()->astOperand1(); + else + typeTok = newTok->astOperand1()->astOperand1(); + } + else { + typeTok = newTok->astOperand1(); + if (typeTok && typeTok->str() == "{") + typeTok = typeTok->astOperand2(); + } + if (bracTok && bracTok->astOperand2() && bracTok->astOperand2()->hasKnownIntValue()) + numElem = bracTok->astOperand2()->getKnownIntValue(); + else if (Token::Match(typeTok, "%type%")) + numElem = 1; + } + + if (numElem >= 0 && newTok->astParent() && newTok->astParent()->isAssignmentOp()) { // size of the allocated type + const Token* typeTok = newTok->astParent()->astOperand1(); // TODO: implement fallback for e.g. "auto p = new Type;" + if (!typeTok || !typeTok->varId()) + typeTok = newTok->astParent()->previous(); // hack for "int** z = ..." + if (typeTok && typeTok->valueType()) { + const MathLib::bigint typeSize = typeTok->valueType()->typeSize(*settings, typeTok->valueType()->pointer > 1); + if (typeSize >= 0) + sizeValue = numElem * typeSize; + } + } + return sizeValue; + }; + for (const Scope *functionScope : symboldatabase->functionScopes) { for (const Token *tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) { if (!Token::Match(tok, "[;{}] %var% =")) @@ -7856,40 +7928,11 @@ static void valueFlowDynamicBufferSize(TokenList* tokenlist, SymbolDatabase* sym if (!rhs) continue; - if (!Token::Match(rhs->previous(), "%name% (")) + const bool isNew = symboldatabase->isCPP() && rhs->str() == "new"; + if (!isNew && !Token::Match(rhs->previous(), "%name% (")) continue; - const Library::AllocFunc *allocFunc = settings->library.getAllocFuncInfo(rhs->previous()); - if (!allocFunc) - allocFunc = settings->library.getReallocFuncInfo(rhs->previous()); - if (!allocFunc || allocFunc->bufferSize == Library::AllocFunc::BufferSize::none) - continue; - - const std::vector args = getArguments(rhs->previous()); - - const Token * const arg1 = (args.size() >= allocFunc->bufferSizeArg1) ? args[allocFunc->bufferSizeArg1 - 1] : nullptr; - const Token * const arg2 = (args.size() >= allocFunc->bufferSizeArg2) ? args[allocFunc->bufferSizeArg2 - 1] : nullptr; - - MathLib::bigint sizeValue = -1; - switch (allocFunc->bufferSize) { - case Library::AllocFunc::BufferSize::none: - break; - case Library::AllocFunc::BufferSize::malloc: - if (arg1 && arg1->hasKnownIntValue()) - sizeValue = arg1->getKnownIntValue(); - break; - case Library::AllocFunc::BufferSize::calloc: - if (arg1 && arg2 && arg1->hasKnownIntValue() && arg2->hasKnownIntValue()) - sizeValue = arg1->getKnownIntValue() * arg2->getKnownIntValue(); - break; - case Library::AllocFunc::BufferSize::strdup: - if (arg1 && arg1->hasKnownValue()) { - const ValueFlow::Value &value = arg1->values().back(); - if (value.isTokValue() && value.tokvalue->tokType() == Token::eString) - sizeValue = Token::getStrLength(value.tokvalue) + 1; // Add one for the null terminator - } - break; - } + const MathLib::bigint sizeValue = isNew ? getBufferSizeFromNew(rhs) : getBufferSizeFromAllocFunc(rhs->previous()); if (sizeValue < 0) continue; diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index 25938a513..7e86382e1 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -214,6 +214,7 @@ private: TEST_CASE(array_index_enum_array); // #8439 TEST_CASE(array_index_container); // #9386 TEST_CASE(array_index_two_for_loops); + TEST_CASE(array_index_new); // #7690 TEST_CASE(buffer_overrun_2_struct); TEST_CASE(buffer_overrun_3); @@ -441,7 +442,7 @@ private: " str[i] = 0;\n" "}\n" "void b() { a(16); }"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Array 'str[16]' accessed at index 16, which is out of bounds.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'str[16]' accessed at index 16, which is out of bounds.\n", errout.str()); } void array_index_4() { @@ -2560,6 +2561,72 @@ private: ASSERT_EQUALS("", errout.str()); } + void array_index_new() { // #7690 + check("void f() {\n" + " int* z = new int;\n" + " for (int n = 0; n < 8; ++n)\n" + " z[n] = 0;\n" + " delete[] z;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'z[1]' accessed at index 7, which is out of bounds.\n", errout.str()); + + check("void f() {\n" + " int* z = new int(1);\n" + " for (int n = 0; n < 8; ++n)\n" + " z[n] = 0;\n" + " delete[] z;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'z[1]' accessed at index 7, which is out of bounds.\n", errout.str()); + + check("void f() {\n" + " int* z = new int{};\n" + " for (int n = 0; n < 8; ++n)\n" + " z[n] = 0;\n" + " delete[] z;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'z[1]' accessed at index 7, which is out of bounds.\n", errout.str()); + + check("void f() {\n" + " int* z = new int[5];\n" + " for (int n = 0; n < 8; ++n)\n" + " z[n] = 0;\n" + " delete[] z;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'z[5]' accessed at index 7, which is out of bounds.\n", errout.str()); + + check("void g() {\n" + " int* z = new int[5]();\n" + " for (int n = 0; n < 8; ++n)\n" + " z[n] = 1;\n" + " delete[] z;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'z[5]' accessed at index 7, which is out of bounds.\n", errout.str()); + + check("void h() {\n" + " int** z = new int* [5];\n" + " for (int n = 0; n < 8; ++n)\n" + " z[n] = nullptr;\n" + " delete[] z;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'z[5]' accessed at index 7, which is out of bounds.\n", errout.str()); + + check("void h() {\n" + " int** z = new int* [5]();\n" + " for (int n = 0; n < 8; ++n)\n" + " z[n] = nullptr;\n" + " delete[] z;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'z[5]' accessed at index 7, which is out of bounds.\n", errout.str()); + + check("void h() {\n" + " int** z = new int* [5]{};\n" + " for (int n = 0; n < 8; ++n)\n" + " z[n] = nullptr;\n" + " delete[] z;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 'z[5]' accessed at index 7, which is out of bounds.\n", errout.str()); + } + void buffer_overrun_2_struct() { check("struct ABC\n" "{\n" @@ -3405,7 +3472,7 @@ private: " char *s; s = new char[10];\n" " s[10] = 0;\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Array 's[10]' accessed at index 10, which is out of bounds.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 's[10]' accessed at index 10, which is out of bounds.\n", errout.str()); // ticket #1670 - false negative when using return check("char f()\n" @@ -3413,7 +3480,7 @@ private: " int *s; s = new int[10];\n" " return s[10];\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Array 's[10]' accessed at index 10, which is out of bounds.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (error) Array 's[10]' accessed at index 10, which is out of bounds.\n", errout.str()); check("struct Fred { char c[10]; };\n" "char f()\n" @@ -3446,7 +3513,7 @@ private: " buf[9] = 0;\n" " delete [] buf;\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:6]: (error) Array 'buf[9]' accessed at index 9, which is out of bounds.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:6]: (error) Array 'buf[9]' accessed at index 9, which is out of bounds.\n", errout.str()); check("void foo()\n" "{\n" @@ -3454,7 +3521,7 @@ private: " char *s; s = new char[Size];\n" " s[Size] = 0;\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:5]: (error) Array 's[10]' accessed at index 10, which is out of bounds.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (error) Array 's[10]' accessed at index 10, which is out of bounds.\n", errout.str()); check("void foo()\n" "{\n" @@ -3462,7 +3529,7 @@ private: " E *e; e = new E[10];\n" " e[10] = ZERO;\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:5]: (error) Array 'e[10]' accessed at index 10, which is out of bounds.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (error) Array 'e[10]' accessed at index 10, which is out of bounds.\n", errout.str()); } // data is allocated with malloc @@ -3557,7 +3624,7 @@ private: " for (int i = 0; i < 3; i++)\n" " a[i] = NULL;\n" "}"); - TODO_ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (error) Array 'a[2]' accessed at index 2, which is out of bounds.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (error) Array 'a[2]' accessed at index 2, which is out of bounds.\n", errout.str()); } // statically allocated buffer @@ -3930,7 +3997,7 @@ private: " char *str = new char[5];\n" " mysprintf(str, \"abcde\");\n" "}", settings); - TODO_ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds.\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (error) Buffer is accessed out of bounds: str\n", errout.str()); check("void f(int condition) {\n" " char str[5];\n"