From a350346a2985b0b426a8bee471dadfd37749ae77 Mon Sep 17 00:00:00 2001 From: Francois Berder Date: Tue, 7 Apr 2026 18:56:01 +0200 Subject: [PATCH 1/6] 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 --- lib/checkers.cpp | 1 + lib/checkio.cpp | 64 ++++++++++++++++++++++++++++++++++++++++++++++++ lib/checkio.h | 4 +++ releasenotes.txt | 1 + test/testio.cpp | 31 +++++++++++++++++++++++ 5 files changed, 101 insertions(+) diff --git a/lib/checkers.cpp b/lib/checkers.cpp index f7d7f913da6..deac9f28b77 100644 --- a/lib/checkers.cpp +++ b/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"}, diff --git a/lib/checkio.cpp b/lib/checkio.cpp index 4161b9b224d..93a962c9056 100644 --- a/lib/checkio.cpp +++ b/lib/checkio.cpp @@ -507,6 +507,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 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 @@ -2057,6 +2119,7 @@ void CheckIO::runChecks(const Tokenizer &tokenizer, ErrorLogger *errorLogger) checkIO.checkWrongPrintfScanfArguments(); checkIO.checkCoutCerrMisusage(); checkIO.checkFileUsage(); + checkIO.checkWrongfeofUsage(); checkIO.invalidScanf(); } @@ -2072,6 +2135,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); diff --git a/lib/checkio.h b/lib/checkio.h index b3c86da3577..7aada8ad364 100644 --- a/lib/checkio.h +++ b/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(); @@ -109,6 +112,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, diff --git a/releasenotes.txt b/releasenotes.txt index 5d94da2ebc3..703f12a90e3 100644 --- a/releasenotes.txt +++ b/releasenotes.txt @@ -9,6 +9,7 @@ New checks: - 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. +- Warn when feof() is used as a while loop condition (wrongfeofUsage). C/C++ support: - diff --git a/test/testio.cpp b/test/testio.cpp index d3344f76b8d..119c2d3f936 100644 --- a/test/testio.cpp +++ b/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,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" From f03b094ef74effdd91fe3417a0768fc0b6901aff Mon Sep 17 00:00:00 2001 From: Francois Berder Date: Tue, 28 Apr 2026 15:57:43 +0200 Subject: [PATCH 2/6] fixup! Fix #958: warn when feof() is used as a while loop condition --- test/cli/other_test.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/cli/other_test.py b/test/cli/other_test.py index 243f600b5a6..f788a7d21c8 100644 --- a/test/cli/other_test.py +++ b/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): From e5c65ce1e1f0f5e4378a719bd25d52637adedf0d Mon Sep 17 00:00:00 2001 From: Francois Berder Date: Wed, 29 Apr 2026 19:07:57 +0200 Subject: [PATCH 3/6] fixup! fixup! Fix #958: warn when feof() is used as a while loop condition --- man/checkers/wrongfeofUsage.md | 39 ++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 man/checkers/wrongfeofUsage.md diff --git a/man/checkers/wrongfeofUsage.md b/man/checkers/wrongfeofUsage.md new file mode 100644 index 00000000000..47c84eb4b23 --- /dev/null +++ b/man/checkers/wrongfeofUsage.md @@ -0,0 +1,39 @@ +# wrongfeofUsage + +**Message**: Using feof() as a loop condition causes the last line to be processed twice.
+**Category**: Correctness
+**Severity**: Warning
+**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 `while` 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 warns when it finds feof in the loop condition and either: +- no file-read call (e.g. `fgets`, `fread`, `fscanf`) precedes the loop and is also present inside the loop body +- a control-flow statement (`return`, `break`, `goto`, `continue`, `throw`) is present in the the loop body. + +## 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); + } +} +``` \ No newline at end of file From e835fcae8cce346ff0a02c92da0c83ccaa4cf140 Mon Sep 17 00:00:00 2001 From: Francois Berder Date: Wed, 29 Apr 2026 19:25:00 +0200 Subject: [PATCH 4/6] fixup! fixup! fixup! Fix #958: warn when feof() is used as a while loop condition --- lib/checkio.cpp | 5 +++++ man/checkers/wrongfeofUsage.md | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/checkio.cpp b/lib/checkio.cpp index 93a962c9056..3b22b7e2b35 100644 --- a/lib/checkio.cpp +++ b/lib/checkio.cpp @@ -532,9 +532,14 @@ void CheckIO::checkWrongfeofUsage() for (const Scope * scope : symbolDatabase->functionScopes) { for (const Token *tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { + // TODO: Handle do-while and for loops if (!Token::simpleMatch(tok, "while ( ! feof (")) continue; + // Bail out if we reach a do-while loop + if (Token::simpleMatch(tok->previous(), "}") && Token::simpleMatch(tok->linkAt(-1)->previous(), "do")) + continue; + // Bail out if we cannot identify file pointer const int fpVarId = tok->tokAt(5)->varId(); if (fpVarId == 0) diff --git a/man/checkers/wrongfeofUsage.md b/man/checkers/wrongfeofUsage.md index 47c84eb4b23..b893a62a95d 100644 --- a/man/checkers/wrongfeofUsage.md +++ b/man/checkers/wrongfeofUsage.md @@ -7,7 +7,7 @@ ## 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 `while` 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. +`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 warns when it finds feof in the loop condition and either: - no file-read call (e.g. `fgets`, `fread`, `fscanf`) precedes the loop and is also present inside the loop body From 853ba8abba04ab9677d0abc46eb4d1fce18493f0 Mon Sep 17 00:00:00 2001 From: Francois Berder Date: Mon, 4 May 2026 20:09:51 +0200 Subject: [PATCH 5/6] fixup! fixup! fixup! fixup! Fix #958: warn when feof() is used as a while loop condition --- lib/checkio.cpp | 94 ++++++++++++++++++++++++++++++++++++++++--------- test/testio.cpp | 19 ++++++++++ 2 files changed, 97 insertions(+), 16 deletions(-) diff --git a/lib/checkio.cpp b/lib/checkio.cpp index 3b22b7e2b35..cc80a53ff17 100644 --- a/lib/checkio.cpp +++ b/lib/checkio.cpp @@ -532,12 +532,8 @@ void CheckIO::checkWrongfeofUsage() for (const Scope * scope : symbolDatabase->functionScopes) { for (const Token *tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { - // TODO: Handle do-while and for loops - if (!Token::simpleMatch(tok, "while ( ! feof (")) - continue; - - // Bail out if we reach a do-while loop - if (Token::simpleMatch(tok->previous(), "}") && Token::simpleMatch(tok->linkAt(-1)->previous(), "do")) + // TODO: Handle for loops + if (!Token::Match(tok, "while ( ! feof ( %var% )")) continue; // Bail out if we cannot identify file pointer @@ -545,21 +541,87 @@ void CheckIO::checkWrongfeofUsage() 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 *bodyStart; + const Token *bodyEnd; - 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 (Token::simpleMatch(tok->previous(), "}") && tok->previous()->scope()->type == ScopeType::eDo) { + bodyEnd = tok->previous(); + bodyStart = bodyEnd->link(); + } else { + bodyEnd = endCond->linkAt(1); + bodyStart = endCond->next(); + } - if (prevFileReadCallTok && loopFileReadCallTok && !prevControlFlowTok && !loopControlFlowTok) + // Bail out if the loop contains control flow (too complex to analyze) + if (Token::findmatch(bodyStart, "return|break|goto|continue|throw", bodyEnd)) continue; - wrongfeofUsage(getCondTok(tok)); + // 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 args = getArguments(lastLoopFileReadCallTok); + // Collect destination varIds + std::vector 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; + } + } + } } } } diff --git a/test/testio.cpp b/test/testio.cpp index 119c2d3f936..daa9e68f591 100644 --- a/test/testio.cpp +++ b/test/testio.cpp @@ -773,6 +773,7 @@ class TestIO : public TestFixture { " {\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()); @@ -795,6 +796,24 @@ class TestIO : public TestFixture { " }\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()); } From d7a37ad840b6e75477a50abc51dc088354fa5326 Mon Sep 17 00:00:00 2001 From: Francois Berder Date: Tue, 26 May 2026 21:41:33 +0200 Subject: [PATCH 6/6] fixup! fixup! fixup! fixup! fixup! Fix #958: warn when feof() is used as a while loop condition --- lib/checkio.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/checkio.cpp b/lib/checkio.cpp index cc80a53ff17..7bd6956f3f4 100644 --- a/lib/checkio.cpp +++ b/lib/checkio.cpp @@ -549,6 +549,8 @@ void CheckIO::checkWrongfeofUsage() bodyEnd = tok->previous(); bodyStart = bodyEnd->link(); } else { + if (!Token::simpleMatch(endCond, ") {")) + continue; bodyEnd = endCond->linkAt(1); bodyStart = endCond->next(); } @@ -614,8 +616,8 @@ void CheckIO::checkWrongfeofUsage() } // 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()) { + 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;