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

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

@@ -507,6 +507,137 @@ void CheckIOImpl::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 CheckIOImpl::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()) {
+            // TODO: Handle for loops
+            if (!Token::Match(tok, "while ( ! feof ( %var% )"))
+                continue;
+
+            // Bail out if we cannot identify file pointer
+            const int fpVarId = tok->tokAt(5)->varId();
+            if (fpVarId == 0)
+                continue;
+
+            const Token *endCond = tok->linkAt(1);
+            const Token *bodyStart;
+            const Token *bodyEnd;
+
+            if (Token::simpleMatch(tok->previous(), "}") && tok->previous()->scope()->type == ScopeType::eDo) {
+                bodyEnd = tok->previous();
+                bodyStart = bodyEnd->link();
+            } else {
+                if (!Token::simpleMatch(endCond, ") {"))
+                    continue;
+                bodyEnd = endCond->linkAt(1);
+                bodyStart = endCond->next();
+            }
+
+            // Bail out if the loop contains control flow (too complex to analyze)
+            if (Token::findmatch(bodyStart, "return|break|goto|continue|throw", bodyEnd))
+                continue;
+
+            // Bail out if fp is used outside of known file I/O functions.
+            // If it is passed to an unknown function, reads may occur there.
+            bool fpUsedElsewhere = false;
+            for (const Token *t = bodyStart->next(); t && t != bodyEnd; t = t->next()) {
+                if (t->varId() != fpVarId)
+                    continue;
+                const Token *p = t->astParent();
+                while (p && p->str() == ",")
+                    p = p->astParent();
+                if (!p || !Token::Match(p->astOperand1(), "fgets|fgetc|getc|fread|fscanf|fprintf|fwrite|fputs|fputc|putc")) {
+                    fpUsedElsewhere = true;
+                    break;
+                }
+            }
+            if (fpUsedElsewhere)
+                continue;
+
+            // No file read call in the loop: feof can never become true inside it
+            const Token *loopFileReadCallTok = findFileReadCall(bodyStart, bodyEnd, fpVarId);
+            if (!loopFileReadCallTok) {
+                // TODO: Warn about infinite loop
+                continue;
+            }
+
+            // Find last file read
+            const Token *lastLoopFileReadCallTok = loopFileReadCallTok;
+            while (loopFileReadCallTok) {
+                lastLoopFileReadCallTok = loopFileReadCallTok;
+                loopFileReadCallTok = findFileReadCall(lastLoopFileReadCallTok->next(), bodyEnd, fpVarId);
+            }
+
+            // Warn if the destination of the last file read is used after the call before bodyEnd.
+            // If it is not, the stale buffer is never accessed on the extra iteration at EOF.
+
+            if (lastLoopFileReadCallTok->str() == "fgetc" || lastLoopFileReadCallTok->str() == "getc") {
+                // Warn if the return value feeds into an expression (astParent of the call node)
+                if (lastLoopFileReadCallTok->astParent() && lastLoopFileReadCallTok->astParent()->astParent())
+                    wrongfeofUsage(getCondTok(tok));
+            } else {
+                const std::vector<const Token*> args = getArguments(lastLoopFileReadCallTok);
+                // Collect destination varIds
+                std::vector<nonneg int> destVarIds;
+                if (lastLoopFileReadCallTok->str() == "fscanf") {
+                    // args[0]=fp, args[1]=format, args[2+]=destinations (typically &var)
+                    for (std::size_t i = 2; i < args.size(); ++i) {
+                        const Token *destTok = Token::Match(args[i], "& %var%") ? args[i]->next() : args[i];
+                        if (destTok->varId() != 0)
+                            destVarIds.push_back(destTok->varId());
+                    }
+                } else {
+                    // Handle fgets, fread
+                    // First argument is the destination buffer
+                    if (!args.empty() && args.front()->varId() != 0)
+                        destVarIds.push_back(args.front()->varId());
+                }
+
+                // Search for any destination use between this call's ';' and endBody
+                const Token *semiColonTok = lastLoopFileReadCallTok->linkAt(1)->next();
+                for (const Token *t = semiColonTok; t && t != bodyEnd; t = t->next()) {
+                    if (std::find(destVarIds.begin(), destVarIds.end(), t->varId()) != destVarIds.end()) {
+                        wrongfeofUsage(getCondTok(tok));
+                        break;
+                    }
+                }
+            }
+        }
+    }
+}
+
+void CheckIOImpl::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
@@ -2057,6 +2188,7 @@ void CheckIO::runChecks(const Tokenizer &tokenizer, ErrorLogger& errorLogger)
     checkIO.checkWrongPrintfScanfArguments();
     checkIO.checkCoutCerrMisusage();
     checkIO.checkFileUsage();
+    checkIO.checkWrongfeofUsage();
     checkIO.invalidScanf();
 }
 
@@ -2072,6 +2204,7 @@ void CheckIO::getErrorMessages(ErrorLogger& errorLogger, const Settings &setting
     c.fcloseInLoopConditionError(nullptr, "fp");
     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

@@ -82,6 +82,9 @@ class CPPCHECKLIB CheckIOImpl : public CheckImpl {
     /** @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();
 
@@ -127,6 +130,7 @@ class CPPCHECKLIB CheckIOImpl : public CheckImpl {
     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,

man/checkers/wrongfeofUsage.md

@@ -0,0 +1,42 @@
+# wrongfeofUsage
+
+**Message**: Using feof() as a loop condition causes the last line to be processed twice.<br/>
+**Category**: Correctness<br/>
+**Severity**: Warning<br/>
+**Language**: C/C++
+
+## Description
+
+`feof()` returns non-zero only after a read operation has failed because the end of file was reached. When used as the sole condition of a loop, the loop body executes one extra time after the last successful read: the read fails silently (or returns partial data), and only then does `feof()` return true and terminate the loop.
+
+This checker matches `while (!feof(fp))` and `do { ... } while (!feof(fp))` loops and warns when all of the following are true:
+
+- The loop body contains at least one file-read call (`fgets`, `fgetc`, `getc`, `fread`, or `fscanf`) on the same file pointer.
+- The destination (return value or output buffer) of the **last** file-read call in the loop is used after that call within the same loop iteration.
+
+The checker skips loops that contain a control-flow statement (`return`, `break`, `goto`, `continue`, `throw`) as those are too complex to analyze reliably, and loops where the file pointer appears in a context other than a recognised I/O function (`fgets`, `fgetc`, `getc`, `fread`, `fscanf`, `fprintf`, `fwrite`, `fputs`, `fputc`, `putc`).
+
+## How to fix
+
+Check the return value of the read function directly in the loop condition.
+
+Before:
+```c
+void process(FILE *fp) {
+    char line[256];
+    while (!feof(fp)) {          /* wrong: processes last line twice */
+        fgets(line, sizeof(line), fp);
+        puts(line);
+    }
+}
+```
+
+After:
+```c
+void process(FILE *fp) {
+    char line[256];
+    while (fgets(line, sizeof(line), fp) != NULL) {
+        puts(line);
+    }
+}
+```

releasenotes.txt

@@ -5,7 +5,7 @@ Major bug fixes & crashes:
 -
 
 New checks:
--
+- Warn when feof() is used as a while loop condition (wrongfeofUsage).
 
 C/C++ support:
 -

test/cli/other_test.py

@@ -4318,25 +4318,25 @@ def __test_active_checkers(tmp_path, active_cnt, total_cnt, use_misra=False, use
 
 
 def test_active_unusedfunction_only(tmp_path):
-    __test_active_checkers(tmp_path, 1, 186, use_unusedfunction_only=True)
+    __test_active_checkers(tmp_path, 1, 187, use_unusedfunction_only=True)
 
 
 def test_active_unusedfunction_only_builddir(tmp_path):
     checkers_exp = [
         'CheckUnusedFunctions::check'
     ]
-    __test_active_checkers(tmp_path, 1, 186, use_unusedfunction_only=True, checkers_exp=checkers_exp)
+    __test_active_checkers(tmp_path, 1, 187, use_unusedfunction_only=True, checkers_exp=checkers_exp)
 
 
 def test_active_unusedfunction_only_misra(tmp_path):
-    __test_active_checkers(tmp_path, 1, 386, use_unusedfunction_only=True, use_misra=True)
+    __test_active_checkers(tmp_path, 1, 387, use_unusedfunction_only=True, use_misra=True)
 
 
 def test_active_unusedfunction_only_misra_builddir(tmp_path):
     checkers_exp = [
         'CheckUnusedFunctions::check'
     ]
-    __test_active_checkers(tmp_path, 1, 386, use_unusedfunction_only=True, use_misra=True, checkers_exp=checkers_exp)
+    __test_active_checkers(tmp_path, 1, 387, use_unusedfunction_only=True, use_misra=True, checkers_exp=checkers_exp)
 
 
 def test_analyzerinfo(tmp_path):

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