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

Author: francois-berder

lib/checkio.cpp

@@ -248,20 +248,13 @@ void CheckIO::checkFileUsage()
 
                     // #1473 Check if fclose is in a while loop condition
                     if (fileTok && fileTok->isVariable()) {
-                        const Token* tmp = tok->astParent();
-                        const Token* loopTok = nullptr;
-                        while (tmp) {
-                            if (Token::simpleMatch(tmp->previous(), "while (")) {
-                                loopTok = tmp->previous();
-                                break;
-                            }
-                            tmp = tmp->astParent();
-                        }
-                        if (loopTok) {
+                        const Token* loopTok = tok->astTop()->previous();
+
+                        if (loopTok && loopTok->str() == "while") {
                             const Token* bodyEnd = nullptr;
                             const Token* bodyStart = nullptr;
 
-                            if (Token::simpleMatch(loopTok->previous(), "}") && Token::simpleMatch(loopTok->linkAt(-1)->previous(), "do")) {  // Handle do-while loops
+                            if (Token::simpleMatch(loopTok->previous(), "}") && loopTok->previous()->scope()->type == ScopeType::eDo) { // Handle do-while loops
                                 bodyEnd = loopTok->previous();
                                 bodyStart = bodyEnd->link();
                             } else {
@@ -271,7 +264,7 @@ void CheckIO::checkFileUsage()
 
                             // Do not trigger a warning if the loop always exits or if the file is opened again in the loop.
                             if (!isReturnScope(bodyEnd, mSettings->library) && Token::findmatch(bodyStart, "%var% =", bodyEnd, fileTok->varId()) == nullptr)
-                                useClosedFileError(tok);
+                                fcloseInLoopConditionError(tok, fileTok->str());
                         }
                     }
                 } else if (whitelist.find(tok->str()) != whitelist.end()) {
@@ -421,6 +414,15 @@ void CheckIO::useClosedFileError(const Token *tok)
                 "useClosedFile", "Used file that is not opened.", CWE910, Certainty::normal);
 }
 
+void CheckIO::fcloseInLoopConditionError(const Token *tok, const std::string &varname)
+{
+    reportError(tok, Severity::warning,
+                "fcloseInLoopCondition",
+                "fclose() should not be used as a loop condition.\n"
+                "fclose() closes '" + varname + "' each time it is evaluated. On success the loop body might never execute, on failure fclose() might be called again on the already-closed file handle.",
+                CWE910, Certainty::normal);
+}
+
 void CheckIO::seekOnAppendedFileError(const Token *tok)
 {
     reportError(tok, Severity::warning,
@@ -2071,6 +2073,7 @@ void CheckIO::getErrorMessages(ErrorLogger *errorLogger, const Settings *setting
     c.readWriteOnlyFileError(nullptr);
     c.writeReadOnlyFileError(nullptr);
     c.useClosedFileError(nullptr);
+    c.fcloseInLoopConditionError(nullptr, "fp");
     c.seekOnAppendedFileError(nullptr);
     c.incompatibleFileOpenError(nullptr, "tmp");
     c.invalidScanfError(nullptr);

lib/checkio.h

@@ -105,6 +105,7 @@ class CPPCHECKLIB CheckIO : public Check {
     void readWriteOnlyFileError(const Token *tok);
     void writeReadOnlyFileError(const Token *tok);
     void useClosedFileError(const Token *tok);
+    void fcloseInLoopConditionError(const Token *tok, const std::string &varname);
     void seekOnAppendedFileError(const Token *tok);
     void incompatibleFileOpenError(const Token *tok, const std::string &filename);
     void invalidScanfError(const Token *tok);

test/testio.cpp

@@ -544,7 +544,7 @@ class TestIO : public TestFixture {
               "    FILE *a = fopen(\"aa\", \"r\");\n"
               "    while (fclose(a)) {}\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:3:12]: (error) Used file that is not opened. [useClosedFile]\n", errout_str());
+        ASSERT_EQUALS("[test.cpp:3:12]: (warning) fclose() should not be used as a loop condition. [fcloseInLoopCondition]\n", errout_str());
 
         check("void foo() {\n"
               "    FILE *a = fopen(\"aa\", \"r\");\n"
@@ -566,7 +566,7 @@ class TestIO : public TestFixture {
               "    FILE *a = fopen(\"aa\", \"r\");\n"
               "    do {} while (fclose(a));\n"
               "}");
-        ASSERT_EQUALS("[test.cpp:3:18]: (error) Used file that is not opened. [useClosedFile]\n", errout_str());
+        ASSERT_EQUALS("[test.cpp:3:18]: (warning) fclose() should not be used as a loop condition. [fcloseInLoopCondition]\n", errout_str());
 
         // #6823
         check("void foo() {\n"