diff --git a/lib/checkother.cpp b/lib/checkother.cpp index f1f56b5c1..f29476149 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -32,6 +32,77 @@ namespace { CheckOther instance; } +//---------------------------------------------------------------------------------- +// The return value of fgetc(), getc(), ungetc(), getchar() etc. is an integer value. +// If this return value is stored in a character variable and then compared +// to compared to EOF, which is an integer, the comparison maybe be false. +// +// Reference: +// - Ticket #160 +// - http://www.cplusplus.com/reference/cstdio/fgetc/ +// - http://www.cplusplus.com/reference/cstdio/getc/ +// - http://www.cplusplus.com/reference/cstdio/getchar/ +// - http://www.cplusplus.com/reference/cstdio/ungetc/ .... +//---------------------------------------------------------------------------------- +void CheckOther::checkCastIntToCharAndBack() +{ + if (!_settings->isEnabled("warning")) + return; + + const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase(); + const std::size_t functions = symbolDatabase->functionScopes.size(); + for (std::size_t i = 0; i < functions; ++i) { + const Scope * scope = symbolDatabase->functionScopes[i]; + unsigned int uiVarId = 0; + std::string strFunctionName; + const std::string strFunctionsToSearch(); + for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { + if (Token::Match(tok, "%var% = fclose|fflush|fputc|fputs|fscanf|getchar|getc|fgetc|putchar|putc|puts|scanf|sscanf|ungetc (")) { + if (tok->varId()) { + const Variable *var = tok->variable(); + if (var && var->typeEndToken()->str() == "char" && !var->typeEndToken()->isSigned()) { + uiVarId = tok->varId(); + strFunctionName = tok->tokAt(2)->str(); + } + } + } else if (Token::Match(tok, "EOF %comp% ( %var% = fclose|fflush|fputc|fputs|fscanf|getchar|getc|fgetc|putchar|putc|puts|scanf|sscanf|ungetc (")) { + tok = tok->tokAt(3); + if (tok && tok->varId()) { + const Variable *var = tok->variable(); + if (var && var->typeEndToken()->str() == "char" && !var->typeEndToken()->isSigned()) { + checkCastIntToCharAndBackError(tok, tok->tokAt(2)->str()); + } + } + } + if (Token::Match(tok, "%var% %comp% EOF")) { + if (uiVarId && tok->varId() == uiVarId) { + checkCastIntToCharAndBackError(tok, strFunctionName); + } + } else if (Token::Match(tok, "EOF %comp% %var%")) { + tok = tok->tokAt(2); + if (uiVarId && tok->varId() == uiVarId) { + checkCastIntToCharAndBackError(tok, strFunctionName); + } + } + } + } +} + +void CheckOther::checkCastIntToCharAndBackError(const Token *tok, const std::string &strFunctionName) +{ + reportError( + tok, + Severity::warning, + "checkCastIntToCharAndBack", + "Storing "+ strFunctionName +"() return value in char variable and then comparing with EOF.\n" + "When saving "+ strFunctionName +"() return value in char variable there is loss of precision. " + " When "+ strFunctionName +"() returns EOF this value is truncated. Comparing the char " + "variable with EOF can have unexpected results. For instance a loop \"while (EOF != (c = "+ strFunctionName +"());\" " + "loops forever on some compilers/platforms and on other compilers/platforms it will stop " + "when the file contains a matching character." + ); +} + //--------------------------------------------------------------------------- //--------------------------------------------------------------------------- void CheckOther::checkIncrementBoolean() diff --git a/lib/checkother.h b/lib/checkother.h index 9bf3d836a..f4d38eccd 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -93,6 +93,7 @@ public: checkOther.clarifyStatement(); checkOther.checkConstantFunctionParameter(); checkOther.checkIncompleteStatement(); + checkOther.checkCastIntToCharAndBack(); checkOther.invalidFunctionUsage(); checkOther.checkZeroDivision(); @@ -301,11 +302,15 @@ public: /** @brief %Check that calling the POSIX pipe() system call is called with an integer array of size two. */ void checkPipeParameterSize(); + /** @brief %Check to avoid casting a return value to unsigned char and then back to integer type. */ + void checkCastIntToCharAndBack(); + private: bool isUnsigned(const Variable *var) const; bool isSigned(const Variable *var) const; // Error messages.. + void checkCastIntToCharAndBackError(const Token *tok, const std::string &strFunctionName); void checkPipeParameterSizeError(const Token *tok, const std::string &strVarName, const std::string &strDim); void oppositeInnerConditionError(const Token *tok); void clarifyCalculationError(const Token *tok, const std::string &op); @@ -405,6 +410,7 @@ private: c.redundantAssignmentError(0, 0, "var", false); // style/warning + c.checkCastIntToCharAndBackError(0,"func_name"); c.oppositeInnerConditionError(0); c.cstyleCastError(0); c.dangerousUsageStrtolError(0, "strtol"); @@ -476,6 +482,7 @@ private: "* double free() or double closedir()\n" "* bitwise operation with negative right operand\n" "* provide wrong dimensioned array to pipe() system command (--std=posix)\n" + "* cast the return values of getc(),fgetc() and getchar() to character and compare it to EOF\n" //performance "* redundant data copying for const variable\n" diff --git a/test/testother.cpp b/test/testother.cpp index 37f13cfdc..222c85549 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -197,6 +197,8 @@ private: TEST_CASE(varFuncNullUB); TEST_CASE(checkPipeParameterSize); // ticket #3521 + + TEST_CASE(checkCastIntToCharAndBack); // ticket #160 } void check(const char code[], const char *filename = NULL, bool experimental = false, bool inconclusive = true, bool posix = false) { @@ -7102,6 +7104,131 @@ private: "}",NULL,false,false,true); ASSERT_EQUALS("", errout.str()); } + + void checkCastIntToCharAndBack() { // #160 + + // check getchar + check("void f(){ \n" + "unsigned char c;\n" + " while( (c = getchar()) != EOF)\n" + " {\n" + " bar(c);\n" + " } ;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Storing getchar() return value in char variable and then comparing with EOF.\n", errout.str()); + + check("void f(){ \n" + "unsigned char c = getchar();\n" + " while( EOF != c)\n" + " {\n" + " bar(c);\n" + " } ;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Storing getchar() return value in char variable and then comparing with EOF.\n", errout.str()); + + check("void f(){ \n" + "unsigned char c;\n" + " while( EOF != (c = getchar()) )\n" + " {\n" + " bar(c);\n" + " } ;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Storing getchar() return value in char variable and then comparing with EOF.\n", errout.str()); + + check("void f(){ \n" + "int i;\n" + " while( (i = getchar()) != EOF)\n" + " {\n" + " bar(i);\n" + " } ;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(){ \n" + "int i;\n" + " while( EOF != (i = getchar()) )\n" + " {\n" + " bar(i);\n" + " } ;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + + // check getc + check("voif f (FILE * pFile){\n" + "unsigned char c;\n" + "do {\n" + " c = getc (pFile);\n" + "} while (c != EOF)" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Storing getc() return value in char variable and then comparing with EOF.\n", errout.str()); + + check("voif f (FILE * pFile){\n" + "unsigned char c;\n" + "do {\n" + " c = getc (pFile);\n" + "} while (EOF != c)" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Storing getc() return value in char variable and then comparing with EOF.\n", errout.str()); + + check("voif f (FILE * pFile){\n" + "int i;\n" + "do {\n" + " i = getc (pFile);\n" + "} while (i != EOF)" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("voif f (FILE * pFile){\n" + "int i;\n" + "do {\n" + " i = getc (pFile);\n" + "} while (EOF != i)" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + + // check fgetc + check("voif f (FILE * pFile){\n" + "unsigned char c;\n" + "do {\n" + " c = fgetc (pFile);\n" + "} while (c != EOF)" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Storing fgetc() return value in char variable and then comparing with EOF.\n", errout.str()); + + check("voif f (FILE * pFile){\n" + "char c;\n" + "do {\n" + " c = fgetc (pFile);\n" + "} while (EOF != c)" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (warning) Storing fgetc() return value in char variable and then comparing with EOF.\n", errout.str()); + + check("voif f (FILE * pFile){\n" + "signed char c;\n" + "do {\n" + " c = fgetc (pFile);\n" + "} while (EOF != c)" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("voif f (FILE * pFile){\n" + "int i;\n" + "do {\n" + " i = fgetc (pFile);\n" + "} while (i != EOF)" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("voif f (FILE * pFile){\n" + "int i;\n" + "do {\n" + " i = fgetc (pFile);\n" + "} while (EOF != i)" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestOther)