[Analyzer][StreamChecker] Added check for "indeterminate file position".

Summary:
According to the standard, after a `wread` or `fwrite` call the file position
becomes "indeterminate". It is assumable that a next read or write causes
undefined behavior, so a (fatal error) warning is added for this case.
The indeterminate position can be cleared by some operations, for example
`fseek` or `freopen`, not with `clearerr`.

Reviewers: Szelethus, baloghadamsoftware, martong, NoQ, xazax.hun, dcoughlin

Reviewed By: Szelethus

Subscribers: rnkovacs, NoQ, xazax.hun, baloghadamsoftware, szepet, a.sidorin, mikhail.ramalho, Szelethus, donat.nagy, dkrupp, gamesh411, Charusso, martong, ASDenysPetrov, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D80018

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

@@ -92,7 +92,27 @@ struct StreamState {
 
   /// State of the error flags.
   /// Ignored in non-opened stream state but must be NoError.
-  StreamErrorState ErrorState;
+  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()) &&
+           "ErrorState should be None in non-opened stream state.");
+  }
 
   bool isOpened() const { return State == Opened; }
   bool isClosed() const { return State == Closed; }
@@ -102,24 +122,27 @@ struct StreamState {
     // In not opened state error state should always NoError, so comparison
     // here is no problem.
     return LastOperation == X.LastOperation && State == X.State &&
-           ErrorState == X.ErrorState;
+           ErrorState == X.ErrorState &&
+           FilePositionIndeterminate == X.FilePositionIndeterminate;
   }
 
   static StreamState getOpened(const FnDescription *L,
-                               const StreamErrorState &ES = {}) {
-    return StreamState{L, Opened, ES};
+                               const StreamErrorState &ES = ErrorNone,
+                               bool IsFilePositionIndeterminate = false) {
+    return StreamState{L, Opened, ES, IsFilePositionIndeterminate};
   }
   static StreamState getClosed(const FnDescription *L) {
-    return StreamState{L, Closed, {}};
+    return StreamState{L, Closed, {}, false};
   }
   static StreamState getOpenFailed(const FnDescription *L) {
-    return StreamState{L, OpenFailed, {}};
+    return StreamState{L, OpenFailed, {}, false};
   }
 
   void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddPointer(LastOperation);
     ID.AddInteger(State);
     ID.AddInteger(ErrorState);
+    ID.AddBoolean(FilePositionIndeterminate);
   }
 };
 
@@ -173,7 +196,8 @@ ProgramStateRef bindInt(uint64_t Value, ProgramStateRef State,
 class StreamChecker
     : public Checker<check::PreCall, eval::Call, check::DeadSymbols> {
   mutable std::unique_ptr<BuiltinBug> BT_nullfp, BT_illegalwhence,
-      BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak, BT_StreamEof;
+      BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak, BT_StreamEof,
+      BT_IndeterminatePosition;
 
 public:
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
@@ -279,6 +303,16 @@ class StreamChecker
   ProgramStateRef ensureStreamOpened(SVal StreamVal, CheckerContext &C,
                                      ProgramStateRef State) const;
 
+  /// Check that the stream has not an invalid ("indeterminate") file position,
+  /// generate warning for it.
+  /// (EOF is not an invalid position.)
+  /// The returned state can be nullptr if a fatal error was generated.
+  /// It can return non-null state if the stream has not an invalid position or
+  /// there is execution path with non-invalid position.
+  ProgramStateRef
+  ensureNoFilePositionIndeterminate(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.
   /// Otherwise returns the state.
@@ -447,6 +481,9 @@ void StreamChecker::preFread(const FnDescription *Desc, const CallEvent &Call,
   if (!State)
     return;
   State = ensureStreamOpened(StreamVal, C, State);
+  if (!State)
+    return;
+  State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
   if (!State)
     return;
 
@@ -468,6 +505,9 @@ void StreamChecker::preFwrite(const FnDescription *Desc, const CallEvent &Call,
   if (!State)
     return;
   State = ensureStreamOpened(StreamVal, C, State);
+  if (!State)
+    return;
+  State = ensureNoFilePositionIndeterminate(StreamVal, C, State);
   if (!State)
     return;
 
@@ -548,7 +588,9 @@ void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
     NewES = (SS->ErrorState == ErrorFEof) ? ErrorFEof : ErrorFEof | ErrorFError;
   else
     NewES = ErrorFError;
-  StreamState NewState = StreamState::getOpened(Desc, NewES);
+  // 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);
   C.addTransition(StateFailed);
 }
@@ -601,9 +643,11 @@ void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
       StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
   // 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));
+      StreamState::getOpened(Desc, ErrorNone | ErrorFEof | ErrorFError, true));
 
   C.addTransition(StateNotFailed);
   C.addTransition(StateFailed);
@@ -623,7 +667,10 @@ void StreamChecker::evalClearerr(const FnDescription *Desc,
 
   assertStreamStateOpened(SS);
 
-  State = State->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
+  // FilePositionIndeterminate is not cleared.
+  State = State->set<StreamMap>(
+      StreamSym,
+      StreamState::getOpened(Desc, ErrorNone, SS->FilePositionIndeterminate));
   C.addTransition(State);
 }
 
@@ -651,15 +698,19 @@ void StreamChecker::evalFeofFerror(const FnDescription *Desc,
     // From now on it is the only one error state.
     ProgramStateRef TrueState = bindAndAssumeTrue(State, C, CE);
     C.addTransition(TrueState->set<StreamMap>(
-        StreamSym, StreamState::getOpened(Desc, ErrorKind)));
+        StreamSym, StreamState::getOpened(Desc, ErrorKind,
+                                          SS->FilePositionIndeterminate &&
+                                              !ErrorKind.isFEof())));
   }
   if (StreamErrorState NewES = SS->ErrorState & (~ErrorKind)) {
     // Execution path(s) with ErrorKind not set.
     // Function returns false.
     // New error state is everything before minus ErrorKind.
     ProgramStateRef FalseState = bindInt(0, State, C, CE);
     C.addTransition(FalseState->set<StreamMap>(
-        StreamSym, StreamState::getOpened(Desc, NewES)));
+        StreamSym,
+        StreamState::getOpened(
+            Desc, NewES, SS->FilePositionIndeterminate && !NewES.isFEof())));
   }
 }
 
@@ -767,6 +818,55 @@ ProgramStateRef StreamChecker::ensureStreamOpened(SVal StreamVal,
   return State;
 }
 
+ProgramStateRef StreamChecker::ensureNoFilePositionIndeterminate(
+    SVal StreamVal, CheckerContext &C, ProgramStateRef State) const {
+  SymbolRef Sym = StreamVal.getAsSymbol();
+  if (!Sym)
+    return State;
+
+  const StreamState *SS = State->get<StreamMap>(Sym);
+  if (!SS)
+    return State;
+
+  assert(SS->isOpened() && "First ensure that stream is opened.");
+
+  if (SS->FilePositionIndeterminate) {
+    if (!BT_IndeterminatePosition)
+      BT_IndeterminatePosition.reset(
+          new BuiltinBug(this, "Invalid stream state",
+                         "File position of the stream might be 'indeterminate' "
+                         "after a failed operation. "
+                         "Can cause undefined behavior."));
+
+    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, BT_IndeterminatePosition->getDescription(),
+          N));
+      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, BT_IndeterminatePosition->getDescription(),
+          N));
+
+    return nullptr;
+  }
+
+  return State;
+}
+
 ProgramStateRef
 StreamChecker::ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
                                         ProgramStateRef State) const {

clang/test/Analysis/stream-error.c

@@ -76,7 +76,7 @@ void error_fread() {
     }
     if (ferror(F)) {
       clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
-      fread(Buf, 1, 10, F);           // no warning
+      fread(Buf, 1, 10, F);           // expected-warning {{might be 'indeterminate'}}
     }
   }
   fclose(F);
@@ -94,7 +94,7 @@ void error_fwrite() {
   } else {
     clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
     clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
-    fwrite(0, 1, 10, F);            // no warning
+    fwrite(0, 1, 10, F);            // expected-warning {{might be 'indeterminate'}}
   }
   fclose(F);
   Ret = fwrite(0, 1, 10, F); // expected-warning {{Stream might be already closed}}
@@ -166,3 +166,70 @@ void error_fseek() {
   }
   fclose(F);
 }
+
+void error_indeterminate() {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+    return;
+  const char *Buf = "123456789";
+  int rc = fseek(F, 0, SEEK_SET);
+  if (rc) {
+    if (feof(F)) {
+      fwrite(Buf, 1, 10, F); // no warning
+    } else if (ferror(F)) {
+      fwrite(Buf, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
+    } else {
+      fwrite(Buf, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
+    }
+  }
+  fclose(F);
+}
+
+void error_indeterminate_clearerr() {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+    return;
+  const char *Buf = "123456789";
+  int rc = fseek(F, 0, SEEK_SET);
+  if (rc) {
+    if (feof(F)) {
+      clearerr(F);
+      fwrite(Buf, 1, 10, F); // no warning
+    } else if (ferror(F)) {
+      clearerr(F);
+      fwrite(Buf, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
+    } else {
+      clearerr(F);
+      fwrite(Buf, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
+    }
+  }
+  fclose(F);
+}
+
+void error_indeterminate_feof1() {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+    return;
+  char Buf[10];
+  if (fread(Buf, 1, 10, F) < 10) {
+    if (feof(F)) {
+      // error is feof, should be non-indeterminate
+      fwrite("1", 1, 1, F); // no warning
+    }
+  }
+  fclose(F);
+}
+
+void error_indeterminate_feof2() {
+  FILE *F = fopen("file", "r+");
+  if (!F)
+    return;
+  char Buf[10];
+  if (fread(Buf, 1, 10, F) < 10) {
+    if (ferror(F) == 0) {
+      // error is feof, should be non-indeterminate
+      fwrite("1", 1, 1, F); // no warning
+    }
+  }
+  fclose(F);
+}