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 @@ -25,6 +25,10 @@ using namespace ento; using namespace std::placeholders; +//===----------------------------------------------------------------------===// +// Definition of state data structures. +//===----------------------------------------------------------------------===// + namespace { struct FnDescription; @@ -146,6 +150,14 @@ } }; +} // namespace + +//===----------------------------------------------------------------------===// +// StreamChecker class and utility functions. +//===----------------------------------------------------------------------===// + +namespace { + class StreamChecker; using FnCheck = std::function; @@ -337,7 +349,8 @@ /// There will be always a state transition into the passed State, /// by the new non-fatal error node or (if failed) a normal transition, /// to ensure uniform handling. - void reportFEofWarning(CheckerContext &C, ProgramStateRef State) const; + void reportFEofWarning(SymbolRef StreamSym, CheckerContext &C, + ProgramStateRef State) const; /// Emit resource leak warnings for the given symbols. /// Createn a non-fatal error node for these, and returns it (if any warnings @@ -389,10 +402,62 @@ CheckerContext &C); }; +class StreamBugVisitor final : public BugReporterVisitor { +public: + // FIXME: Use mode FOpen to find place of opening a file, instead of NoteTag. + enum NotificationMode { FOpen, FEof }; + + StreamBugVisitor(SymbolRef S, NotificationMode M, unsigned int BugSeq = 0) + : StreamSym{S}, Mode{M}, BugSeq{BugSeq} {} + + static void *getTag() { + static int Tag = 0; + return &Tag; + } + + void Profile(llvm::FoldingSetNodeID &ID) const override { + ID.AddPointer(getTag()); + ID.AddPointer(StreamSym); + // FIXME: Other? + } + + PathDiagnosticPieceRef VisitNode(const ExplodedNode *N, + BugReporterContext &BRC, + PathSensitiveBugReport &BR) override; + +private: + // The stream symbol to be tracked. + SymbolRef StreamSym; + + // What kind of diagnostics to emit. + NotificationMode Mode; + + // Sequence number of last bug-introducing operation. + unsigned int BugSeq; +}; + } // end anonymous namespace +// This map holds the state of a stream. +// The stream is identified with a SymbolRef that is created when a stream +// opening function is modeled by the checker. REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState) +// Additional info for a stream: A value that is incermented at change of +// FEOF flag from false to true. This way it is possible to find the last +// place where the stream becomes into EOF state. +REGISTER_MAP_WITH_PROGRAMSTATE(EofSeqMap, SymbolRef, unsigned int) + +ProgramStateRef initEofSeq(SymbolRef StreamSym, ProgramStateRef State) { + return State->set(StreamSym, 0); +} + +ProgramStateRef incrementEofSeq(SymbolRef StreamSym, ProgramStateRef State) { + const unsigned int *Seq = State->get(StreamSym); + assert(Seq && "EofSeqMap should be initialized first"); + return State->set(StreamSym, *Seq + 1); +} + inline void assertStreamStateOpened(const StreamState *SS) { assert(SS->isOpened() && "Previous create of error node for non-opened stream failed?"); @@ -419,6 +484,55 @@ return nullptr; } +PathDiagnosticPieceRef StreamBugVisitor::VisitNode(const ExplodedNode *N, + BugReporterContext &BRC, + PathSensitiveBugReport &BR) { + const ExplodedNode *Pred = N->getFirstPred(); + if (!Pred) + return nullptr; + + const Stmt *S = N->getStmtForDiagnostics(); + if (!S) + return nullptr; + + ProgramStateRef CurrState = N->getState(); + ProgramStateRef PredState = Pred->getState(); + const StreamState *SCurr = CurrState->get(StreamSym); + const StreamState *SPred = PredState->get(StreamSym); + + if (Mode == FEof) { + if (!SCurr || !SPred) + return nullptr; + const unsigned int *CurrEofSeq = CurrState->get(StreamSym); + if (!CurrEofSeq) + return nullptr; + const unsigned int *PredEofSeq = PredState->get(StreamSym); + if (!PredEofSeq) + return nullptr; + if (*CurrEofSeq != BugSeq) + return nullptr; + if (*CurrEofSeq == *PredEofSeq + 1) { + PathDiagnosticLocation L{S, BRC.getSourceManager(), + N->getLocationContext()}; + return std::make_shared( + L, "Assuming that stream reaches end-of-file here", true); + } + if (SCurr->ErrorState.isFEof() && !SPred->ErrorState.isFEof()) { + PathDiagnosticLocation L{S, BRC.getSourceManager(), + N->getLocationContext()}; + return std::make_shared( + L, "End-of-file status is discovered here", true); + } + return nullptr; + } + + return nullptr; +} + +//===----------------------------------------------------------------------===// +// Methods of StreamChecker. +//===----------------------------------------------------------------------===// + void StreamChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const { const FnDescription *Desc = lookupFn(Call); @@ -461,6 +575,7 @@ StateNotNull = StateNotNull->set(RetSym, StreamState::getOpened(Desc)); + StateNotNull = initEofSeq(RetSym, StateNotNull); StateNull = StateNull->set(RetSym, StreamState::getOpenFailed(Desc)); @@ -566,7 +681,7 @@ if (Sym && State->get(Sym)) { const StreamState *SS = State->get(Sym); if (SS->ErrorState & ErrorFEof) - reportFEofWarning(C, State); + reportFEofWarning(Sym, C, State); } else { C.addTransition(State); } @@ -668,6 +783,8 @@ // indicator for the stream is indeterminate. StreamState NewState = StreamState::getOpened(Desc, NewES, !NewES.isFEof()); StateFailed = StateFailed->set(StreamSym, NewState); + if (IsFread && SS->ErrorState != ErrorFEof) + StateFailed = incrementEofSeq(StreamSym, StateFailed); C.addTransition(StateFailed); } @@ -725,6 +842,7 @@ StateFailed = StateFailed->set( StreamSym, StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true)); + StateFailed = incrementEofSeq(StreamSym, StateFailed); C.addTransition(StateNotFailed); C.addTransition(StateFailed); @@ -960,14 +1078,18 @@ return State; } -void StreamChecker::reportFEofWarning(CheckerContext &C, +void StreamChecker::reportFEofWarning(SymbolRef StreamSym, CheckerContext &C, ProgramStateRef State) const { if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) { - C.emitReport(std::make_unique( + auto R = std::make_unique( BT_StreamEof, "Read function called when stream is in EOF state. " "Function has no effect.", - N)); + N); + const unsigned int *EofSeq = State->get(StreamSym); + assert(EofSeq && "No EOF sequence number found for stream."); + R->addVisitor(StreamSym, StreamBugVisitor::FEof, *EofSeq); + C.emitReport(std::move(R)); return; } C.addTransition(State); @@ -1058,6 +1180,10 @@ return State; } +//===----------------------------------------------------------------------===// +// Checker registration. +//===----------------------------------------------------------------------===// + void ento::registerStreamChecker(CheckerManager &Mgr) { Mgr.registerChecker(); } diff --git a/clang/test/Analysis/stream-note.c b/clang/test/Analysis/stream-note.c --- a/clang/test/Analysis/stream-note.c +++ b/clang/test/Analysis/stream-note.c @@ -88,3 +88,60 @@ fclose(F); // expected-warning {{Stream pointer might be NULL}} // expected-note@-1 {{Stream pointer might be NULL}} } + +void check_eof_notes_1() { + FILE *F; + char Buf[10]; + F = fopen("foo1.c", "r"); + if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}} + return; + } + fread(Buf, 1, 1, F); + if (feof(F)) { // expected-note {{Taking true branch}} + clearerr(F); + fread(Buf, 1, 1, F); // expected-note {{Assuming that stream reaches end-of-file here}} + if (feof(F)) { // expected-note {{End-of-file status is discovered here}} expected-note {{Taking true branch}} + fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}} + // expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}} + } + } + fclose(F); +} + +void check_eof_notes_2() { + FILE *F; + char Buf[10]; + F = fopen("foo1.c", "r"); + if (F == NULL) { // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}} + return; + } + fread(Buf, 1, 1, F); + if (feof(F)) { // expected-note {{Taking false branch}} + fclose(F); + return; + } else if (ferror(F)) { // expected-note {{Taking false branch}} + fclose(F); + return; + } + fread(Buf, 1, 1, F); // expected-note {{Assuming that stream reaches end-of-file here}} + if (feof(F)) { // expected-note {{End-of-file status is discovered here}} expected-note {{Taking true branch}} + fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}} + // expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}} + } + fclose(F); +} + +void check_eof_notes_3() { + FILE *F; + char Buf[10]; + F = fopen("foo1.c", "r"); + if (F == NULL) // expected-note {{Taking false branch}} expected-note {{'F' is not equal to NULL}} + return; + int RRet = fread(Buf, 1, 1, F); // expected-note {{Assuming that stream reaches end-of-file here}} + if (ferror(F)) { // expected-note {{End-of-file status is discovered here}} expected-note {{Taking false branch}} + } else { + fread(Buf, 1, 1, F); // expected-warning {{Read function called when stream is in EOF state. Function has no effect}} + // expected-note@-1 {{Read function called when stream is in EOF state. Function has no effect}} + } + fclose(F); +}