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..7bd6956f3f4 100644 --- a/lib/checkio.cpp +++ b/lib/checkio.cpp @@ -507,6 +507,137 @@ 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()) { + // 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 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; + } + } + } + } + } +} + +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 +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); 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/man/checkers/wrongfeofUsage.md b/man/checkers/wrongfeofUsage.md new file mode 100644 index 00000000000..b893a62a95d --- /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 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 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/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): diff --git a/test/testio.cpp b/test/testio.cpp index d3344f76b8d..daa9e68f591 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,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"