Fix cppcheck-opensource#958: warn when feof() is used as a while loop condition

Using while(!feof(fp)) is a common C bug: feof() only returns true
after a read has already failed, causing the loop body to execute
once more after the last successful read. Read errors also go
undetected since feof() does not distinguish them from EOF.

Signed-off-by: Francois Berder <fberder@outlook.fr>

Author: francois-berder

lib/checkio.cpp

@@ -481,6 +481,36 @@ void CheckIO::invalidScanfError(const Token *tok)
                 CWE119, Certainty::normal);
 }
 
+void CheckIO::checkWrongfeofUsage()
+{
+    const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
+
+    logChecker("CheckIO::checkWrongfeofUsage");
+
+    for (const Scope * scope : symbolDatabase->functionScopes) {
+        for (const Token *tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) {
+            if (Token::simpleMatch(tok, "while (")) {
+                const Token* cond = getCondTok(tok);
+                if (!cond)
+                    continue;
+                if (Token::Match(cond, "! feof (")) {
+                    wrongfeofUsage(cond);
+                }
+            }
+        }
+    }
+}
+
+void CheckIO::wrongfeofUsage(const Token * tok)
+{
+    reportError(tok, Severity::warning,
+                "wrongfeofUsage",
+                "Using feof() as a loop condition may cause the last line to be processed twice.\n"
+                "feof() returns true only after a read has failed due to end-of-file, so the loop "
+                "body executes once more after the last successful read. Check the return value of "
+                "the read function instead (e.g. fgets, fread, fscanf).");
+}
+
 //---------------------------------------------------------------------------
 //    printf("%u", "xyz"); // Wrong argument type
 //    printf("%u%s", 1); // Too few arguments
@@ -2031,6 +2061,7 @@ void CheckIO::runChecks(const Tokenizer &tokenizer, ErrorLogger *errorLogger)
     checkIO.checkWrongPrintfScanfArguments();
     checkIO.checkCoutCerrMisusage();
     checkIO.checkFileUsage();
+    checkIO.checkWrongfeofUsage();
     checkIO.invalidScanf();
 }
 

lib/checkio.h

@@ -64,6 +64,9 @@ class CPPCHECKLIB CheckIO : public Check {
     /** @brief scanf can crash if width specifiers are not used */
     void invalidScanf();
 
+    /** @brief %Check wrong usage of feof */
+    void checkWrongfeofUsage();
+
     /** @brief %Checks type and number of arguments given to functions like printf or scanf*/
     void checkWrongPrintfScanfArguments();
 
@@ -108,6 +111,7 @@ class CPPCHECKLIB CheckIO : public Check {
     void seekOnAppendedFileError(const Token *tok);
     void incompatibleFileOpenError(const Token *tok, const std::string &filename);
     void invalidScanfError(const Token *tok);
+    void wrongfeofUsage(const Token *tok);
     void wrongPrintfScanfArgumentsError(const Token* tok,
                                         const std::string &functionName,
                                         nonneg int numFormat,

test/testio.cpp

@@ -45,6 +45,7 @@ class TestIO : public TestFixture {
         TEST_CASE(seekOnAppendedFile);
         TEST_CASE(fflushOnInputStream);
         TEST_CASE(incompatibleFileOpen);
+        TEST_CASE(testWrongfeofUsage); // #958
 
         TEST_CASE(testScanf1); // Scanf without field limiters
         TEST_CASE(testScanf2);
@@ -743,6 +744,19 @@ class TestIO : public TestFixture {
         ASSERT_EQUALS("[test.cpp:3:16]: (warning) The file '\"tmp\"' is opened for read and write access at the same time on different streams [incompatibleFileOpen]\n", errout_str());
     }
 
+    void testWrongfeofUsage() { // ticket #958
+        check("void foo() {\n"
+              "  FILE * fp = fopen(\"test.txt\", \"r\");\n"
+              "  while (!feof(fp)) \n"
+              "  {\n"
+              "     char line[100];\n"
+              "     fgets(line, sizeof(line), fp);\n"
+              "  }\n"
+              "  fclose(fp);\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3:10]: (warning) Using feof() as a loop condition may cause the last line to be processed twice. [wrongfeofUsage]\n", errout_str());
+    }
+
 
     void testScanf1() {
         check("void foo() {\n"