Fixed #818 (Detect sprintf buffer overrun with struct members)

lib/checkbufferoverrun.cpp

@@ -671,38 +671,38 @@ void CheckBufferOverrun::checkScope(const Token *tok, const std::vector<std::str
 
 
         // Detect few strcat() calls
-        if (varid > 0 && Token::Match(tok, "strcat ( %varid% , %str% ) ;", varid))
+        const std::string strcatPattern = varid > 0 ? "strcat ( %varid% , %str% ) ;" : ("strcat ( " + varnames + " , %str% ) ;");
+        if (Token::Match(tok, strcatPattern.c_str(), varid))
         {
             size_t charactersAppend = 0;
             const Token *tok2 = tok;
 
-            while (tok2 && Token::Match(tok2, "strcat ( %varid% , %str% ) ;", varid))
+            while (tok2 && Token::Match(tok2, strcatPattern.c_str(), varid))
             {
-                charactersAppend += Token::getStrLength(tok2->tokAt(4));
+                charactersAppend += Token::getStrLength(tok2->tokAt(4 + varc));
                 if (charactersAppend >= static_cast<size_t>(total_size))
                 {
                     bufferOverrun(tok2);
                     break;
                 }
-                tok2 = tok2->tokAt(7);
+                tok2 = tok2->tokAt(7 + varc);
             }
         }
 
-        // sprintf / snprintf..
+        // sprintf..
-        if (varid > 0)
+        const std::string sprintfPattern = varid > 0 ? "sprintf ( %varid% , %str% [,)]" : ("sprintf ( " + varnames + " , %str% [,)]");
+        if (Token::Match(tok, sprintfPattern.c_str(), varid))
         {
-            if (Token::Match(tok, "sprintf ( %varid% , %str% [,)]", varid))
+            checkSprintfCall(tok, total_size);
-            {
+        }
-                checkSprintfCall(tok, total_size);
-            }
 
-            // snprintf..
+        // snprintf..
-            if (Token::Match(tok, "snprintf ( %varid% , %num% ,", varid))
+        const std::string snprintfPattern = varid > 0 ? "snprintf ( %varid% , %num% ," : ("snprintf ( " + varnames + " , %num% ,");
-            {
+        if (Token::Match(tok, snprintfPattern.c_str(), varid))
-                int n = MathLib::toLongNumber(tok->strAt(4));
+        {
-                if (n > total_size)
+            int n = MathLib::toLongNumber(tok->strAt(4 + varc));
-                    outOfBounds(tok->tokAt(4), "snprintf size");
+            if (n > total_size)
-            }
+                outOfBounds(tok->tokAt(4 + varc), "snprintf size");
         }
 
         // Function calls not handled
@@ -1344,14 +1344,23 @@ int CheckBufferOverrun::countSprintfLength(const std::string &input_string, cons
     return input_string_size;
 }
 
-
 void CheckBufferOverrun::checkSprintfCall(const Token *tok, int size)
 {
-    std::list<const Token*> parameters;
+    const Token *end = tok->next()->link();
-    if (tok->tokAt(5)->str() == ",")
+        
+    // Count the number of tokens in the buffer variable's name
+    int varc = 0;
+    for (const Token *tok1 = tok->tokAt(3); tok1 != end; tok1 = tok1->next())
     {
-        const Token *end = tok->next()->link();
+        if (tok1->str() == ",")
-        for (const Token *tok2 = tok->tokAt(5); tok2 && tok2 != end; tok2 = tok2->next())
+            break;
+        ++ varc;
+    }
+    
+    std::list<const Token*> parameters;
+    if (tok->tokAt(5 + varc)->str() == ",")
+    {
+        for (const Token *tok2 = tok->tokAt(5 + varc); tok2 && tok2 != end; tok2 = tok2->next())
         {
             if (Token::Match(tok2, ", %any% [,)]"))
             {
@@ -1396,7 +1405,7 @@ void CheckBufferOverrun::checkSprintfCall(const Token *tok, int size)
         }
     }
 
-    int len = countSprintfLength(tok->tokAt(4)->strValue(), parameters);
+    int len = countSprintfLength(tok->tokAt(4 + varc)->strValue(), parameters);
     if (len > size)
     {
         bufferOverrun(tok);

test/testbufferoverrun.cpp

@@ -130,14 +130,22 @@ private:
         TEST_CASE(sprintf4);
         TEST_CASE(sprintf5);
         TEST_CASE(sprintf6);
+        TEST_CASE(sprintf7);
+        TEST_CASE(sprintf8);
 
         TEST_CASE(snprintf1);
         TEST_CASE(snprintf2);
         TEST_CASE(snprintf3);
         TEST_CASE(snprintf4);
+        TEST_CASE(snprintf5);
+        TEST_CASE(snprintf6);
 
         TEST_CASE(strncat1);
         TEST_CASE(strncat2);
+        TEST_CASE(strncat3);
+        
+        TEST_CASE(strcat1);
+        TEST_CASE(strcat2);
 
         TEST_CASE(memfunc);		// memchr/memset/memcpy
 
@@ -1677,6 +1685,28 @@ private:
         ASSERT_EQUALS("", errout.str());	// catch changes
         TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Buffer access out-of-bounds\n", errout.str());
     }
+    
+    void sprintf7()
+    {
+        check("struct Foo { char a[1]; };\n"
+              "void f()\n"
+              "{\n"
+              "  struct Foo x;\n"
+              "  sprintf(x.a, \"aa\");\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Buffer access out-of-bounds\n", errout.str());
+    }
+    
+    void sprintf8()
+    {
+        check("struct Foo { char a[3]; };\n"
+              "void f()\n"
+              "{\n"
+              "  struct Foo x;\n"
+              "  sprintf(x.a, \"aa\");\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
 
     void snprintf1()
     {
@@ -1718,8 +1748,27 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
-
+    void snprintf5()
-
+    {
+        check("struct Foo { char a[1]; };\n"
+              "void f()\n"
+              "{\n"
+              "  struct Foo x;\n"
+              "  snprintf(x.a, 2, \"aa\");\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (error) snprintf size is out of bounds\n", errout.str());
+    }
+    
+    void snprintf6()
+    {
+        check("struct Foo { char a[3]; };\n"
+              "void f()\n"
+              "{\n"
+              "  struct Foo x;\n"
+              "  snprintf(x.a, 2, \"aa\");\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
 
     void strncat1()
     {
@@ -1742,6 +1791,41 @@ private:
         ASSERT_EQUALS("[test.cpp:4]: (style) Dangerous usage of strncat. Tip: the 3rd parameter means maximum number of characters to append\n", errout.str());
     }
 
+    void strncat3()
+    {
+        check("struct Foo { char a[4]; };\n"
+              "void f(char *a)\n"
+              "{\n"
+              "  struct Foo x;\n"
+              "  strncat(x.a, a, 5);\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Buffer access out-of-bounds: x.a\n", errout.str());
+    }
+
+
+    void strcat1()
+    {
+        check("struct Foo { char a[4]; };\n"
+              "void f()\n"
+              "{\n"
+              "  struct Foo x;\n"
+              "  strcat(x.a, \"aa\");\n"
+              "  strcat(x.a, \"aa\");\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:6]: (error) Buffer access out-of-bounds\n", errout.str());
+    }
+    
+    void strcat2()
+    {
+        check("struct Foo { char a[5]; };\n"
+              "void f()\n"
+              "{\n"
+              "  struct Foo x;\n"
+              "  strcat(x.a, \"aa\");\n"
+              "  strcat(x.a, \"aa\");\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
 
 
     // memchr/memset/memcpy/etc