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

Author: francois-berder

lib/checkio.cpp

@@ -481,6 +481,20 @@ void CheckIO::invalidScanfError(const Token *tok)
                 CWE119, Certainty::normal);
 }
 
+static bool findFileReadCall(const Token *start, const Token *end, int varid)
+{
+    const Token* found = Token::findmatch(start, "fgets|fgetc|getc|fread|fscanf (", end);
+    const std::vector<const Token*> args = getArguments(found);
+    if (args.empty())
+        return false;
+
+    if (found->str() == "fscanf") {
+        return args.front()->varId() == varid;
+    } else {
+        return args.back()->varId() == varid;
+    }
+}
+
 void CheckIO::checkWrongfeofUsage()
 {
     const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
@@ -489,14 +503,17 @@ void 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::simpleMatch(cond, "! feof (")) {
-                    wrongfeofUsage(cond);
-                }
-            }
+            if (!Token::Match(tok, "while ( ! feof ("))
+                continue;
+
+            // Usage of feof is correct if a read happens before and within the loop
+            const Token *endCond = tok->linkAt(1);
+            const Token *endBody = endCond->linkAt(1);
+            const int fpVarId = tok->tokAt(5)->varId();
+            if (findFileReadCall(scope->bodyStart, tok, fpVarId) && findFileReadCall(tok, endBody, fpVarId))
+                continue;
+
+            wrongfeofUsage(getCondTok(tok));
         }
     }
 }
@@ -505,7 +522,7 @@ 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"
+                "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).");

releasenotes.txt

@@ -6,6 +6,7 @@ Major bug fixes & crashes:
 
 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.
+- Warn when feof() is used as a while loop condition (wrongfeofUsage).
 
 C/C++ support:
 -

test/testio.cpp

@@ -745,27 +745,33 @@ class TestIO : public TestFixture {
     }
 
     void testWrongfeofUsage() { // ticket #958
-        check("void foo() {\n"
-              "  FILE * fp = fopen(\"test.txt\", \"r\");\n"
+        check("void foo(FILE * fp) {\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());
+        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() {\n"
-              "  FILE * fp = fopen(\"test.txt\", \"r\");\n"
+        check("int foo(FILE *fp) {\n"
               "  char line[100];\n"
               "  while (fgets(line, sizeof(line), fp)) {}\n"
               "  if (!feof(fp))\n"
               "    return 1;\n"
-              "  fclose(fp);\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());
     }