diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -34,7 +34,7 @@ bool isOpened() const { return K == Opened; } bool isClosed() const { return K == Closed; } - //bool isOpenFailed() const { return K == OpenFailed; } + bool isOpenFailed() const { return K == OpenFailed; } //bool isEscaped() const { return K == Escaped; } bool operator==(const StreamState &X) const { return K == X.K; } @@ -74,7 +74,7 @@ class StreamChecker : public Checker { mutable std::unique_ptr BT_nullfp, BT_illegalwhence, - BT_doubleclose, BT_ResourceLeak; + BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak; public: void checkPreCall(const CallEvent &Call, CheckerContext &C) const; @@ -89,7 +89,7 @@ {&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2}}, {{"tmpfile"}, {nullptr, &StreamChecker::evalFopen, ArgNone}}, {{"fclose", 1}, - {&StreamChecker::preFclose, &StreamChecker::evalFclose, 0}}, + {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}}, {{"fread", 4}, {&StreamChecker::preDefault, nullptr, 3}}, {{"fwrite", 4}, {&StreamChecker::preDefault, nullptr, 3}}, {{"fseek", 3}, {&StreamChecker::preFseek, nullptr, 0}}, @@ -111,8 +111,6 @@ void evalFreopen(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const; - void preFclose(const FnDescription *Desc, const CallEvent &Call, - CheckerContext &C) const; void evalFclose(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const; @@ -130,12 +128,11 @@ ProgramStateRef ensureStreamNonNull(SVal StreamVal, CheckerContext &C, ProgramStateRef State) const; - /// Check that the stream is not closed. - /// Return a state where the stream is guaranteed to not in closed state - /// (if data about it exists). - /// Generate error if the stream is provable in closed state. - ProgramStateRef ensureStreamNotClosed(SVal StreamVal, CheckerContext &C, - ProgramStateRef State) const; + /// Check that the stream is the opened state. + /// If the stream is known to be not opened an error is generated + /// and nullptr returned, otherwise the original state is returned. + ProgramStateRef ensureStreamOpened(SVal StreamVal, CheckerContext &C, + ProgramStateRef State) const; /// Check the legality of the 'whence' argument of 'fseek'. /// Generate error and return nullptr if it is found to be illegal. @@ -266,16 +263,6 @@ C.addTransition(StateRetNull); } -void StreamChecker::preFclose(const FnDescription *Desc, const CallEvent &Call, - CheckerContext &C) const { - ProgramStateRef State = C.getState(); - State = ensureStreamNotClosed(getStreamArg(Desc, Call), C, State); - if (!State) - return; - - C.addTransition(State); -} - void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call, CheckerContext &C) const { ProgramStateRef State = C.getState(); @@ -300,6 +287,9 @@ ProgramStateRef State = C.getState(); SVal StreamVal = getStreamArg(Desc, Call); State = ensureStreamNonNull(StreamVal, C, State); + if (!State) + return; + State = ensureStreamOpened(StreamVal, C, State); if (!State) return; State = ensureFseekWhenceCorrect(Call.getArgSVal(2), C, State); @@ -314,6 +304,9 @@ ProgramStateRef State = C.getState(); SVal StreamVal = getStreamArg(Desc, Call); State = ensureStreamNonNull(StreamVal, C, State); + if (!State) + return; + State = ensureStreamOpened(StreamVal, C, State); if (!State) return; @@ -346,9 +339,9 @@ return StateNotNull; } -ProgramStateRef -StreamChecker::ensureStreamNotClosed(SVal StreamVal, CheckerContext &C, - ProgramStateRef State) const { +ProgramStateRef StreamChecker::ensureStreamOpened(SVal StreamVal, + CheckerContext &C, + ProgramStateRef State) const { SymbolRef Sym = StreamVal.getAsSymbol(); if (!Sym) return State; @@ -357,23 +350,43 @@ if (!SS) return State; - // Check: Double close a File Descriptor could cause undefined behaviour. - // Conforming to man-pages if (SS->isClosed()) { + // Using a stream pointer after 'fclose' causes undefined behavior + // according to cppreference.com . ExplodedNode *N = C.generateErrorNode(); if (N) { - if (!BT_doubleclose) - BT_doubleclose.reset(new BuiltinBug( - this, "Double fclose", "Try to close a file Descriptor already" - " closed. Cause undefined behaviour.")); + if (!BT_UseAfterClose) + BT_UseAfterClose.reset(new BuiltinBug(this, "Closed stream", + "Stream might be already closed. " + "Causes undefined behaviour.")); C.emitReport(std::make_unique( - *BT_doubleclose, BT_doubleclose->getDescription(), N)); + *BT_UseAfterClose, BT_UseAfterClose->getDescription(), N)); return nullptr; } return State; } + if (SS->isOpenFailed()) { + // Using a stream that has failed to open is likely to cause problems. + // This should usually not occur because stream pointer is NULL. + // But freopen can cause a state when stream pointer remains non-null but + // failed to open. + ExplodedNode *N = C.generateErrorNode(); + if (N) { + if (!BT_UseAfterOpenFailed) + BT_UseAfterOpenFailed.reset( + new BuiltinBug(this, "Invalid stream", + "Stream might be invalid after " + "(re-)opening it has failed. " + "Can cause undefined behaviour.")); + C.emitReport(std::make_unique( + *BT_UseAfterOpenFailed, BT_UseAfterOpenFailed->getDescription(), N)); + return nullptr; + } + return State; + } + return State; } diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c --- a/clang/test/Analysis/stream.c +++ b/clang/test/Analysis/stream.c @@ -3,9 +3,9 @@ typedef __typeof__(sizeof(int)) size_t; typedef __typeof__(sizeof(int)) fpos_t; typedef struct _IO_FILE FILE; -#define SEEK_SET 0 /* Seek from beginning of file. */ -#define SEEK_CUR 1 /* Seek from current position. */ -#define SEEK_END 2 /* Seek from end of file. */ +#define SEEK_SET 0 /* Seek from beginning of file. */ +#define SEEK_CUR 1 /* Seek from current position. */ +#define SEEK_END 2 /* Seek from end of file. */ extern FILE *fopen(const char *path, const char *mode); extern FILE *tmpfile(void); extern int fclose(FILE *fp); @@ -108,19 +108,56 @@ void f_double_close(void) { FILE *p = fopen("foo", "r"); - fclose(p); - fclose(p); // expected-warning {{Try to close a file Descriptor already closed. Cause undefined behaviour}} + if (!p) + return; + fclose(p); + fclose(p); // expected-warning {{Stream might be already closed}} } void f_double_close_alias(void) { FILE *p1 = fopen("foo", "r"); + if (!p1) + return; FILE *p2 = p1; fclose(p1); - fclose(p2); // expected-warning {{Try to close a file Descriptor already closed. Cause undefined behaviour}} + fclose(p2); // expected-warning {{Stream might be already closed}} +} + +void f_use_after_close(void) { + FILE *p = fopen("foo", "r"); + if (!p) + return; + fclose(p); + clearerr(p); // expected-warning {{Stream might be already closed}} +} + +void f_open_after_close(void) { + FILE *p = fopen("foo", "r"); + if (!p) + return; + fclose(p); + p = fopen("foo", "r"); + if (!p) + return; + fclose(p); +} + +void f_reopen_after_close(void) { + FILE *p = fopen("foo", "r"); + if (!p) + return; + fclose(p); + // Allow reopen after close. + p = freopen("foo", "w", p); + if (!p) + return; + fclose(p); } void f_leak(int c) { FILE *p = fopen("foo.c", "r"); + if (!p) + return; if(c) return; // expected-warning {{Opened File never closed. Potential Resource leak}} fclose(p); @@ -155,13 +192,13 @@ if (f2) { // Check if f1 and f2 point to the same stream. fclose(f1); - fclose(f2); // expected-warning {{Try to close a file Descriptor already closed. Cause undefined behaviour}} + fclose(f2); // expected-warning {{Stream might be already closed.}} } else { // Reopen failed. - // f1 points now to a possibly invalid stream but this condition is currently not checked. - // f2 is NULL. - rewind(f1); - rewind(f2); // expected-warning {{Stream pointer might be NULL}} + // f1 is non-NULL but points to a possibly invalid stream. + rewind(f1); // expected-warning {{Stream might be invalid}} + // f2 is NULL but the previous error stops the checker. + rewind(f2); } } } @@ -170,9 +207,9 @@ FILE *f1 = fopen("foo.c", "r"); if (f1) { // Unchecked result of freopen. - // The f1 may be invalid after this call (not checked by the checker). + // The f1 may be invalid after this call. freopen(0, "w", f1); - rewind(f1); + rewind(f1); // expected-warning {{Stream might be invalid}} fclose(f1); } -} +} \ No newline at end of file