Implemented new check (Ticket #160): Storing getc() retun value in char variable and comparing to EOF.

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()

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"

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)