Fixed #2627 (switch case fall through: redundant strcpy)

lib/checkother.cpp

@@ -482,6 +482,7 @@ void CheckOther::checkRedundantAssignmentInSwitch()
 
         // Check the contents of the switch statement
         std::map<unsigned int, const Token*> varsAssigned;
+        std::map<unsigned int, const Token*> stringsCopied;
         int indentLevel = 0;
         for (const Token *tok2 = tok->tokAt(5); tok2; tok2 = tok2->next())
         {
@@ -496,9 +497,17 @@ void CheckOther::checkRedundantAssignmentInSwitch()
                     for (const Token* tok3 = tok2; tok3 != endOfConditional; tok3 = tok3->next())
                     {
                         if (tok3->varId() != 0)
+                        {
                             varsAssigned.erase(tok3->varId());
+                            stringsCopied.erase(tok3->varId());
+                        }
                         else if (Token::Match(tok3, functionPattern) || Token::Match(tok3, breakPattern))
+                        {
                             varsAssigned.clear();
+
+                            if (tok3->str() != "strcpy" && tok3->str() != "strncpy")
+                                stringsCopied.clear();
+                        }
                     }
                     tok2 = endOfConditional;
                 }
@@ -524,6 +533,19 @@ void CheckOther::checkRedundantAssignmentInSwitch()
                     varsAssigned[tok2->varId()] = tok2;
                 else
                     redundantAssignmentInSwitchError(i->second, i->second->str());
+
+                stringsCopied.erase(tok2->varId());
+            }
+            // String copy. Report an error if it's copied to twice before a break. E.g.:
+            //    case 3: strcpy(str, "a");    // <== redundant
+            //    case 4: strcpy(str, "b");
+            else if (Token::Match(tok2->previous(), ";|{|}|: strcpy|strncpy ( %var% ,") && tok2->tokAt(2)->varId() != 0)
+            {
+                std::map<unsigned int, const Token*>::iterator i = stringsCopied.find(tok2->tokAt(2)->varId());
+                if (i == stringsCopied.end())
+                    stringsCopied[tok2->tokAt(2)->varId()] = tok2->tokAt(2);
+                else
+                    redundantStrcpyInSwitchError(i->second, i->second->str());
             }
             // Not a simple assignment so there may be good reason if this variable is assigned to twice. E.g.:
             //    case 3: b = 1;
@@ -534,8 +556,13 @@ void CheckOther::checkRedundantAssignmentInSwitch()
             // Reset our record of assignments if there is a break or function call. E.g.:
             //    case 3: b = 1; break;
             if (Token::Match(tok2, functionPattern) || Token::Match(tok2, breakPattern))
+            {
                 varsAssigned.clear();
 
+                if (tok2->str() != "strcpy" && tok2->str() != "strncpy")
+                    stringsCopied.clear();
+            }
+
         }
 
         tok = Token::findmatch(tok->next(), switchPattern);
@@ -548,6 +575,14 @@ void CheckOther::redundantAssignmentInSwitchError(const Token *tok, const std::s
                 "redundantAssignInSwitch", "Redundant assignment of \"" + varname + "\" in switch");
 }
 
+void CheckOther::redundantStrcpyInSwitchError(const Token *tok, const std::string &varname)
+{
+    reportError(tok, Severity::warning,
+                "redundantStrcpyInSwitch",
+                "Switch case fall-through. Redundant strcpy of \"" + varname + "\".\n"
+                "Switch case fall-through. Redundant strcpy of \"" + varname + "\". The string is overwritten in a later case block.");
+}
+
 //---------------------------------------------------------------------------
 //---------------------------------------------------------------------------
 void CheckOther::checkSwitchCaseFallThrough()

lib/checkother.h

@@ -252,6 +252,7 @@ public:
     void mathfunctionCallError(const Token *tok, const unsigned int numParam = 1);
     void fflushOnInputStreamError(const Token *tok, const std::string &varname);
     void redundantAssignmentInSwitchError(const Token *tok, const std::string &varname);
+    void redundantStrcpyInSwitchError(const Token *tok, const std::string &varname);
     void switchCaseFallThrough(const Token *tok);
     void selfAssignmentError(const Token *tok, const std::string &varname);
     void assignmentInAssertError(const Token *tok, const std::string &varname);
@@ -362,6 +363,7 @@ public:
                "* condition that is always true/false\n"
                "* unusal pointer arithmetic. For example: \"abc\" + 'd'\n"
                "* redundant assignment in a switch statement\n"
+               "* redundant strcpy in a switch statement\n"
                "* look for 'sizeof sizeof ..'\n"
                "* look for calculations inside sizeof()\n"
                "* assignment of a variable to itself\n"

test/testother.cpp

@@ -1305,6 +1305,72 @@ private:
               "        }\n"
               "}\n");
         ASSERT_EQUALS("", errout.str());
+
+        check("void foo(char *str, int a)\n"
+              "{\n"
+              "        switch (a)\n"
+              "        {\n"
+              "        case 2:\n"
+              "          strcpy(str, \"a'\");\n"
+              "        case 3:\n"
+              "          strcpy(str, \"b'\");\n"
+              "        }\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:6]: (warning) Switch case fall-through. Redundant strcpy of \"str\".\n", errout.str());
+
+        check("void foo(char *str, int a)\n"
+              "{\n"
+              "        switch (a)\n"
+              "        {\n"
+              "        case 2:\n"
+              "          strncpy(str, \"a'\");\n"
+              "        case 3:\n"
+              "          strncpy(str, \"b'\");\n"
+              "        }\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:6]: (warning) Switch case fall-through. Redundant strcpy of \"str\".\n", errout.str());
+
+        check("void foo(char *str, int a)\n"
+              "{\n"
+              "		   int z = 0;\n"
+              "        switch (a)\n"
+              "        {\n"
+              "        case 2:\n"
+              "          strcpy(str, \"a'\");\n"
+              "			 z++;\n"
+              "        case 3:\n"
+              "          strcpy(str, \"b'\");\n"
+              "			 z++;\n"
+              "        }\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:7]: (warning) Switch case fall-through. Redundant strcpy of \"str\".\n", errout.str());
+
+        check("void foo(char *str, int a)\n"
+              "{\n"
+              "        switch (a)\n"
+              "        {\n"
+              "        case 2:\n"
+              "          strcpy(str, \"a'\");\n"
+              "			 break;\n"
+              "        case 3:\n"
+              "          strcpy(str, \"b'\");\n"
+              "			 break;\n"
+              "        }\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void foo(char *str, int a)\n"
+              "{\n"
+              "        switch (a)\n"
+              "        {\n"
+              "        case 2:\n"
+              "          strcpy(str, \"a'\");\n"
+              "			 printf(str);\n"
+              "        case 3:\n"
+              "          strcpy(str, \"b'\");\n"
+              "        }\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
     }
 
     void switchFallThroughCase()