Fixed #1952 (false negative: buffer acces out of bounds with memcpy)

lib/checkbufferoverrun.cpp

@@ -536,7 +536,7 @@ void CheckBufferOverrun::parse_for_body(const Token *tok2, const ArrayInfo &arra
 
 
 
-void CheckBufferOverrun::checkFunctionCall(const Token &tok, unsigned int par, const ArrayInfo &arrayInfo)
+void CheckBufferOverrun::checkFunctionParameter(const Token &tok, unsigned int par, const ArrayInfo &arrayInfo)
 {
     // total_size : which parameter in function call takes the total size?
     std::map<std::string, unsigned int> total_size;
@@ -721,6 +721,42 @@ void CheckBufferOverrun::checkFunctionCall(const Token &tok, unsigned int par, c
 }
 
 
+void CheckBufferOverrun::checkFunctionCall(const Token *tok, const ArrayInfo &arrayInfo)
+{
+
+    // 1st parameter..
+    if (Token::Match(tok->tokAt(2), "%varid% ,|)", arrayInfo.varid))
+        checkFunctionParameter(*tok, 1, arrayInfo);
+    else if (Token::Match(tok->tokAt(2), "%varid% + %num% ,|)", arrayInfo.varid))
+    {
+        const ArrayInfo ai(arrayInfo.limit(MathLib::toLongNumber(tok->strAt(4))));
+        checkFunctionParameter(*tok, 1, ai);
+    }
+
+    // goto 2nd parameter and check it..
+    for (const Token *tok2 = tok->tokAt(2); tok2; tok2 = tok2->next())
+    {
+        if (tok2->str() == "(")
+        {
+            tok2 = tok2->link();
+            continue;
+        }
+        if (tok2->str() == ";" || tok2->str() == ")")
+            break;
+        if (tok2->str() == ",")
+        {
+            if (Token::Match(tok2, ", %varid% ,|)", arrayInfo.varid))
+                checkFunctionParameter(*tok, 2, arrayInfo);
+            else if (Token::Match(tok2, ", %varid% + %num% ,|)", arrayInfo.varid))
+            {
+                const ArrayInfo ai(arrayInfo.limit(MathLib::toLongNumber(tok2->strAt(3))));
+                checkFunctionParameter(*tok, 2, ai);
+            }
+            break;
+        }
+    }
+}
+
 
 void CheckBufferOverrun::checkScope(const Token *tok, const std::vector<std::string> &varname, const MathLib::bigint size, const MathLib::bigint total_size, unsigned int varid)
 {
@@ -827,9 +863,9 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector<std::str
                                 (unsigned int)(total_size / size),
                                 (unsigned int)size);
             if (Token::Match(tok, ("%var% ( " + varnames + " ,").c_str()))
-                checkFunctionCall(*tok, 1, arrayInfo);
+                checkFunctionParameter(*tok, 1, arrayInfo);
             if (Token::Match(tok, ("%var% ( %var% , " + varnames + " ,").c_str()))
-                checkFunctionCall(*tok, 2, arrayInfo);
+                checkFunctionParameter(*tok, 2, arrayInfo);
         }
 
         // Loop..
@@ -934,10 +970,15 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector<std::str
                 outOfBounds(tok->tokAt(4 + varc), "snprintf size");
         }
 
-        // Function calls not handled
+        // Check function call..
         if (Token::Match(tok, "%var% ("))
         {
-            continue;
+            // No varid => function calls are not handled
+            if (varid == 0)
+                continue;
+
+            const ArrayInfo arrayInfo(varid, varnames, size, total_size / size);
+            checkFunctionCall(tok, arrayInfo);
         }
 
         // undefined behaviour: result of pointer arithmetic is out of bounds
@@ -1081,37 +1122,7 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo
         // Check function call..
         if (Token::Match(tok, "%var% ("))
         {
-            // 1st parameter..
+            checkFunctionCall(tok, arrayInfo);
-            if (Token::Match(tok->tokAt(2), "%varid% ,|)", arrayInfo.varid))
-                checkFunctionCall(*tok, 1, arrayInfo);
-            else if (Token::Match(tok->tokAt(2), "%varid% + %num% ,|)", arrayInfo.varid))
-            {
-                const ArrayInfo ai(arrayInfo.limit(MathLib::toLongNumber(tok->strAt(4))));
-                checkFunctionCall(*tok, 1, ai);
-            }
-
-            // goto 2nd parameter and check it..
-            for (const Token *tok2 = tok->tokAt(2); tok2; tok2 = tok2->next())
-            {
-                if (tok2->str() == "(")
-                {
-                    tok2 = tok2->link();
-                    continue;
-                }
-                if (tok2->str() == ";" || tok2->str() == ")")
-                    break;
-                if (tok2->str() == ",")
-                {
-                    if (Token::Match(tok2, ", %varid% ,|)", arrayInfo.varid))
-                        checkFunctionCall(*tok, 2, arrayInfo);
-                    else if (Token::Match(tok2, ", %varid% + %num% ,|)", arrayInfo.varid))
-                    {
-                        const ArrayInfo ai(arrayInfo.limit(MathLib::toLongNumber(tok2->strAt(3))));
-                        checkFunctionCall(*tok, 2, ai);
-                    }
-                    break;
-                }
-            }
         }
 
         if (_settings->_checkCodingStyle)

lib/checkbufferoverrun.h

@@ -169,12 +169,19 @@ public:
     void parse_for_body(const Token *tok2, const ArrayInfo &arrayInfo, const std::string &strindex, bool condition_out_of_bounds, unsigned int counter_varid, const std::string &min_counter_value, const std::string &max_counter_value);
 
     /**
-     * Helper function for checkScope - check a function call
+     * Helper function for checkFunctionCall - check a function parameter
      * \param tok token for the function name
      * \param par on what parameter is the array used
      * \param arrayInfo the array information
      */
-    void checkFunctionCall(const Token &tok, const unsigned int par, const ArrayInfo &arrayInfo);
+    void checkFunctionParameter(const Token &tok, const unsigned int par, const ArrayInfo &arrayInfo);
+
+    /**
+     * Helper function that checks if the array is used and if so calls the checkFunctionCall
+     * @param tok token that matches "%var% ("
+     * @param arrayInfo the array information
+     */
+    void checkFunctionCall(const Token *tok, const ArrayInfo &arrayInfo);
 
     void arrayIndexOutOfBounds(const Token *tok, MathLib::bigint size, MathLib::bigint index);
     void arrayIndexOutOfBounds(const Token *tok, const ArrayInfo &arrayInfo, const std::vector<MathLib::bigint> &index);

test/testbufferoverrun.cpp

@@ -180,6 +180,7 @@ private:
         TEST_CASE(alloc1);    // Buffer allocated with new
         TEST_CASE(alloc2);    // Buffer allocated with malloc
         TEST_CASE(alloc3);    // statically allocated buffer
+        TEST_CASE(malloc_memset);	// using memset on buffer allocated with malloc
 
         TEST_CASE(memset1);
         TEST_CASE(memset2);
@@ -2381,6 +2382,15 @@ private:
         ASSERT_EQUALS("[test.cpp:4]: (error) Array 's[1]' index 10 out of bounds\n", errout.str());
     }
 
+    void malloc_memset()
+    {
+        check("void f() {\n"
+              "    char *p = malloc(10);\n"
+              "    memset(p,0,100);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Buffer access out-of-bounds\n", errout.str());
+    }
+
     void memset1()
     {
         check("void foo()\n"