From 82366918ff728a7a6886204cb8b343f48c23ebdb Mon Sep 17 00:00:00 2001 From: Zachary Blair Date: Thu, 13 Oct 2011 01:27:22 -0700 Subject: [PATCH] Fixed #2627 (switch case fall through: redundant strcpy) --- lib/checkother.cpp | 35 ++++++++++++++++++++++++ lib/checkother.h | 2 ++ test/testother.cpp | 66 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index a81292fdb..72e46b5ff 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -482,6 +482,7 @@ void CheckOther::checkRedundantAssignmentInSwitch() // Check the contents of the switch statement std::map varsAssigned; + std::map 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::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() diff --git a/lib/checkother.h b/lib/checkother.h index 5aca5ccbb..a39c1e13f 100644 --- a/lib/checkother.h +++ b/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" diff --git a/test/testother.cpp b/test/testother.cpp index b5cac3077..cf00c3061 100644 --- a/test/testother.cpp +++ b/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()