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 ee1b4e36c73..f93815ae468 100644 --- a/lib/checkio.cpp +++ b/lib/checkio.cpp @@ -481,6 +481,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 @@ -2031,6 +2093,7 @@ void CheckIO::runChecks(const Tokenizer &tokenizer, ErrorLogger *errorLogger) checkIO.checkWrongPrintfScanfArguments(); checkIO.checkCoutCerrMisusage(); checkIO.checkFileUsage(); + checkIO.checkWrongfeofUsage(); checkIO.invalidScanf(); } @@ -2045,6 +2108,7 @@ void CheckIO::getErrorMessages(ErrorLogger *errorLogger, const Settings *setting c.useClosedFileError(nullptr); 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 e37a942770b..e58854f3d57 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(); @@ -108,6 +111,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 94e79a792e3..7562a086161 100644 --- a/releasenotes.txt +++ b/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: - diff --git a/test/testio.cpp b/test/testio.cpp index b402f5c0aa1..6fd32d8c93a 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); @@ -743,6 +744,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"