[Analyzer] Improve report of file read at end-of-file condition.

[balazske]

Summary

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.

[balazske]

Add checker name to commit message.

[NoQ]

Thanks! Why isn't this a NoteTag?

[balazske]

I could not find a way to pass the needed information from the BugReport to the NoteTag. The "sequence number" (in EofSeqMap) is only known when the bug is detected, this should be passed to the NoteTag somehow. The value is used to make the new note appear only at the last place where EOF is set.

[balazske]

Is it better to save the actual ExplodedNode in the note tag? Then the error node of the BugReport can be used to search the bug path and check if the saved node is encountered. In this way a NoteTag could be used and the "sequence numbers" would be unnecessary.

[NoQ]

In D104925#2846283, @balazske wrote:

I could not find a way to pass the needed information from the BugReport to the NoteTag. The "sequence number" (in EofSeqMap) is only known when the bug is detected, this should be passed to the NoteTag somehow.

You can capture it.

That's the whole point of note tags, to have access to all the information from both the bug report and the moment of time the event has happened.

[martong]

+1 for note tags.

clang/test/Analysis/stream-note.c

134

Perhaps we could have some more explanatory test case names. I mean how _3 is different than _2? What is the case each of them exactly tests?

141

Strictly speaking it is not necessarily and end-of-file status, ferror indicates if there was an error.

[balazske]

Using NoteTag.
Removed the EOF sequence number.
Renamed test functions.

[balazske]

Updated to use NoteTag.
Note "End-of-file status is discovered here" is removed.

clang/test/Analysis/stream-note.c

141

Exacly the ferror(F) is false here. If it is false we may have successful read or EOF after read. So the message about "Assuming that stream reaches end-of-file here" is really an assumption here (pick one from the two possible results). And if EOF happens and ferror is false the feof must be true, this is revealed when we know that ferror is false.

[NoQ]

Let's simplify this even further!

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

430–432

I guess it's a matter of preference but I really don't understand the need to define a structure when a lambda is sufficient. Lambda is the intended syntax sugar for exactly what you have written.

436–437

You can compare BugType objects directly (by address).

440–446

This code says "If there exists a state below in the path where the stream is not at EOF, drop the note". The report will not be emitted at all if the stream is not at EOF at the end, right? Are you trying to account for the situation when the stream gets reset into a non-EOF state and therefore you don't need to emit the note above that point?

In such cases the intended solution is to add another note tag to mark the stream uninteresting once it gets reset, because all the history before that point is irrelevant to our report entirely (whatever the previous position was, its get completely overwritten by the reset).

The API for marking objects uninteresting is not yet there but it's suggested in D104300. I believe you should rebase your patch on top of it and take advantage of it as it lands.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

430–432

The reason is that the NoteTag is added at more than one place. And I do not want a function that returns a lambda.

440–446

It was unknown to me if the visit of NoteTag's happens from the bug path end to begin or in different way. If it is from end to begin it may be enough to remove the interestingness in the current NoteTag function when a message is produced, it should find exactly the last place where that EOF flag is set to true. And NotePosition is not needed.

440–446

I tried it out, seems to work. But to continue D104300 should be finished or the "reset of interestingness" must be split to a separate change (or into this patch), is this possible?

[martong]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

440–446

I tried it out, seems to work. But to continue D104300 should be finished or the "reset of interestingness" must be split to a separate change (or into this patch), is this possible?

Yes, I think it would be the logical way forward if we want a smooth development with this change. So let's create a parent patch that implements solely markNotInteresting from D104300. And that new patch should be the parent of this patch, also D104300 could be rebased once the new patch of markNotInteresting is landed.

[balazske]

Using the new symbol "uninterestingness" feature.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

430–432

Problem solved: see constructSetEofNoteTag

[Szelethus]

The bug reports speak for themselves, they are awesome. Nice work!

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

28–30

Thank you! Big fan of these.

34–35

You don't seem to reuse these in this or the followup patch? Its not a big deal, I don't mind you creating them, just feels a bit odd to me.

402

Can you not capture this as well? We could eliminate StreamChecker::Instance that way I suppose.

705–711

Oh, this took me quite a bit. Can we change SS to something that reflects that its the current error state, not the new one? Like CurrSS?

[NoQ]

Looks great now!

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

409

The word "that" looks redundant, we don't use it in any other similar notes, eg. "Assuming that 'ptr' is null" etc.

[balazske]

Removed Instance, small rename, fix of warning text.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

34–35

These remained from old version of the patch, removed.

[Szelethus]

LGTM!

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include <functional>
 
 using namespace clang;
 using namespace ento;
 using namespace std::placeholders;
 
+//===----------------------------------------------------------------------===//
+// Definition of state data structures.
+//===----------------------------------------------------------------------===//
+
 namespace {
 
 struct FnDescription;
 
 /// State of the stream error flags.
 /// Sometimes it is not known to the checker what error flags are set.
 /// This is indicated by setting more than one flag to true.
 /// This is an optimization to avoid state splits.
 /// A stream can either be in FEOF or FERROR but not both at the same time.
 /// Multiple flags are set to handle the corresponding states together.
 struct StreamErrorState {
   /// The stream can be in state where none of the error flags set.
struct StreamState {
   void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddPointer(LastOperation);
     ID.AddInteger(State);
     ID.AddInteger(ErrorState);
     ID.AddBoolean(FilePositionIndeterminate);
   }
 };
 
+} // namespace
+
+//===----------------------------------------------------------------------===//
+// StreamChecker class and utility functions.
+//===----------------------------------------------------------------------===//
+
+namespace {
+
 class StreamChecker;
 using FnCheck = std::function<void(const StreamChecker *, const FnDescription *,
                                    const CallEvent &, CheckerContext &)>;
 
 using ArgNoTy = unsigned int;
 static const ArgNoTy ArgNone = std::numeric_limits<ArgNoTy>::max();
 
 struct FnDescription {
public:
   ProgramStateRef checkPointerEscape(ProgramStateRef State,
                                      const InvalidatedSymbols &Escaped,
                                      const CallEvent *Call,
                                      PointerEscapeKind Kind) const;
 
   /// If true, evaluate special testing stream functions.
   bool TestMode = false;
 
+  const BugType *getBT_StreamEof() const { return &BT_StreamEof; }
+
 private:
   CallDescriptionMap<FnDescription> FnDescriptions = {
       {{"fopen"}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
       {{"freopen", 3},
        {&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2}},
       {{"tmpfile"}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
       {{"fclose", 1},
        {&StreamChecker::preDefault, &StreamChecker::evalFclose, 0}},
private:
   /// (State is not changed here because the "whence" value is already known.)
   ProgramStateRef ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
                                            ProgramStateRef State) const;
 
   /// Generate warning about stream in EOF state.
   /// 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
   /// were generated). Return value is non-null.
   ExplodedNode *reportLeaks(const SmallVector<SymbolRef, 2> &LeakedSyms,
                             CheckerContext &C, ExplodedNode *Pred) const;
 
   /// Find the description data of the function called by a call event.
for (auto P : Call.parameters()) {
         return nullptr;
     }
 
     return FnDescriptions.lookup(Call);
   }
 
   /// Generate a message for BugReporterVisitor if the stored symbol is
   /// marked as interesting by the actual bug report.
+  // FIXME: Use lambda instead.
   struct NoteFn {
-    const CheckerNameRef CheckerName;
+    const BugType *BT_ResourceLeak;
     SymbolRef StreamSym;
     std::string Message;
 
     std::string operator()(PathSensitiveBugReport &BR) const {
-      if (BR.isInteresting(StreamSym) &&
-          CheckerName == BR.getBugType().getCheckerName())
+      if (BR.isInteresting(StreamSym) && &BR.getBugType() == BT_ResourceLeak)
         return Message;
 
       return "";
     }
   };
 
   const NoteTag *constructNoteTag(CheckerContext &C, SymbolRef StreamSym,
                                   const std::string &Message) const {
-    return C.getNoteTag(NoteFn{getCheckerName(), StreamSym, Message});
+    return C.getNoteTag(NoteFn{&BT_ResourceLeak, StreamSym, Message});
+  }
+
+  const NoteTag *constructSetEofNoteTag(CheckerContext &C,
+                                        SymbolRef StreamSym) const {
+    return C.getNoteTag([this, StreamSym](PathSensitiveBugReport &BR) {
+      if (!BR.isInteresting(StreamSym) ||
+          &BR.getBugType() != this->getBT_StreamEof())
+        return "";
+
+      BR.markNotInteresting(StreamSym);
+
+      return "Assuming stream reaches end-of-file here";
+    });
   }
 
   /// Searches for the ExplodedNode where the file descriptor was acquired for
   /// StreamSym.
   static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
                                                 SymbolRef StreamSym,
                                                 CheckerContext &C);
 };
 
 } // 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)
 
 inline void assertStreamStateOpened(const StreamState *SS) {
   assert(SS->isOpened() &&
          "Previous create of error node for non-opened stream failed?");
 }
 
 const ExplodedNode *StreamChecker::getAcquisitionSite(const ExplodedNode *N,
                                                       SymbolRef StreamSym,
                                                       CheckerContext &C) {
   ProgramStateRef State = N->getState();
   // When bug type is resource leak, exploded node N may not have state info
   // for leaked file descriptor, but predecessor should have it.
   if (!State->get<StreamMap>(StreamSym))
     N = N->getFirstPred();
 
   const ExplodedNode *Pred = N;
   while (N) {
     State = N->getState();
     if (!State->get<StreamMap>(StreamSym))
       return Pred;
     Pred = N;
     N = N->getFirstPred();
   }
 
   return nullptr;
 }
 
+//===----------------------------------------------------------------------===//
+// Methods of StreamChecker.
+//===----------------------------------------------------------------------===//
+
 void StreamChecker::checkPreCall(const CallEvent &Call,
                                  CheckerContext &C) const {
   const FnDescription *Desc = lookupFn(Call);
   if (!Desc || !Desc->PreFn)
     return;
 
   Desc->PreFn(this, Desc, Call, C);
 }
void StreamChecker::preFread(const FnDescription *Desc, const CallEvent &Call,
   State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
   if (!State)
     return;
 
   SymbolRef Sym = StreamVal.getAsSymbol();
   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);
   }
 }
 
 void StreamChecker::preFwrite(const FnDescription *Desc, const CallEvent &Call,
                               CheckerContext &C) const {
   ProgramStateRef State = C.getState();
void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
 
   Optional<NonLoc> SizeVal = Call.getArgSVal(1).getAs<NonLoc>();
   if (!SizeVal)
     return;
   Optional<NonLoc> NMembVal = Call.getArgSVal(2).getAs<NonLoc>();
   if (!NMembVal)
     return;
 
-  const StreamState *SS = State->get<StreamMap>(StreamSym);
-  if (!SS)
+  const StreamState *OldSS = State->get<StreamMap>(StreamSym);
+  if (!OldSS)
     return;
 
-  assertStreamStateOpened(SS);
+  assertStreamStateOpened(OldSS);
 
   // C'99 standard, §7.19.8.1.3, the return value of fread:
   // The fread function returns the number of elements successfully read, which
   // may be less than nmemb if a read error or end-of-file is encountered. If
   // size or nmemb is zero, fread returns zero and the contents of the array and
   // the state of the stream remain unchanged.
 
   if (State->isNull(*SizeVal).isConstrainedTrue() ||
       State->isNull(*NMembVal).isConstrainedTrue()) {
     // This is the "size or nmemb is zero" case.
     // Just return 0, do nothing more (not clear the error flags).
     State = bindInt(0, State, C, CE);
     C.addTransition(State);
     return;
   }
 
   // Generate a transition for the success state.
   // If we know the state to be FEOF at fread, do not add a success state.
-  if (!IsFread || (SS->ErrorState != ErrorFEof)) {
+  if (!IsFread || (OldSS->ErrorState != ErrorFEof)) {
     ProgramStateRef StateNotFailed =
         State->BindExpr(CE, C.getLocationContext(), *NMembVal);
     if (StateNotFailed) {
       StateNotFailed = StateNotFailed->set<StreamMap>(
           StreamSym, StreamState::getOpened(Desc));
       C.addTransition(StateNotFailed);
     }
   }
void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
   if (!Cond)
     return;
   StateFailed = StateFailed->assume(*Cond, true);
   if (!StateFailed)
     return;
 
   StreamErrorState NewES;
   if (IsFread)
-    NewES = (SS->ErrorState == ErrorFEof) ? ErrorFEof : ErrorFEof | ErrorFError;
+    NewES =
+        (OldSS->ErrorState == ErrorFEof) ? ErrorFEof : ErrorFEof | ErrorFError;
   else
     NewES = ErrorFError;
   // If a (non-EOF) error occurs, the resulting value of the file position
   // indicator for the stream is indeterminate.
-  StreamState NewState = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
-  StateFailed = StateFailed->set<StreamMap>(StreamSym, NewState);
+  StreamState NewSS = StreamState::getOpened(Desc, NewES, !NewES.isFEof());
+  StateFailed = StateFailed->set<StreamMap>(StreamSym, NewSS);
+  if (IsFread && OldSS->ErrorState != ErrorFEof)
+    C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
+  else
     C.addTransition(StateFailed);
 }
 
 void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call,
                              CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SVal StreamVal = getStreamArg(Desc, Call);
   State = ensureStreamNonNull(StreamVal, Call.getArgExpr(Desc->StreamArgNo), C,
                               State);
   if (!State)
void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
   // It is possible that fseek fails but sets none of the error flags.
   // If fseek failed, assume that the file position becomes indeterminate in any
   // case.
   StateFailed = StateFailed->set<StreamMap>(
       StreamSym,
       StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true));
 
   C.addTransition(StateNotFailed);
-  C.addTransition(StateFailed);
+  C.addTransition(StateFailed, constructSetEofNoteTag(C, StreamSym));
 }
 
 void StreamChecker::evalClearerr(const FnDescription *Desc,
                                  const CallEvent &Call,
                                  CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   if (!StreamSym)
C.emitReport(std::make_unique<PathSensitiveBugReport>(
         "SEEK_SET, SEEK_END, or SEEK_CUR.",
         N));
     return nullptr;
   }
 
   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);
+    R->markInteresting(StreamSym);
+    C.emitReport(std::move(R));
     return;
   }
   C.addTransition(State);
 }
 
 ExplodedNode *
 StreamChecker::reportLeaks(const SmallVector<SymbolRef, 2> &LeakedSyms,
                            CheckerContext &C, ExplodedNode *Pred) const {
for (SymbolRef Sym : Escaped) {
     // somewhere else.
     // Remove symbol from state so the following stream calls on this symbol are
     // not handled by the checker.
     State = State->remove<StreamMap>(Sym);
   }
   return State;
 }
 
+//===----------------------------------------------------------------------===//
+// Checker registration.
+//===----------------------------------------------------------------------===//
+
 void ento::registerStreamChecker(CheckerManager &Mgr) {
   Mgr.registerChecker<StreamChecker>();
 }
 
 bool ento::shouldRegisterStreamChecker(const CheckerManager &Mgr) {
   return true;
 }
 
 void ento::registerStreamTesterChecker(CheckerManager &Mgr) {
   auto *Checker = Mgr.getChecker<StreamChecker>();
   Checker->TestMode = true;
 }
 
 bool ento::shouldRegisterStreamTesterChecker(const CheckerManager &Mgr) {
   return true;
-}
+}
 No newline at end of file

clang/test/Analysis/stream-note.c

void check_track_null() {
   F = fopen("foo1.c", "r"); // expected-note {{Value assigned to 'F'}} expected-note {{Assuming pointer value is null}}
   if (F != NULL) {          // expected-note {{Taking false branch}} expected-note {{'F' is equal to NULL}}
     fclose(F);
     return;
   }
   fclose(F); // expected-warning {{Stream pointer might be NULL}}
   // expected-note@-1 {{Stream pointer might be NULL}}
 }
+
+void check_eof_notes_feof_after_feof() {
+  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 stream reaches end-of-file here}}
+    if (feof(F)) {         // 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_feof_after_no_feof() {
+  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 stream reaches end-of-file here}}
+  if (feof(F)) {         // 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_feof_or_no_error() {
+  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 stream reaches end-of-file here}}
+  if (ferror(F)) {                // 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);
+}