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

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/checkers.cpp

@@ -101,6 +101,7 @@ namespace checkers {
         {"CheckFunctions::useStandardLibrary","style"},
         {"CheckIO::checkCoutCerrMisusage","c"},
         {"CheckIO::checkFileUsage",""},
+        {"CheckIO::checkWrongfeofUsage",""},
         {"CheckIO::checkWrongPrintfScanfArguments",""},
         {"CheckIO::invalidScanf",""},
         {"CheckLeakAutoVar::check","notclang"},

lib/checkio.cpp

@@ -481,6 +481,68 @@ void CheckIO::invalidScanfError(const Token *tok)
                 CWE119, Certainty::normal);
 }
 
+static const Token* findFileReadCall(const Token *start, const Token *end, int varid)
+{
+    const Token* found = Token::findmatch(start, "fgets|fgetc|getc|fread|fscanf (", end);
+    while (found) {
+        const std::vector<const Token*> args = getArguments(found);
+        if (!args.empty()) {
+            const bool match = (found->str() == "fscanf")
+                               ? args.front()->varId() == varid
+                               : args.back()->varId() == varid;
+            if (match)
+                return found;
+        }
+        found = Token::findmatch(found->next(), "fgets|fgetc|getc|fread|fscanf (", end);
+    }
+    return nullptr;
+}
+
+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 ( ! feof ("))
+                continue;
+
+            // Bail out if we cannot identify file pointer
+            const int fpVarId = tok->tokAt(5)->varId();
+            if (fpVarId == 0)
+                continue;
+
+            // Usage of feof is correct if a read happens before and within the loop.
+            // However, if we find a control flow statement in between the fileReadCall
+            // token and the while loop condition, then we bail out.
+            const Token *endCond = tok->linkAt(1);
+            const Token *endBody = endCond->linkAt(1);
+
+            const Token *prevFileReadCallTok = findFileReadCall(scope->bodyStart, tok, fpVarId);
+            const Token *loopFileReadCallTok = findFileReadCall(tok, endBody, fpVarId);
+            const Token *prevControlFlowTok = Token::findmatch(prevFileReadCallTok, "return|break|goto|continue|throw", tok);
+            const Token *loopControlFlowTok = Token::findmatch(tok, "return|break|goto|continue|throw", loopFileReadCallTok);
+
+            if (prevFileReadCallTok && loopFileReadCallTok && !prevControlFlowTok && !loopControlFlowTok)
+                continue;
+
+            wrongfeofUsage(getCondTok(tok));
+        }
+    }
+}
+
+void CheckIO::wrongfeofUsage(const Token * tok)
+{
+    reportError(tok, Severity::warning,
+                "wrongfeofUsage",
+                "Using feof() as a loop condition causes 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
@@ -2030,6 +2092,7 @@ void CheckIO::runChecks(const Tokenizer &tokenizer, ErrorLogger *errorLogger)
     checkIO.checkWrongPrintfScanfArguments();
     checkIO.checkCoutCerrMisusage();
     checkIO.checkFileUsage();
+    checkIO.checkWrongfeofUsage();
     checkIO.invalidScanf();
 }
 
@@ -2044,6 +2107,7 @@ void CheckIO::getErrorMessages(ErrorLogger *errorLogger, const Settings *setting
     c.useClosedFileError(nullptr);
     c.seekOnAppendedFileError(nullptr);
     c.incompatibleFileOpenError(nullptr, "tmp");
+    c.wrongfeofUsage(nullptr);
     c.invalidScanfError(nullptr);
     c.wrongPrintfScanfArgumentsError(nullptr, "printf",3,2);
     c.invalidScanfArgTypeError_s(nullptr,  1, "s", nullptr);

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,

releasenotes.txt

@@ -8,6 +8,7 @@ New checks:
 - MISRA C 2012 rule 10.3 now warns on assigning integer literals 0 and 1 to bool in C99 and later while preserving the existing C89 behavior.
 - funcArgNamesDifferentUnnamed warns on function declarations/definitions where a parameter in either location is unnamed
 - uninitMemberVarNoCtor warns on user-defined types where some but not all members requiring initialization have in-class initializers.
+- Warn when feof() is used as a while loop condition (wrongfeofUsage).
 
 C/C++ support:
 -

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,36 @@ 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(FILE * fp) {\n"
+              "  while (!feof(fp)) \n"
+              "  {\n"
+              "     char line[100];\n"
+              "     fgets(line, sizeof(line), fp);\n"
+              "  }\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2:10]: (warning) Using feof() as a loop condition causes the last line to be processed twice. [wrongfeofUsage]\n", errout_str());
+
+        check("int foo(FILE *fp) {\n"
+              "  char line[100];\n"
+              "  while (fgets(line, sizeof(line), fp)) {}\n"
+              "  if (!feof(fp))\n"
+              "    return 1;\n"
+              "  return 0;\n"
+              "}");
+        ASSERT_EQUALS("", errout_str());
+
+        check("void foo(FILE *fp){\n"
+              "  char line[100];\n"
+              "  fgets(line, sizeof(line), fp);\n"
+              "  while (!feof(fp)){\n"
+              "    dostuff(line);\n"
+              "    fgets(line, sizeof(line), fp);"
+              "  }\n"
+              "}");
+        ASSERT_EQUALS("", errout_str());
+    }
+
 
     void testScanf1() {
         check("void foo() {\n"