[Analyzer] Improve report of "indeterminate file position" condition (alpha.unix.Stream).

[balazske]

Summary

The checker warns if specific stream operation is called in a failed state
(that is called "indeterminate file position condition").
The commit adds indication of location of the previously failed operation
that causes the failed state.

[Szelethus]

I suppose the counter is outdated, similarly to D104925#2850420?

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

101–109

Shouldn't this be all you need to retrieve the fact that the streams position is indeterminate?

[balazske]

This will be rebased on D104925 when that is finished.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

101–109

The "problem" can be that the error state can contain a combination of errors, this means FEOF and FERROR together. In this case the "indeterminate" is just not applicable to the FEOF state.

[balazske]

Adding this functionality is continued in D106262.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

enum KindTy {
     Closed, /// Closed stream (an invalid stream pointer after it was closed).
     OpenFailed /// The last open operation has failed.
   } State;
 
   /// State of the error flags.
   /// Ignored in non-opened stream state but must be NoError.
   StreamErrorState const ErrorState;
 
   /// Indicate if the file has an "indeterminate file position indicator".
   /// This can be set at a failing read or write or seek operation.
   /// If it is set no more read or write is allowed.
   /// This value is not dependent on the stream error flags:
   /// The error flag may be cleared with `clearerr` but the file position
   /// remains still indeterminate.
   /// This value applies to all error states in ErrorState except FEOF.
   /// An EOF+indeterminate state is the same as EOF state.
   bool const FilePositionIndeterminate = false;
 
   StreamState(const FnDescription *L, KindTy S, const StreamErrorState &ES,
               bool IsFilePositionIndeterminate)
       : LastOperation(L), State(S), ErrorState(ES),
         FilePositionIndeterminate(IsFilePositionIndeterminate) {
     assert((!ES.isFEof() || !IsFilePositionIndeterminate) &&
            "FilePositionIndeterminate should be false in FEof case.");
     assert((State == Opened || ErrorState.isNoError()) &&
private:
   static const ExplodedNode *getAcquisitionSite(const ExplodedNode *N,
                                                 SymbolRef StreamSym,
                                                 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 };
+  enum NotificationMode { FOpen, FEof, Indeterminate };
 
   StreamBugVisitor(SymbolRef S, NotificationMode M, unsigned int BugSeq = 0)
       : StreamSym{S}, Mode{M}, BugSeq{BugSeq} {}
 
   static void *getTag() {
     static int Tag = 0;
     return &Tag;
   }

 // 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)
+// Simillar for the "FilePositionIndeterminate" value.
+REGISTER_MAP_WITH_PROGRAMSTATE(IndeterminateSeqMap, SymbolRef, unsigned int)
 
-ProgramStateRef initEofSeq(SymbolRef StreamSym, ProgramStateRef State) {
-  return State->set<EofSeqMap>(StreamSym, 0);
+ProgramStateRef initSeq(SymbolRef StreamSym, ProgramStateRef State) {
+  State = State->set<EofSeqMap>(StreamSym, 0);
+  return State->set<IndeterminateSeqMap>(StreamSym, 0);
 }
 
 ProgramStateRef incrementEofSeq(SymbolRef StreamSym, ProgramStateRef State) {
   const unsigned int *Seq = State->get<EofSeqMap>(StreamSym);
-  assert(Seq && "EofSeqMap should be initialized first");
+  assert(Seq && "Map should be initialized first");
   return State->set<EofSeqMap>(StreamSym, *Seq + 1);
 }
 
+ProgramStateRef incrementIndeterminateSeq(SymbolRef StreamSym,
+                                          ProgramStateRef State) {
+  const unsigned int *Seq = State->get<IndeterminateSeqMap>(StreamSym);
+  assert(Seq && "Map should be initialized first");
+  return State->set<IndeterminateSeqMap>(StreamSym, *Seq + 1);
+}
+
 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) {
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;
   }
 
+  if (Mode == Indeterminate) {
+    if (!SCurr || !SPred)
+      return nullptr;
+    const unsigned int *CurrIndeterminateSeq =
+        CurrState->get<IndeterminateSeqMap>(StreamSym);
+    if (!CurrIndeterminateSeq)
+      return nullptr;
+    const unsigned int *PredIndeterminateSeq =
+        PredState->get<IndeterminateSeqMap>(StreamSym);
+    if (!PredIndeterminateSeq)
+      return nullptr;
+    if (*CurrIndeterminateSeq != BugSeq)
+      return nullptr;
+    if (*CurrIndeterminateSeq == *PredIndeterminateSeq + 1) {
+      PathDiagnosticLocation L{S, BRC.getSourceManager(),
+                               N->getLocationContext()};
+      return std::make_shared<PathDiagnosticEventPiece>(
+          L, "Assuming that this stream operation fails", true);
+    }
+    return nullptr;
+  }
+
   return nullptr;
 }
 
 //===----------------------------------------------------------------------===//
 // Methods of StreamChecker.
 //===----------------------------------------------------------------------===//
 
 void StreamChecker::checkPreCall(const CallEvent &Call,
void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
   // Bifurcate the state into two: one with a valid FILE* pointer, the other
   // with a NULL.
   ProgramStateRef StateNotNull, StateNull;
   std::tie(StateNotNull, StateNull) =
       C.getConstraintManager().assumeDual(State, RetVal);
 
   StateNotNull =
       StateNotNull->set<StreamMap>(RetSym, StreamState::getOpened(Desc));
-  StateNotNull = initEofSeq(RetSym, StateNotNull);
+  StateNotNull = initSeq(RetSym, StateNotNull);
   StateNull =
       StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed(Desc));
 
   C.addTransition(StateNotNull,
                   constructNoteTag(C, RetSym, "Stream opened here"));
   C.addTransition(StateNull);
 }
 
void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
   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);
   if (IsFread && SS->ErrorState != ErrorFEof)
     StateFailed = incrementEofSeq(StreamSym, StateFailed);
+  if (!NewES.isFEof())
+    StateFailed = incrementIndeterminateSeq(StreamSym, StateFailed);
   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,
void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
   // We get error.
   // 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));
   StateFailed = incrementEofSeq(StreamSym, StateFailed);
+  StateFailed = incrementIndeterminateSeq(StreamSym, StateFailed);
 
   C.addTransition(StateNotFailed);
   C.addTransition(StateFailed);
 }
 
 void StreamChecker::evalClearerr(const FnDescription *Desc,
                                  const CallEvent &Call,
                                  CheckerContext &C) const {
ProgramStateRef StreamChecker::ensureNoFilePositionIndeterminate(
 
   const StreamState *SS = State->get<StreamMap>(Sym);
   if (!SS)
     return State;
 
   assert(SS->isOpened() && "First ensure that stream is opened.");
 
   if (SS->FilePositionIndeterminate) {
+    const unsigned int *IndeterminateSeq = State->get<IndeterminateSeqMap>(Sym);
+    assert(IndeterminateSeq && "No failure sequence number found for stream.");
     if (SS->ErrorState & ErrorFEof) {
       // The error is unknown but may be FEOF.
       // Continue analysis with the FEOF error state.
       // Report warning because the other possible error states.
       ExplodedNode *N = C.generateNonFatalErrorNode(State);
       if (!N)
         return nullptr;
 
-      C.emitReport(std::make_unique<PathSensitiveBugReport>(
-          BT_IndeterminatePosition, BugMessage, N));
+      auto R = std::make_unique<PathSensitiveBugReport>(
+          BT_IndeterminatePosition, BugMessage, N);
+      R->addVisitor<StreamBugVisitor>(Sym, StreamBugVisitor::Indeterminate,
+                                      *IndeterminateSeq);
+      C.emitReport(std::move(R));
       return State->set<StreamMap>(
           Sym, StreamState::getOpened(SS->LastOperation, ErrorFEof, false));
     }
 
     // Known or unknown error state without FEOF possible.
     // Stop analysis, report error.
     ExplodedNode *N = C.generateErrorNode(State);
-    if (N)
-      C.emitReport(std::make_unique<PathSensitiveBugReport>(
-          BT_IndeterminatePosition, BugMessage, N));
+    if (N) {
+      auto R = std::make_unique<PathSensitiveBugReport>(
+          BT_IndeterminatePosition, BugMessage, N);
+      R->addVisitor<StreamBugVisitor>(Sym, StreamBugVisitor::Indeterminate,
+                                      *IndeterminateSeq);
+      C.emitReport(std::move(R));
+    }
 
     return nullptr;
   }
 
   return State;
 }
 
 ProgramStateRef

clang/test/Analysis/stream-note.c

void check_eof_notes_3() {
   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);
 }
+
+void check_indeterminate_notes_1() {
+  FILE *F;
+  char Buf[10];
+  F = fopen("foo1.c", "r");
+  if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}}
+    return;
+  fread(Buf, 1, 1, F);
+  if (ferror(F)) { // expected-note {{Taking true branch}}
+    F = freopen(0, "w", F);
+    if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}}
+      return;
+    fread(Buf, 1, 1, F);   // expected-note {{Assuming that this stream operation fails}}
+    if (ferror(F)) {       // expected-note {{Taking true branch}}
+      fread(Buf, 1, 1, F); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+      // expected-note@-1 {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+    }
+  }
+  fclose(F);
+}
+
+void check_indeterminate_notes_2() {
+  FILE *F;
+  char Buf[10];
+  F = fopen("foo1.c", "r");
+  if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}}
+    return;
+  fseek(F, 1, SEEK_SET); // expected-note {{Assuming that this stream operation fails}}
+  if (!feof(F))          // expected-note {{Taking true branch}}
+    fread(Buf, 1, 1, F); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+  // expected-note@-1 {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+  fclose(F);
+}
+
+void check_indeterminate_notes_3() {
+  FILE *F;
+  char Buf[10];
+  F = fopen("foo1.c", "r");
+  if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}}
+    return;
+  fwrite(Buf, 1, 1, F);  // expected-note {{Assuming that this stream operation fails}}
+  if (ferror(F))         // expected-note {{Taking true branch}}
+    fread(Buf, 1, 1, F); // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+  // expected-note@-1 {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+  fclose(F);
+}
+
+void check_indeterminate_notes_4() {
+  FILE *F;
+  char Buf[10];
+  F = fopen("foo1.c", "r");
+  if (!F) // expected-note {{Taking false branch}} expected-note {{'F' is non-null}}
+    return;
+  fseek(F, 1, SEEK_SET);      // expected-note {{Assuming that this stream operation fails}}
+  if (!ferror(F) && !feof(F)) // expected-note {{Taking true branch}} // expected-note {{Left side of '&&' is true}}
+    fread(Buf, 1, 1, F);      // expected-warning {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+  // expected-note@-1 {{File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior}}
+  fclose(F);
+}