diff --git a/lib/checkbufferoverrun.cpp b/lib/checkbufferoverrun.cpp index 17359533a..3ec9af3c9 100644 --- a/lib/checkbufferoverrun.cpp +++ b/lib/checkbufferoverrun.cpp @@ -1069,3 +1069,65 @@ void CheckBufferOverrun::objectIndexError(const Token *tok, const ValueFlow::Val CWE758, Certainty::normal); } + +static bool isVLAIndex(const Token* tok) +{ + if (!tok) + return false; + if (tok->varId() != 0U) + return true; + if (tok->str() == "?") { + // this is a VLA index if both expressions around the ":" is VLA index + if (tok->astOperand2() && + tok->astOperand2()->str() == ":" && + isVLAIndex(tok->astOperand2()->astOperand1()) && + isVLAIndex(tok->astOperand2()->astOperand2())) + return true; + return false; + } + return isVLAIndex(tok->astOperand1()) || isVLAIndex(tok->astOperand2()); +} + +void CheckBufferOverrun::negativeArraySize() +{ + const SymbolDatabase* symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Variable* var : symbolDatabase->variableList()) { + if (!var || !var->isArray()) + continue; + const Token* const nameToken = var->nameToken(); + if (!Token::Match(nameToken, "%var% [") || !nameToken->next()->astOperand2()) + continue; + const ValueFlow::Value* sz = nameToken->next()->astOperand2()->getValueLE(-1, mSettings); + // don't warn about constant negative index because that is a compiler error + if (sz && isVLAIndex(nameToken->next()->astOperand2())) + negativeArraySizeError(nameToken); + } + + for (const Scope* functionScope : symbolDatabase->functionScopes) { + for (const Token* tok = functionScope->bodyStart; tok != functionScope->bodyEnd; tok = tok->next()) { + if (!tok->isKeyword() || tok->str() != "new" || !tok->astOperand1() || tok->astOperand1()->str() != "[") + continue; + const Token* valOperand = tok->astOperand1()->astOperand2(); + if (!valOperand) + continue; + const ValueFlow::Value* sz = valOperand->getValueLE(-1, mSettings); + if (sz) + negativeMemoryAllocationSizeError(tok); + } + } +} + +void CheckBufferOverrun::negativeArraySizeError(const Token* tok) +{ + const std::string arrayName = tok ? tok->expressionString() : std::string(); + const std::string line1 = arrayName.empty() ? std::string() : ("$symbol:" + arrayName + '\n'); + reportError(tok, Severity::error, "negativeArraySize", + line1 + + "Declaration of array '" + arrayName + "' with negative size is undefined behaviour", CWE758, Certainty::safe); +} + +void CheckBufferOverrun::negativeMemoryAllocationSizeError(const Token* tok) +{ + reportError(tok, Severity::error, "negativeMemoryAllocationSize", + "Memory allocation size is negative.", CWE131, Certainty::safe); +} diff --git a/lib/checkbufferoverrun.h b/lib/checkbufferoverrun.h index 7e9c8b557..900d7aa31 100644 --- a/lib/checkbufferoverrun.h +++ b/lib/checkbufferoverrun.h @@ -75,6 +75,7 @@ public: checkBufferOverrun.stringNotZeroTerminated(); checkBufferOverrun.objectIndex(); checkBufferOverrun.argumentSize(); + checkBufferOverrun.negativeArraySize(); } void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const override { @@ -86,6 +87,9 @@ public: c.bufferOverflowError(nullptr, nullptr, Certainty::normal); c.objectIndexError(nullptr, nullptr, true); c.argumentSizeError(nullptr, "function", 1, "buffer", nullptr, nullptr); + c.negativeMemoryAllocationSizeError(nullptr); + c.negativeArraySizeError(nullptr); + c.negativeMemoryAllocationSizeError(nullptr); } /** @brief Parse current TU and extract file info */ @@ -119,6 +123,10 @@ private: void argumentSize(); void argumentSizeError(const Token *tok, const std::string &functionName, nonneg int paramIndex, const std::string ¶mExpression, const Variable *paramVar, const Variable *functionArg); + void negativeArraySize(); + void negativeArraySizeError(const Token* tok); + void negativeMemoryAllocationSizeError(const Token* tok); // provide a negative value to memory allocation function + void objectIndex(); void objectIndexError(const Token *tok, const ValueFlow::Value *v, bool known); @@ -159,7 +167,8 @@ private: "- Dangerous usage of strncat()\n" "- Using array index before checking it\n" "- Partial string write that leads to buffer that is not zero terminated.\n" - "- Check for large enough arrays being passed to functions\n"; + "- Check for large enough arrays being passed to functions\n" + "- Allocating memory with a negative size\n"; } }; /// @} diff --git a/releasenotes.txt b/releasenotes.txt index 807afe900..1b6b6706b 100644 --- a/releasenotes.txt +++ b/releasenotes.txt @@ -1,2 +1,3 @@ release notes for cppcheck-2.9 +- restored check for negative allocation (new[]) and negative VLA sizes from cppcheck 1.87 (LCppC backport) \ No newline at end of file diff --git a/test/testbufferoverrun.cpp b/test/testbufferoverrun.cpp index e793354bd..0a7d07548 100644 --- a/test/testbufferoverrun.cpp +++ b/test/testbufferoverrun.cpp @@ -4781,6 +4781,14 @@ private: } void negativeMemoryAllocationSizeError() { // #389 + check("void f()\n" + "{\n" + " int *a;\n" + " a = new int[-1];\n" + " delete [] a;\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) Memory allocation size is negative.\n", errout.str()); + check("void f()\n" "{\n" " int *a;\n" @@ -4810,7 +4818,7 @@ private: " int a[sz];\n" "}\n" "void x() { f(-100); }"); - TODO_ASSERT_EQUALS("[test.cpp:2]: (error) Declaration of array 'a' with negative size is undefined behaviour\n", "", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (error) Declaration of array 'a' with negative size is undefined behaviour\n", errout.str()); // don't warn for constant sizes -> this is a compiler error so this is used for static assertions for instance check("int x, y;\n" diff --git a/test/testfunctions.cpp b/test/testfunctions.cpp index 8085439ed..e95c0fc8c 100644 --- a/test/testfunctions.cpp +++ b/test/testfunctions.cpp @@ -97,6 +97,8 @@ private: TEST_CASE(returnLocalStdMove4); TEST_CASE(returnLocalStdMove5); + + TEST_CASE(negativeMemoryAllocationSizeError); // #389 } #define check(...) check_(__FILE__, __LINE__, __VA_ARGS__) @@ -1706,6 +1708,23 @@ private: "A f2() { volatile A var; return std::move(var); }"); ASSERT_EQUALS("", errout.str()); } + + void negativeMemoryAllocationSizeError() { // #389 + check("void f() {\n" + " int *a;\n" + " a = malloc( -10 );\n" + " free(a);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Invalid malloc() argument nr 1. The value is -10 but the valid values are '0:'.\n", errout.str()); + + check("void f() {\n" + " int *a;\n" + " a = alloca( -10 );\n" + " free(a);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Obsolete function 'alloca' called.\n" + "[test.cpp:3]: (error) Invalid alloca() argument nr 1. The value is -10 but the valid values are '0:'.\n", errout.str()); + } }; REGISTER_TEST(TestFunctions)