LCppC backport: Restored Check: Detect negative VLA and allocation (new[]) sizes (#4187)

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);
+}

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 &paramExpression, 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";
     }
 };
 /// @}

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)

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"

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)