From a8382ea5536d62d762d32672d7f644558152e7eb Mon Sep 17 00:00:00 2001 From: PKEuS Date: Tue, 22 May 2012 05:30:22 -0700 Subject: [PATCH] Implemented file pointer usage checking: - File I/O without positioning function call (#1742) - Read/Write to a file that was opened for writing/reading (#463) - Operations on closed file Old fflushOnInputStream check is now part of the new check. --- lib/checkio.cpp | 209 +++++++++++++++++++++++++++++++++++++++++++-- lib/checkio.h | 25 ++++-- test/testio.cpp | 222 ++++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 437 insertions(+), 19 deletions(-) diff --git a/lib/checkio.cpp b/lib/checkio.cpp index 0f0bc69df..6901f2611 100644 --- a/lib/checkio.cpp +++ b/lib/checkio.cpp @@ -62,22 +62,219 @@ void CheckIO::coutCerrMisusageError(const Token* tok, const std::string& streamN //--------------------------------------------------------------------------- // fflush(stdin) <- fflush only applies to output streams in ANSI C +// fread(); fwrite(); <- consecutive read/write statements require repositioning inbetween +// fopen("","r"); fwrite(); <- write to read-only file (or vice versa) +// fclose(); fread(); <- Use closed file //--------------------------------------------------------------------------- -void CheckIO::checkFflushOnInputStream() +enum OpenMode {CLOSED, READ_MODE, WRITE_MODE, RW_MODE, UNKNOWN}; +static OpenMode getMode(const std::string& str) { - const Token *tok = _tokenizer->tokens(); - while (tok && ((tok = Token::findsimplematch(tok, "fflush ( stdin )")) != NULL)) { - fflushOnInputStreamError(tok, tok->strAt(2)); - tok = tok->tokAt(4); + if (str.find('+', 1) != std::string::npos) + return RW_MODE; + else if (str.find('w') != std::string::npos || str.find('a') != std::string::npos) + return WRITE_MODE; + else if (str.find('r') != std::string::npos) + return READ_MODE; + return UNKNOWN; +} +void CheckIO::checkFileUsage() +{ + static const char* _whitelist[] = { + "clearerr", "feof", "ferror", "fgetpos", "ftell", "setbuf", "setvbuf", "ungetc" + }; + static const std::set whitelist(_whitelist, _whitelist + sizeof(_whitelist)/sizeof(*_whitelist)); + + struct Filepointer { + OpenMode mode; + unsigned int mode_indent; + enum Operation {NONE, UNIMPORTANT, READ, WRITE, POSITIONING, OPEN, CLOSE, UNKNOWN_OP} lastOperation; + unsigned int op_indent; + Filepointer(OpenMode mode_ = UNKNOWN) + : mode(mode_), mode_indent(0), lastOperation(NONE), op_indent(0) { + } + }; + std::map filepointers; + + const SymbolDatabase* symbolDatabase = _tokenizer->getSymbolDatabase(); + unsigned int varListSize = symbolDatabase->getVariableListSize(); + for (unsigned int i = 1; i < varListSize; i++) { + const Variable* var = symbolDatabase->getVariableFromVarId(i); + if (!var || !var->varId() || !Token::Match(var->typeStartToken(), "FILE *")) + continue; + + if (var->isLocal()) + filepointers.insert(std::make_pair(var->varId(), Filepointer(CLOSED))); + else { + filepointers.insert(std::make_pair(var->varId(), Filepointer(UNKNOWN))); + // TODO: If all fopen calls we find open the file in the same type, we can set Filepointer::mode + } + } + + unsigned int indent = 0; + for (const Token* tok = _tokenizer->list.front(); tok; tok = tok->next()) { + if (tok->str() == "{") + indent++; + else if (tok->str() == "}") { + indent--; + for (std::map::iterator i = filepointers.begin(); i != filepointers.end(); i++) { + if (indent < i->second.mode_indent) { + i->second.mode_indent = 0; + i->second.mode = UNKNOWN; + } + if (indent < i->second.op_indent) { + i->second.op_indent = 0; + i->second.lastOperation = Filepointer::UNKNOWN_OP; + } + } + } else if (tok->varId() && Token::Match(tok, "%var% =") && (tok->strAt(2) != "fopen" && tok->strAt(2) != "tmpfile")) { + std::map::iterator i = filepointers.find(tok->varId()); + if (i != filepointers.end()) { + i->second.mode = UNKNOWN; + i->second.lastOperation = Filepointer::UNKNOWN_OP; + } + } else if (Token::Match(tok, "%var% (")) { + std::string mode; + const Token* fileTok = 0; + Filepointer::Operation operation = Filepointer::NONE; + + if (tok->str() == "fopen" || tok->str() == "freopen" || tok->str() == "tmpfile") { + if (tok->str() != "tmpfile") { + const Token* modeTok = tok->tokAt(2)->nextArgument(); + if (modeTok) + mode = modeTok->strValue(); + } else + mode = "wb+"; + fileTok = tok->tokAt(-2); + operation = Filepointer::OPEN; + } else if (tok->str() == "rewind" || tok->str() == "fseek" || tok->str() == "fsetpos" || tok->str() == "fflush") { + if (Token::Match(tok, "fflush ( stdin )")) + fflushOnInputStreamError(tok, tok->strAt(2)); + else { + fileTok = tok->tokAt(2); + operation = Filepointer::POSITIONING; + } + } else if (tok->str() == "fgetc" || tok->str() == "fgets" || tok->str() == "fread" || tok->str() == "fscanf" || tok->str() == "getc") { + if (tok->str() == "fscanf") + fileTok = tok->tokAt(2); + else + fileTok = tok->linkAt(1)->previous(); + operation = Filepointer::READ; + } else if (tok->str() == "fputc" || tok->str() == "fputs" || tok->str() == "fwrite" || tok->str() == "fprintf" || tok->str() == "putcc") { + if (tok->str() == "fprintf") + fileTok = tok->tokAt(2); + else + fileTok = tok->linkAt(1)->previous(); + operation = Filepointer::WRITE; + } else if (tok->str() == "fclose") { + fileTok = tok->tokAt(2); + operation = Filepointer::CLOSE; + } else if (whitelist.find(tok->str()) != whitelist.end()) { + fileTok = tok->tokAt(2); + if (tok->str() == "ungetc" && fileTok) + fileTok = fileTok->nextArgument(); + operation = Filepointer::UNIMPORTANT; + } else if (!Token::Match(tok, "if|for|while|catch|return")) { + const Token* const end2 = tok->linkAt(1); + for (const Token* tok2 = tok->tokAt(2); tok2 != end2; tok2 = tok2->next()) { + if (tok2->varId() && filepointers.find(tok2->varId()) != filepointers.end()) { + fileTok = tok2; + operation = Filepointer::UNKNOWN_OP; // Assume that repositioning was last operation and that the file is opened now + break; + } + } + } + + if (!fileTok || !fileTok->varId()) + continue; + + if (filepointers.find(fileTok->varId()) == filepointers.end()) { // function call indicates: Its a File + filepointers.insert(std::make_pair(fileTok->varId(), Filepointer(UNKNOWN))); + } + Filepointer& f = filepointers[fileTok->varId()]; + + switch (operation) { + case Filepointer::OPEN: + f.mode = getMode(mode); + f.mode_indent = indent; + break; + case Filepointer::POSITIONING: + if (f.mode == CLOSED) + useClosedFileError(tok); + break; + case Filepointer::READ: + if (f.mode == CLOSED) + useClosedFileError(tok); + else if (f.mode == WRITE_MODE) + readWriteOnlyFileError(tok); + else if (f.lastOperation == Filepointer::WRITE) + ioWithoutPositioningError(tok); + break; + case Filepointer::WRITE: + if (f.mode == CLOSED) + useClosedFileError(tok); + else if (f.mode == READ_MODE) + writeReadOnlyFileError(tok); + else if (f.lastOperation == Filepointer::READ) + ioWithoutPositioningError(tok); + break; + case Filepointer::CLOSE: + if (f.mode == CLOSED) + useClosedFileError(tok); + else + f.mode = CLOSED; + f.mode_indent = indent; + break; + case Filepointer::UNIMPORTANT: + if (f.mode == CLOSED) + useClosedFileError(tok); + break; + case Filepointer::UNKNOWN_OP: + f.mode = UNKNOWN; + f.mode_indent = 0; + break; + default: + break; + } + if (operation != Filepointer::NONE && operation != Filepointer::UNIMPORTANT) { + f.op_indent = indent; + f.lastOperation = operation; + } + } } } void CheckIO::fflushOnInputStreamError(const Token *tok, const std::string &varname) { reportError(tok, Severity::error, - "fflushOnInputStream", "fflush() called on input stream \"" + varname + "\" may result in undefined behaviour"); + "fflushOnInputStream", "fflush() called on input stream \"" + varname + "\" may result in undefined behaviour."); } +void CheckIO::ioWithoutPositioningError(const Token *tok) +{ + reportError(tok, Severity::error, + "IOWithoutPositioning", "Read and write operations without a call to a positioning function (fseek, fsetpos or rewind) or fflush inbetween result in undefined behaviour."); +} + +void CheckIO::readWriteOnlyFileError(const Token *tok) +{ + + reportError(tok, Severity::error, + "readWriteOnlyFile", "Read operation on a file that was only opened for writing."); +} + +void CheckIO::writeReadOnlyFileError(const Token *tok) +{ + reportError(tok, Severity::error, + "writeReadOnlyFile", "Write operation on a file that was only opened for reading."); +} + +void CheckIO::useClosedFileError(const Token *tok) +{ + reportError(tok, Severity::error, + "useClosedFile", "Used file that is not opened."); +} + + //--------------------------------------------------------------------------- // scanf without field width limits can crash with huge input data //--------------------------------------------------------------------------- diff --git a/lib/checkio.h b/lib/checkio.h index 2807fb1a3..7700fd9b8 100644 --- a/lib/checkio.h +++ b/lib/checkio.h @@ -51,15 +51,15 @@ public: CheckIO checkIO(tokenizer, settings, errorLogger); checkIO.checkCoutCerrMisusage(); - checkIO.checkFflushOnInputStream(); + checkIO.checkFileUsage(); checkIO.invalidScanf(); } /** @brief %Check for missusage of std::cout */ void checkCoutCerrMisusage(); - /** @brief %Check for using fflush() on an input stream*/ - void checkFflushOnInputStream(); + /** @brief %Check usage of files*/ + void checkFileUsage(); /** @brief scanf can crash if width specifiers are not used */ void invalidScanf(); @@ -71,6 +71,10 @@ private: // Reporting errors.. void coutCerrMisusageError(const Token* tok, const std::string& streamName); void fflushOnInputStreamError(const Token *tok, const std::string &varname); + void ioWithoutPositioningError(const Token *tok); + void readWriteOnlyFileError(const Token *tok); + void writeReadOnlyFileError(const Token *tok); + void useClosedFileError(const Token *tok); void invalidScanfError(const Token *tok); void wrongPrintfScanfArgumentsError(const Token* tok, const std::string &function, @@ -88,6 +92,10 @@ private: c.coutCerrMisusageError(0, "cout"); c.fflushOnInputStreamError(0, "stdin"); + c.ioWithoutPositioningError(0); + c.readWriteOnlyFileError(0); + c.writeReadOnlyFileError(0); + c.useClosedFileError(0); c.invalidScanfError(0); c.wrongPrintfScanfArgumentsError(0,"printf",3,2); c.invalidScanfArgTypeError(0, "scanf", 1); @@ -104,10 +112,13 @@ private: std::string classInfo() const { return "Check input/output operations.\n" - "* Bad usage of the function 'sprintf' (overlapping data)\n" - "* Using fflush() on an input stream\n" - "* Invalid usage of output stream. For example: std::cout << std::cout;'\n" - "* Wrong number of arguments given to 'printf' or 'scanf;'\n"; + "* Bad usage of the function 'sprintf' (overlapping data)\n" + "* Use a file that has been closed\n" + "* File input/output without positioning results in undefined behaviour\n" + "* Read to a file that has only been opened for writing (or vice versa)\n" + "* Using fflush() on an input stream\n" + "* Invalid usage of output stream. For example: std::cout << std::cout;'\n" + "* Wrong number of arguments given to 'printf' or 'scanf;'\n"; } }; /// @} diff --git a/test/testio.cpp b/test/testio.cpp index 932d109ba..0a0b0bcd1 100644 --- a/test/testio.cpp +++ b/test/testio.cpp @@ -33,7 +33,11 @@ private: void run() { TEST_CASE(coutCerrMisusage); - TEST_CASE(fflushOnInputStreamTest); + TEST_CASE(wrongMode_simple); + TEST_CASE(wrongMode_complex); + TEST_CASE(useClosedFile); + TEST_CASE(fileIOwithoutPositioning); + TEST_CASE(fflushOnInputStream); TEST_CASE(testScanf1); // Scanf without field limiters TEST_CASE(testScanf2); @@ -63,7 +67,7 @@ private: // Simplify token list.. tokenizer.simplifyTokenList(); checkIO.checkCoutCerrMisusage(); - checkIO.checkFflushOnInputStream(); + checkIO.checkFileUsage(); checkIO.invalidScanf(); } @@ -111,18 +115,224 @@ private: + void wrongMode_simple() { + // Read mode + check("void foo(FILE*& f) {\n" + " f = fopen(name, \"r\");\n" + " fread(buffer, 5, 6, f);\n" + " rewind(f);\n" + " fwrite(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) Write operation on a file that was only opened for reading.\n", errout.str()); - void fflushOnInputStreamTest() { + check("void foo(FILE*& f) {\n" + " f = fopen(name, \"r+\");\n" + " fwrite(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // Write mode + check("void foo(FILE*& f) {\n" + " f = fopen(name, \"w\");\n" + " fwrite(buffer, 5, 6, f);\n" + " rewind(f);\n" + " fread(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) Read operation on a file that was only opened for writing.\n", errout.str()); + + check("void foo(FILE*& f) {\n" + " f = fopen(name, \"w+\");\n" + " fread(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // Append mode + check("void foo(FILE*& f) {\n" + " f = fopen(name, \"a\");\n" + " fwrite(buffer, 5, 6, f);\n" + " rewind(f);\n" + " fread(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) Read operation on a file that was only opened for writing.\n", errout.str()); + + check("void foo(FILE*& f) {\n" + " f = fopen(name, \"a+\");\n" + " fread(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // Variable declared locally + check("void foo() {\n" + " FILE* f = fopen(name, \"r\");\n" + " fwrite(buffer, 5, 6, f);\n" + " fclose(f);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Write operation on a file that was only opened for reading.\n", errout.str()); + + // Call unknown function + check("void foo(FILE*& f) {\n" + " f = fopen(name, \"a\");\n" + " fwrite(buffer, 5, 6, f);\n" + " bar(f);\n" + " fread(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(FILE*& f) {\n" + " f = fopen(name, \"a\");\n" + " fwrite(buffer, 5, 6, f);\n" + " clearerr(f);\n" + " fread(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("[test.cpp:5]: (error) Read operation on a file that was only opened for writing.\n", errout.str()); + + // freopen and tmpfile + check("void foo(FILE*& f) {\n" + " f = freopen(name, \"r\", f);\n" + " fwrite(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Write operation on a file that was only opened for reading.\n", errout.str()); + + check("void foo(FILE*& f) {\n" + " f = tmpfile();\n" // tmpfile opens as wb+ + " fwrite(buffer, 5, 6, f);\n" + " rewind(f);\n" + " fread(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void wrongMode_complex() { + check("void foo(FILE* f) {\n" + " if(a) f = fopen(name, \"w\");\n" + " else f = fopen(name, \"r\");\n" + " if(a) fwrite(buffer, 5, 6, f);\n" + " else fread(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo() {\n" + " FILE* f;\n" + " if(a) f = fopen(name, \"w\");\n" + " else f = fopen(name, \"r\");\n" + " if(a) fwrite(buffer, 5, 6, f);\n" + " else fread(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo() {\n" + " FILE* f = fopen(name, \"w\");\n" + " if(a) fwrite(buffer, 5, 6, f);\n" + " else fread(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) Read operation on a file that was only opened for writing.\n", errout.str()); + } + + void useClosedFile() { + check("void foo(FILE*& f) {\n" + " fclose(f);\n" + " fwrite(buffer, 5, 6, f);\n" + " clearerr(f);\n" + " fread(buffer, 5, 6, f);\n" + " ungetc('a', f);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Used file that is not opened.\n" + "[test.cpp:4]: (error) Used file that is not opened.\n" + "[test.cpp:5]: (error) Used file that is not opened.\n" + "[test.cpp:6]: (error) Used file that is not opened.\n", errout.str()); + + check("void foo(FILE*& f) {\n" + " if(!ferror(f)) {\n" + " fclose(f);\n" + " return;" + " }\n" + " fwrite(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(FILE*& f) {\n" + " fclose(f);\n" + " f = fopen(name, \"r\");\n" + " fread(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(FILE*& f) {\n" + " f = fopen(name, \"r\");\n" + " f = g;\n" + " fwrite(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } + + void fileIOwithoutPositioning() { + check("void foo(FILE* f) {\n" + " fwrite(buffer, 5, 6, f);\n" + " fread(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Read and write operations without a call to a positioning function (fseek, fsetpos or rewind) or fflush inbetween result in undefined behaviour.\n", errout.str()); + + check("void foo(FILE* f) {\n" + " fread(buffer, 5, 6, f);\n" + " fwrite(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) Read and write operations without a call to a positioning function (fseek, fsetpos or rewind) or fflush inbetween result in undefined behaviour.\n", errout.str()); + + check("void foo(FILE* f, bool read) {\n" + " if(read)\n" + " fread(buffer, 5, 6, f);\n" + " else\n" + " fwrite(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(FILE* f) {\n" + " fread(buffer, 5, 6, f);\n" + " fflush(f);\n" + " fwrite(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(FILE* f) {\n" + " fread(buffer, 5, 6, f);\n" + " rewind(f);\n" + " fwrite(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(FILE* f) {\n" + " fread(buffer, 5, 6, f);\n" + " fsetpos(f, pos);\n" + " fwrite(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(FILE* f) {\n" + " fread(buffer, 5, 6, f);\n" + " fseek(f, 0, SEEK_SET);\n" + " fwrite(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(FILE* f) {\n" + " fread(buffer, 5, 6, f);\n" + " long pos = ftell(f);\n" + " fwrite(buffer, 5, 6, f);\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (error) Read and write operations without a call to a positioning function (fseek, fsetpos or rewind) or fflush inbetween result in undefined behaviour.\n", errout.str()); + } + + void fflushOnInputStream() { check("void foo()\n" "{\n" " fflush(stdin);\n" - "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (error) fflush() called on input stream \"stdin\" may result in undefined behaviour\n", errout.str()); + "}"); + ASSERT_EQUALS("[test.cpp:3]: (error) fflush() called on input stream \"stdin\" may result in undefined behaviour.\n", errout.str()); check("void foo()\n" "{\n" " fflush(stdout);\n" - "}\n"); + "}"); ASSERT_EQUALS("", errout.str()); }