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

Using fclose() as a while loop condition closes the file on every
iteration and operates on an already-closed file handle from the second
iteration onward.

---------

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

Author: francois-berder

lib/checkio.cpp

@@ -240,6 +240,28 @@ void CheckIO::checkFileUsage()
                 } else if (tok->str() == "fclose") {
                     fileTok = tok->tokAt(2);
                     operation = Filepointer::Operation::CLOSE;
+
+                    // #1473 Check if fclose is in a while loop condition
+                    if (fileTok && fileTok->isVariable()) {
+                        const Token* loopTok = tok->astTop()->previous();
+
+                        if (loopTok && loopTok->str() == "while") {
+                            const Token* bodyEnd = nullptr;
+                            const Token* bodyStart = nullptr;
+
+                            if (Token::simpleMatch(loopTok->previous(), "}") && loopTok->previous()->scope()->type == ScopeType::eDo) { // Handle do-while loops
+                                bodyEnd = loopTok->previous();
+                                bodyStart = bodyEnd->link();
+                            } else {
+                                bodyStart = loopTok->linkAt(1)->next();
+                                bodyEnd = bodyStart->link();
+                            }
+
+                            // 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)
+                                fcloseInLoopConditionError(tok, fileTok->str());
+                        }
+                    }
                 } else if (whitelist.find(tok->str()) != whitelist.end()) {
                     fileTok = tok->tokAt(2);
                     if ((tok->str() == "ungetc" || tok->str() == "ungetwc") && fileTok)
@@ -387,6 +409,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() used as loop condition may skip loop body or double-close file handle.\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,
@@ -2038,6 +2069,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);

man/checkers/fcloseInLoopCondition.md

@@ -0,0 +1,40 @@
+# fcloseInLoopCondition
+
+**Message**: fclose() used as loop condition may skip loop body or double-close file handle.<br/>
+**Category**: Resource Management<br/>
+**Severity**: Warning<br/>
+**Language**: C and C++
+
+## Description
+
+Using `fclose()` as a loop condition leads to two unwanted outcomes:
+
+- On **success**, the condition is false and the loop body never executes. The intent was likely to process the file inside the loop, but it is already closed.
+- On **failure**, the condition is true and the loop body executes but `fclose()` is called again on the already-closed file handle on the next iteration, which is undefined behaviour.
+
+This pattern is almost always a misunderstanding of what `fclose()` returns, or confusion with a function that reads/processes data incrementally (like `fgets` or `fread`). Unlike those functions, `fclose()` is a one-shot teardown operation and has no meaningful retry or loop-until-done semantic.
+
+## How to fix
+
+Call `fclose()` outside the loop condition. If you need to check whether the close succeeded, store the return value and test it separately.
+
+Before:
+```c
+FILE *fp = fopen("data.txt", "r");
+while (fclose(fp)) {
+    /* process file */
+}
+```
+
+After:
+```c
+FILE *fp = fopen("data.txt", "r");
+/* process file */
+if (fclose(fp) != 0) {
+    /* handle close error */
+}
+```
+
+## Related checkers
+
+- `useClosedFile` - for using a file handle that has already been closed

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.
+- fcloseInLoopCondition warns when fclose() is used as a while loop condition, which may skip the loop body or double-close the file handle.
 
 C/C++ support:
 -

test/testio.cpp

@@ -545,7 +545,29 @@ class TestIO : public TestFixture {
               "    FILE *a = fopen(\"aa\", \"r\");\n"
               "    while (fclose(a)) {}\n"
               "}");
-        TODO_ASSERT_EQUALS("[test.cpp:3:5]: (error) Used file that is not opened. [useClosedFile]\n", "", errout_str());
+        ASSERT_EQUALS("[test.cpp:3:12]: (warning) fclose() used as loop condition may skip loop body or double-close file handle. [fcloseInLoopCondition]\n", errout_str());
+
+        check("void foo() {\n"
+              "    FILE *a = fopen(\"aa\", \"r\");\n"
+              "    while (fclose(a)) {\n"
+              "         break;\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout_str());
+
+        check("void foo() {\n"
+              "    FILE *a = fopen(\"aa\", \"r\");\n"
+              "    while (fclose(a)) {\n"
+              "        a = fopen(\"aa\", \"r\");\n"
+              "    }\n"
+              "}");
+        ASSERT_EQUALS("", errout_str());
+
+        check("void foo() {\n"
+              "    FILE *a = fopen(\"aa\", \"r\");\n"
+              "    do {} while (fclose(a));\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:3:18]: (warning) fclose() used as loop condition may skip loop body or double-close file handle. [fcloseInLoopCondition]\n", errout_str());
 
         // #6823
         check("void foo() {\n"