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

@@ -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();
+    if (str.find('+', 1) != std::string::npos)
-    while (tok && ((tok = Token::findsimplematch(tok, "fflush ( stdin )")) != NULL)) {
+        return RW_MODE;
-        fflushOnInputStreamError(tok, tok->strAt(2));
+    else if (str.find('w') != std::string::npos || str.find('a') != std::string::npos)
-        tok = tok->tokAt(4);
+        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<std::string> 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<unsigned int, Filepointer> 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<unsigned int, Filepointer>::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<unsigned int, Filepointer>::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
 //---------------------------------------------------------------------------

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*/
+    /** @brief %Check usage of files*/
-    void checkFflushOnInputStream();
+    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"
+               "* Bad usage of the function 'sprintf' (overlapping data)\n"
-        "* Using fflush() on an input stream\n"
+               "* Use a file that has been closed\n"
-        "* Invalid usage of output stream. For example: std::cout << std::cout;'\n"
+               "* File input/output without positioning results in undefined behaviour\n"
-        "* Wrong number of arguments given to 'printf' or 'scanf;'\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";
     }
 };
 /// @}

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());
     }