warn when buffer is not zero terminated after memmove

lib/checkbufferoverrun.cpp

@@ -155,22 +155,15 @@ void CheckBufferOverrun::sizeArgumentAsCharError(const Token *tok)
 }
 
 
-void CheckBufferOverrun::terminateStrncpyError(const Token *tok, const std::string &varname, bool conclusive)
+void CheckBufferOverrun::terminateStrncpyError(const Token *tok, const std::string &varname)
 {
-    if (conclusive)
+    reportError(tok, Severity::warning, "terminateStrncpy",
-        reportError(tok, Severity::warning, "terminateStrncpy",
+                "The buffer '" + varname + "' may not be zero-terminated after the call to strncpy().\n"
-                    "The buffer '" + varname + "' is not zero-terminated after the call to strncpy().\n"
+                "The use of strncpy() usually indicates that the programmer wants to ensure "
-                    "The use of strncpy() usually indicates that the programmer wants to ensure "
+                "the buffer is zero-terminated after the call. However if the (buffer) size given for "
-                    "the buffer is zero-terminated after the call. This will cause bugs later in the code if "
+                "the strncpy() call matches the actual buffer size strncpy() does not add the "
-                    "the code assumes buffer is zero-terminated.");
+                "zero at the end of the buffer. This may cause bugs later in the code if "
-    else
+                "the code assumes buffer is zero-terminated.");
-        reportError(tok, Severity::warning, "terminateStrncpy",
-                    "The buffer '" + varname + "' may not be zero-terminated after the call to strncpy().\n"
-                    "The use of strncpy() usually indicates that the programmer wants to ensure "
-                    "the buffer is zero-terminated after the call. However if the (buffer) size given for "
-                    "the strncpy() call matches the actual buffer size strncpy() does not add the "
-                    "zero at the end of the buffer. This may cause bugs later in the code if "
-                    "the code assumes buffer is zero-terminated.");
 }
 
 void CheckBufferOverrun::cmdLineArgsError(const Token *tok)
@@ -178,10 +171,10 @@ void CheckBufferOverrun::cmdLineArgsError(const Token *tok)
     reportError(tok, Severity::error, "insecureCmdLineArgs", "Buffer overrun possible for long cmd-line args");
 }
 
-void CheckBufferOverrun::terminateMemcpyError(const Token *tok, const std::string &varname)
+void CheckBufferOverrun::bufferNotZeroTerminatedError(const Token *tok, const std::string &varname, const std::string &function)
 {
-    reportError(tok, Severity::warning, "terminateMemcpy",
+    reportError(tok, Severity::warning, "bufferNotZeroTerminated",
-                "The buffer '" + varname + "' is not zero-terminated after the call to memcpy().\n"
+                "The buffer '" + varname + "' is not zero-terminated after the call to " + function + "().\n"
                 "This will cause bugs later in the code if the code assumes buffer is zero-terminated.");
 }
 
@@ -1234,18 +1227,13 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo
             checkFunctionCall(tok, arrayInfo);
         }
 
-        if (Token::Match(tok, "strncpy|memcpy ( %varid% , %str% , %num% )", arrayInfo.varid()))
+        if (Token::Match(tok, "strncpy|memcpy|memmove ( %varid% , %str% , %num% )", arrayInfo.varid()))
         {
             unsigned int num = (unsigned int)MathLib::toLongNumber(tok->strAt(6));
             if (Token::getStrLength(tok->tokAt(4)) >= total_size && total_size == num)
             {
                 if (_settings->inconclusive)
-                {
+                    bufferNotZeroTerminatedError(tok, tok->strAt(2), tok->str());
-                    if (tok->str() == "strncpy")
-                        terminateStrncpyError(tok, tok->strAt(2), true);
-                    else
-                        terminateMemcpyError(tok, tok->strAt(2));
-                }
             }
         }
 
@@ -1271,7 +1259,7 @@ void CheckBufferOverrun::checkScope(const Token *tok, const ArrayInfo &arrayInfo
                             {
                                 // this is currently 'inconclusive'. See TestBufferOverrun::terminateStrncpy3
                                 if (_settings->isEnabled("style") && _settings->inconclusive)
-                                    terminateStrncpyError(tok, tok->strAt(2), false);
+                                    terminateStrncpyError(tok, tok->strAt(2));
                             }
 
                             break;

lib/checkbufferoverrun.h

@@ -216,8 +216,8 @@ public:
     void strncatUsageError(const Token *tok);
     void outOfBoundsError(const Token *tok, const std::string &what);
     void sizeArgumentAsCharError(const Token *tok);
-    void terminateStrncpyError(const Token *tok, const std::string &varname, bool conclusive);
+    void terminateStrncpyError(const Token *tok, const std::string &varname);
-    void terminateMemcpyError(const Token *tok, const std::string &varname);
+    void bufferNotZeroTerminatedError(const Token *tok, const std::string &varname, const std::string &function);
     void negativeIndexError(const Token *tok, MathLib::bigint index);
     void cmdLineArgsError(const Token *tok);
     void pointerOutOfBoundsError(const Token *tok, const std::string &object);	// UB when result of calculation is out of bounds
@@ -232,8 +232,8 @@ public:
         c.strncatUsageError(0);
         c.outOfBoundsError(0, "index");
         c.sizeArgumentAsCharError(0);
-        c.terminateStrncpyError(0, "buffer", false);
+        c.terminateStrncpyError(0, "buffer");
-        c.terminateMemcpyError(0, "buffer");
+        c.bufferNotZeroTerminatedError(0, "buffer", "strncpy");
         c.negativeIndexError(0, -1);
         c.cmdLineArgsError(0);
         c.pointerOutOfBoundsError(0, "array");

test/testbufferoverrun.cpp

@@ -231,6 +231,8 @@ private:
 
         // Access array and then check if the used index is within bounds
         TEST_CASE(arrayIndexThenCheck);
+
+        TEST_CASE(bufferNotZeroTerminated);
     }
 
 
@@ -2583,13 +2585,6 @@ private:
               "    memchr(a, b, 10);\n"
               "}\n");
         TODO_ASSERT_EQUALS("[test.cpp:4]: (error) Buffer access out-of-bounds\n", "", errout.str());
-
-        check("void f()\n"
-              "{\n"
-              "    char c[6];\n"
-              "    memcpy(c,\"hello!\",sizeof(c));\n"
-              "}\n");
-        ASSERT_EQUALS("[test.cpp:4]: (warning) The buffer 'c' is not zero-terminated after the call to memcpy().\n", errout.str());
     }
 
     // ticket #2121 - buffer access out of bounds when using uint32_t
@@ -3315,6 +3310,30 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:2]: (style) Array index i is used before limits check\n", errout.str());
     }
+
+    void bufferNotZeroTerminated()
+    {
+        check("void f()\n"
+              "{\n"
+              "    char c[6];\n"
+              "    strncpy(c,\"hello!\",sizeof(c));\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (warning) The buffer 'c' is not zero-terminated after the call to strncpy().\n", errout.str());
+
+        check("void f()\n"
+              "{\n"
+              "    char c[6];\n"
+              "    memcpy(c,\"hello!\",sizeof(c));\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (warning) The buffer 'c' is not zero-terminated after the call to memcpy().\n", errout.str());
+
+        check("void f()\n"
+              "{\n"
+              "    char c[6];\n"
+              "    memmove(c,\"hello!\",sizeof(c));\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:4]: (warning) The buffer 'c' is not zero-terminated after the call to memmove().\n", errout.str());
+    }
 };
 
 REGISTER_TEST(TestBufferOverrun)