Fixed #168 (buffer overflow: not enough room for the null terminator)

lib/checkbufferoverrun.cpp

@@ -1228,6 +1228,7 @@ void CheckBufferOverrun::bufferOverrun()
 {
     checkGlobalAndLocalVariable();
     checkStructVariable();
+    checkBufferAllocatedWithStrlen();
 }
 //---------------------------------------------------------------------------
 
@@ -1403,6 +1404,85 @@ void CheckBufferOverrun::checkSprintfCall(const Token *tok, int size)
 }
 
 
+
+//---------------------------------------------------------------------------
+// Checking for allocating insufficient memory for copying a string by
+// allocating only strlen(src) bytes instead of strlen(src) + 1 bytes (one
+// extra for the terminating null character).
+// Example:
+//   char *b = malloc(strlen(a));   // Should be malloc(strlen(a) + 1);
+//   strcpy(b, a);                  // <== Buffer overrun
+//---------------------------------------------------------------------------
+void CheckBufferOverrun::checkBufferAllocatedWithStrlen()
+{
+    const char pattern[] = "%var% = new|malloc|g_malloc|g_try_malloc|realloc|g_realloc|g_try_realloc";
+    for (const Token *tok = Token::findmatch(_tokenizer->tokens(), pattern); tok; tok = Token::findmatch(tok->next(),pattern))
+    {
+        unsigned int dstVarId;
+        unsigned int srcVarId;
+              
+        // Look for allocation of a buffer based on the size of a string
+        if (Token::Match(tok, "%var% = malloc|g_malloc|g_try_malloc ( strlen ( %var% ) )"))
+        {
+            dstVarId = tok->varId();
+            srcVarId = tok->tokAt(6)->varId();
+            tok      = tok->tokAt(8);
+        }
+        else if (Token::Match(tok, "%var% = new char [ strlen ( %var% ) ]"))
+        {
+            dstVarId = tok->varId();
+            srcVarId = tok->tokAt(7)->varId();
+            tok      = tok->tokAt(9);
+        }
+        else if (Token::Match(tok, "%var% = realloc|g_realloc|g_try_realloc ( %var% , strlen ( %var% ) )"))
+        {
+            dstVarId = tok->varId();
+            srcVarId = tok->tokAt(8)->varId();
+            tok      = tok->tokAt(10);
+        }
+        else
+            continue;
+        
+        int indentlevel = 0;
+        for (; tok && tok->next(); tok = tok->next())
+        {
+            // To avoid false positives and added complexity, we will only look for
+            // improper usage of the buffer within the block that it was allocated
+            if (tok->str() == "{")
+            {
+                ++indentlevel;
+            }
+
+            else if (tok->str() == "}")
+            {
+                --indentlevel;
+                if (indentlevel < 0)
+                    return;
+            }
+                       
+            // If the buffers are modified, we can't be sure of their sizes
+            if (tok->varId() == srcVarId || tok->varId() == dstVarId)
+                break;
+            
+            if (Token::Match(tok, "strcpy ( %varid% , %var% )", dstVarId) &&
+                tok->tokAt(4)->varId() == srcVarId)
+            {
+                bufferOverrun(tok);
+            }
+            else if (Token::Match(tok, "sprintf ( %varid% , %str% , %var% )", dstVarId) &&
+                tok->tokAt(6)->varId() == srcVarId &&
+                tok->tokAt(4)->str().find("%s") != std::string::npos)
+            {
+                bufferOverrun(tok);
+            }
+                
+        }
+        
+    }
+}
+//---------------------------------------------------------------------------
+
+
 void CheckBufferOverrun::negativeIndexError(const Token *tok, long index)
 {
     std::ostringstream ostr;

lib/checkbufferoverrun.h

@@ -95,6 +95,9 @@ public:
     /** Check for buffer overruns - locate global variables and local function variables and check them with the checkScope function */
     void checkGlobalAndLocalVariable();
 
+    /** Check for buffer overruns due to allocating strlen(src) bytes instead of (strlen(src)+1) bytes before copying a string */
+    void checkBufferAllocatedWithStrlen();
+
     /** Check for negative index */
     void negativeIndex();
 

test/testbufferoverrun.cpp

@@ -122,6 +122,7 @@ private:
         TEST_CASE(buffer_overrun_11);
         TEST_CASE(buffer_overrun_12);
         TEST_CASE(buffer_overrun_13);
+        TEST_CASE(buffer_overrun_14);
 
         TEST_CASE(sprintf1);
         TEST_CASE(sprintf2);
@@ -1521,6 +1522,83 @@ private:
         ASSERT_EQUALS("[test.cpp:3]: (error) Buffer access out-of-bounds: a\n", errout.str());
     }
 
+    void buffer_overrun_14()
+    {
+        check("void f(char *a) {\n"
+              "  char *b = new char[strlen(a)];\n"
+              "  strcpy(b, a);\n"
+              "  return b;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Buffer access out-of-bounds\n", errout.str());
+        
+        check("void f(char *a) {\n"
+              "  char *b = new char[strlen(a) + 1];\n"
+              "  strcpy(b, a);\n"
+              "  return b;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+        
+        check("void f(char *a) {\n"
+              "  char *b = new char[strlen(a)];\n"
+              "  a[0] = '\\0';\n"
+              "  strcpy(b, a);\n"
+              "  return b;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+        
+        check("void f(char *a) {\n"
+              "  char *b = malloc(strlen(a));\n"
+              "  b = realloc(b, 10000);\n"
+              "  strcpy(b, a);\n"
+              "  return b;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f(char *a) {\n"
+              "  char *b = malloc(strlen(a));\n"
+              "  strcpy(b, a);\n"
+              "  return b;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Buffer access out-of-bounds\n", errout.str());
+        
+        check("void f(char *a) {\n"
+              "  char *b = malloc(strlen(a));\n"
+              "  if (1) {\n"
+              "    strcpy(b, a);\n"
+              "  }\n"
+              "  return b;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (error) Buffer access out-of-bounds\n", errout.str());
+              
+        check("void f(char *a) {\n"
+              "  char *b = malloc(strlen(a) + 1);\n"
+              "  strcpy(b, a);\n"
+              "  return b;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+        
+        check("void f(char *a, char *c) {\n"
+              "  char *b = realloc(c, strlen(a));\n"
+              "  strcpy(b, a);\n"
+              "  return b;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Buffer access out-of-bounds\n", errout.str());
+        
+        check("void f(char *a, char *c) {\n"
+              "  char *b = realloc(c, strlen(a) + 1);\n"
+              "  strcpy(b, a);\n"
+              "  return b;\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+        
+        check("void f(char *a) {\n"
+              "  char *b = malloc(strlen(a));\n"
+              "  sprintf(b, \"%s\", a);\n"
+              "  return b;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:3]: (error) Buffer access out-of-bounds\n", errout.str());
+    }
+
     void sprintf1()
     {
         check("void f()\n"