balazske created this revision.
Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, gamesh411,
dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet,
baloghadamsoftware, xazax.hun, whisperity.
Herald added a reviewer: Szelethus.
balazske requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
The checker warns if a stream is read that is already in EOF state.
The commit adds indication of locations where the EOF flag is set
on the stream and where it is discovered by the program.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D104925
Files:
clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
clang/test/Analysis/stream-note.c
Index: clang/test/Analysis/stream-note.c
===================================================================
--- clang/test/Analysis/stream-note.c
+++ 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);
+}
Index: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
===================================================================
--- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ 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<void(const StreamChecker *, const FnDescription *,
const CallEvent &, CheckerContext &)>;
@@ -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<EofSeqMap>(StreamSym, 0);
+}
+
+ProgramStateRef incrementEofSeq(SymbolRef StreamSym, ProgramStateRef State) {
+ const unsigned int *Seq = State->get<EofSeqMap>(StreamSym);
+ assert(Seq && "EofSeqMap should be initialized first");
+ return State->set<EofSeqMap>(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<StreamMap>(StreamSym);
+ const StreamState *SPred = PredState->get<StreamMap>(StreamSym);
+
+ if (Mode == FEof) {
+ if (!SCurr || !SPred)
+ return nullptr;
+ const unsigned int *CurrEofSeq = CurrState->get<EofSeqMap>(StreamSym);
+ if (!CurrEofSeq)
+ return nullptr;
+ const unsigned int *PredEofSeq = PredState->get<EofSeqMap>(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<PathDiagnosticEventPiece>(
+ 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<PathDiagnosticEventPiece>(
+ 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<StreamMap>(RetSym, StreamState::getOpened(Desc));
+ StateNotNull = initEofSeq(RetSym, StateNotNull);
StateNull =
StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
@@ -566,7 +681,7 @@
if (Sym && State->get<StreamMap>(Sym)) {
const StreamState *SS = State->get<StreamMap>(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<StreamMap>(StreamSym, NewState);
+ if (IsFread && SS->ErrorState != ErrorFEof)
+ StateFailed = incrementEofSeq(StreamSym, StateFailed);
C.addTransition(StateFailed);
}
@@ -725,6 +842,7 @@
StateFailed = StateFailed->set<StreamMap>(
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<PathSensitiveBugReport>(
+ auto R = std::make_unique<PathSensitiveBugReport>(
BT_StreamEof,
"Read function called when stream is in EOF state. "
"Function has no effect.",
- N));
+ N);
+ const unsigned int *EofSeq = State->get<EofSeqMap>(StreamSym);
+ assert(EofSeq && "No EOF sequence number found for stream.");
+ R->addVisitor<StreamBugVisitor>(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<StreamChecker>();
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
