[Analyzer][StreamChecker] Introduction of stream error state handling.

[balazske]

Summary

This is a way to handle stream error state in StreamChecker.
This is initial and work-in-progress,
only store of the error is implemented and create of error states
for function 'fseek'. This principle should work for other functions
and for testing if a function is called in error state.

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

50–68

Shouldn't we merge this with StreamState?

92–125

Do you have other patches that really crave the need for this class? Why isn't CallEvent::getReturnValue sufficient? This is a legitimate question, I really don't know. :)

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

50–68

The intention was that the error state is only stored when the stream is opened (in opened state). Additionally it exists in the map only if there is error, so no "NoError" kind is needed. This is only to save memory, if it is not relevant I can move the error information into StreamState (that will contain two enums then).

92–125

This is an "interesting" solution for the problem that there is need for a function with 3 return values. The constructor performs the task of the function: Create a conjured value (and get the various objects for it). The output values are RetVal and RetSym, and the success state, and the call expr that is computed here anyway. It could be computed independently but if the value was retrieved once it is better to store it for later use. (I did not check how costly that operation is.)

I had some experience that using only getReturnValue and make constraints on that does not work as intended, and the reason can be that we need to bind a value for the call expr otherwise it is an unknown (undefined?) value (and not the conjured symbol)?

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

50–68

I personally wouldn't worry about memory consumption in this case too much, considering how much information needs to be store for simple expressions, stream objects are relatively few and far in between, even on projects that use them a lot.

Having one more enum in StreamState would be better in this case then! :)

92–125

I suspect that getReturnValue might only work in postCall, but I'm not sure.

I think instead of this class, a function returning a std::tuple would be nicer, with std::tie on the call site. You seem to use all 3 returns values in the functions that instantiate MakeRetVal anyways :).

In StdLibraryFunctionsChecker's evalCall, the return value is explicitly constructed, and further constraints on it are only imposed in postCall. I wonder why that is. @martong, any idea why we don't apply the constraints for pure functions in evalCall?

383

According to the C'98 standard §7.19.9.2.5:

After determining the new position, a successful call to the fseek function undoes any effects of the ungetc function on the stream, clears the end-of-file indicator for the stream, and then establishes the new position. After a successful fseek call, the next operation on an update stream may be either input or output.

So it definitely doesn't clear the EOF flag on failure.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

92–125

The return value case is not as simple because the DefinedSVal has no default constructor, but it is not as bad to return only the RetVal and have a CE argument.

383

Yes it does say nothing about what happens with EOF flag on failure, so it should be is better to not change it. And we do not know if it is possible to get an EOF error (seek to after the end of file?).

[balazske]

Removed MakeRetVal, fixed a bug in evalFseek.

evalFseek is to be updated further.

[balazske]

Updated StreamState to include the error state.

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

40–41

Could you please move these to the individual enums please? :) I have an indexer that can query those as documentation.

46–49

These too. Also, I'm not yet sure whether we need OtherError and AnyError, as stated in a later inline.

92–125

I like the current solution very much!

369–371

If we check in preCall whether the stream is opened why don't we conservatively assume it to be open?

387–390

But why? The standard suggests that the error state isn't changed upon failure. I think we should leave it as-is.

472–473

Right here, should we just assume a stream to be opened when we can't prove otherwise? ensureStreamOpened is only called when we are about to evaluate a function that assumes the stream to be opened anyways, so I don't expect many false positive lack of fclose reports.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

369–371

If we do that we get a resource leak error for example in the test function pr8081 (there is only a call to fseek). We can assume that if the function gets the file pointer as argument it does not "own" it so the close is not needed. Otherwise the false positive resource leaks come always in functions that take a file argument and use file operations. Or this case (the file pointer is passed as input argument, the file is not opened by the function that is analyzed) can be handled specially, we can assume that there is no error initially and closing the file is not needed.

387–390

The fseek can fail and set the error flag, see the example code here:
https://en.cppreference.com/w/c/io/fseek
Probably it can not set the EOF flag, according to that page it is possible to seek after the end of the file. So the function can succeed or fail with OtherError.

472–473

The warning is created only if we know that the stream is not opened. This function makes no difference if the stream is "tracked" (found in StreamMap) or not. In the not-tracked case it is the same as if it were opened. Probably the function can be changed to take a StreamState instead of StreamVal and the not-tracked case can be handled separately. Or this function can add the new StreamState in (opened state).

[balazske]

• Moved enum comments.
• Updated fseek behaviour.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

46–49

I plan to use AnyError for failing functions that can have EOF and other error as result. At a later ferror or feof call a new state split follows to differentiate the error (instead of having to add 3 new states after the function, for success, EOF error and other error). If other operation is called we know anyway that some error happened.

369–371

This can be done in a next change. It involves more changes at other places. I think of inserting the state for the stream if it was not there before. But we need to save if this was such an insert or a normal fopen (and do not report resource leak for the "insert" case).

[Szelethus]

Okay, I think we're mostly in agreement so far -- could we implement a warning and add some test files for unchecked stream states after a failed fseek call?

The title of the revision is "[Analyzer][StreamChecker] Introduction of stream error state handling.", yet it is mostly about fseek, even if the intention is to demonstrate how error state handling would look like through its better modeling. How about we change the title to "[Analyzer][StreamChecker] Model fseek better by introducing stream error states"?

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

46–49

I think it would still be better to introduce them as we find uses for them :) Also, to stay in the scope of this patch, shouldn't we only introduce FseekError? If we did, we could make warning messages a lot clearer.

369–371

This can be done in a next change.

Consider me convinced :)

387–390

Yup, right, you won :) I tried some examples out on my system, and it could preserve or change the error state of the stream. To me it seems like that not checking the state of the stream after a failed fseek is surely unadvised.

This still should be AnyError (or FseekError), as according to the OtherError's description OtherError may not refer to EOF, yet after a failed fseek call the stream can be in EOF:

$ cat test.cpp

#include <cstdio>

int main() {
  FILE *F = fopen("test.cpp", "r");

  while (EOF != fgetc(F)) {}

  if (feof(F))
    printf("The file is closed!\n");

  // Return value is non-zero on failure.
  if (fseek(F, -100000, SEEK_END)) {
    if (feof(F))
      printf("The file is still closed!\n");
    else
      printf("The file is no longer closed!\n");
  }
}

$ clang test.cpp && ./a.out
The file is closed!
The file is still closed!

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

46–49

This change is generally about introduction of the error handling, not specifically about fseek. Probably not fseek was the best choice, it could be any other function. Probably I can add another, or multiple ones, but then again the too-big-patch problem comes up. (If now the generic error states exist the diffs for adding more stream operations could be more simple by only adding new functions and not changing the StreamState at all.) (How is this related to warning messages?)

387–390

After some experimenting I think it is best to have every possibility of errors after fseek. This means, either EOF, or other-error, or non-EOF and not other-error (but failed fseek call according to return value). So we need 1 success and 2 error return value states, one error return with AnyError and one with NoError (strange but happens according to the shown output). Probably there is relation between the previous state and the produced result of fseek but I do not want to figure out, it may be different in other systems and the documentations say nothing.

This comes from this program:

#include <stdio.h>

void print_result(FILE *F, int rc, const char *errtxt) {
  printf("--------\n%s", errtxt);
  if (rc) {
    printf("failed...\n");
    if (feof(F)) {
      printf("FEOF\n");
    }
    if (ferror(F)) {
      printf("FERROR\n");
      perror("error");
    }
  } else {
    printf("success\n");
  }
}

int main() {
  FILE *F = fopen("fseek.c", "r");

  while (EOF != fgetc(F)) {}
  print_result(F, 1, "read done\n");

  // Return value is non-zero on failure.
  int rc = fseek(F, -100000, SEEK_END);
  print_result(F, rc, "seek invalid\n");

  rc = fseek(F, 2, SEEK_END);
  print_result(F, rc, "seek valid\n");

  rc = fseek(F, -100000, SEEK_END);
  print_result(F, rc, "seek invalid\n");
  
  fputs("str", F);
  print_result(F, 1, "failed operation\n");

  rc = fseek(F, -100000, SEEK_END);
  print_result(F, rc, "seek invalid\n");

  rc = fseek(F, -1, SEEK_END);
  print_result(F, rc, "seek valid\n");

  rc = fseek(F, -100000, SEEK_END);
  print_result(F, rc, "seek invalid\n");
}

And the result is:

--------
read done
failed...
FEOF
--------
seek invalid
failed...
FEOF
--------
seek valid
success
--------
seek invalid
failed...
--------
failed operation
failed...
FERROR
error: Bad file descriptor
--------
seek invalid
failed...
FERROR
error: Invalid argument
--------
seek valid
success
--------
seek invalid
failed...
FERROR
error: Invalid argument

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

46–49

For now, the EofError and OtherError can be removed, in this change these are not used (according to how fseek will be done).

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

46–49

This change is generally about introduction of the error handling, not specifically about fseek. Probably not fseek was the best choice, it could be any other function.

You could not have picked a better function! Since the rules around the error state of the stream after a failed fseek call are quite complex, few functions deserve their own error state more.

(How is this related to warning messages?)

I had the following image in my head, it could be used at the bug report emission site to give a precise description:

if (SS->isInErrorState()) {
  switch(SS.getErrorKind) {
  case FseekError:
    reportBug("After a failed fseek call, the state of the stream may "
              "have changed, and it might be feof() or ferror()!");
    break;
  case EofError:
    reportBug("The stream is in an feof() state!");
    break;
  case Errorf:
    reportBug("The stream is in an ferror() state!");
    break;
  case OtherError:
    // We don't know what the precise error is, but we surely
    // know its in one.
    reportBug("The stream is in an error state!");
    break;
  }

(If now the generic error states exist the diffs for adding more stream operations could be more simple by only adding new functions and not changing the StreamState at all.)

For the last case in the code snippet (OtherError), I'm not too sure what the conditions are -- when do we know what the stream state is (some sort of an error), but not know precisely how? In the case of fseek, we don't precisely know what the state is but we know how it came about. I just don't yet see why we need a generic error state.

Probably I can add another, or multiple ones, but then again the too-big-patch problem comes up.

I think the point of the patch is to demonstrate the implementation of an error state, not to implement them all, and it does it quite well!

For now, the EofError and OtherError can be removed, in this change these are not used (according to how fseek will be done).

I agree!

[balazske]

To avoid problems I created a new version of this diff too that follows after the other new ones:
https://reviews.llvm.org/D75682
(Adding a new diff can make inline comment positions even more inexact specially if the diff has many differences from an older one?)

[martong]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

92–125

In StdLibraryFunctionsChecker's evalCall, the return value is explicitly constructed, and further constraints on it are only imposed in postCall. I wonder why that is. @martong, any idea why we don't apply the constraints for pure functions in evalCall?

We could apply them in evalCall technically.
I think the reason why we don't do that is the matter of implementation, and more importantly this way we are consequent with the traditional Hoare logic: {Pre}C{Post} as {Post} is done in postCall.

[Szelethus]

In D75356#1909193, @balazske wrote:

To avoid problems I created a new version of this diff too that follows after the other new ones:
https://reviews.llvm.org/D75682
(Adding a new diff can make inline comment positions even more inexact specially if the diff has many differences from an older one?)

I'd strongly prefer to finish this and move on after that -- we have the discussion here, and a great looking patch with only a few things to address.

[Szelethus]

Could you please fix up the dependencies of this revision?

[balazske]

I have "mirrored" all 3 changes in this stack to the new series in D75682. Probably it is possible to reuse these revisions instead but I do not know if it will not confuse phabricator somehow (and how phabricator behaves in such "tricky" cases, there is not a usable documentation for it). The D75682 is the one that should be used now, it is the same change as this one plus the ferror and feof functions and tests. The new part with ferror and feof can be done in a new change but these belong logically into this change to make the error handling complete and testable (this change in current form is not good for tests).

[Szelethus]

This comment was removed by Szelethus.

[Szelethus]

In D75356#1909610, @balazske wrote:

The D75682 is the one that should be used now,

If this patch is supposed to be a followup to D75682, could you please mark it as such? I find these revisions difficult to navigate.

I have "mirrored" all 3 changes in this stack to the new series in D75682. Probably it is possible to reuse these revisions instead but I do not know if it will not confuse phabricator somehow (and how phabricator behaves in such "tricky" cases, there is not a usable documentation for it).

Since this is the patch where we held the discussion about error states, I think it would be better for this revision land first, that would also solve the problem of inlines being all over the place. It doesn't really matter whether we're introducing error states first through feof and ferror, or the admittedly quirky fseek. WDYT?

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

struct StreamState {
 
   StreamState(Kind k) : K(k) {}
 
   bool isOpened() const { return K == Opened; }
   bool isClosed() const { return K == Closed; }
   bool isOpenFailed() const { return K == OpenFailed; }
   // bool isEscaped() const { return K == Escaped; }
 
   bool operator==(const StreamState &X) const { return K == X.K; }
 
   static StreamState getOpened() { return StreamState(Opened); }
   static StreamState getClosed() { return StreamState(Closed); }
   static StreamState getOpenFailed() { return StreamState(OpenFailed); }
   static StreamState getEscaped() { return StreamState(Escaped); }
 
   void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(K); }
 };
 
+struct StreamErrorState {
+  // The error state of an opened stream.
+  // EofError: EOF condition (feof returns true)
+  // OtherError: other (non-EOF) error (ferror returns true)
+  // AnyError: EofError or OtherError
+  enum Kind { EofError, OtherError, AnyError } K;
+
+  StreamErrorState(Kind k) : K(k) {}
+
+  bool isEof() const { return K == EofError; }
+  bool isOther() const { return K == OtherError; }
+  bool isAny() const { return K == AnyError; }
+
+  bool operator==(const StreamErrorState &X) const { return K == X.K; }
+
+  static StreamErrorState getOther() { return StreamErrorState(OtherError); }
+
+  void Profile(llvm::FoldingSetNodeID &ID) const { ID.AddInteger(K); }
+};
+
 class StreamChecker;
 struct FnDescription;
 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 {
   FnCheck PreFn;
   FnCheck EvalFn;
   ArgNoTy StreamArgNo;
 };
 
 /// Get the value of the stream argument out of the passed call event.
 /// The call should contain a function that is described by Desc.
 SVal getStreamArg(const FnDescription *Desc, const CallEvent &Call) {
   assert(Desc && Desc->StreamArgNo != ArgNone &&
          "Try to get a non-existing stream argument.");
   return Call.getArgSVal(Desc->StreamArgNo);
 }
 
+class MakeRetVal {
+  const CallExpr *CE = nullptr;
+  std::unique_ptr<DefinedSVal> RetVal;
+  SymbolRef RetSym;
+
+public:
+  MakeRetVal(const CallEvent &Call, CheckerContext &C)
+      : CE{dyn_cast_or_null<CallExpr>(Call.getOriginExpr())} {
+    if (!CE)
+      return;
+    SValBuilder &SVB = C.getSValBuilder();
+    const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
+    RetVal = std::make_unique<DefinedSVal>(
+        SVB.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount())
+            .castAs<DefinedSVal>());
+    RetSym = RetVal->getAsSymbol();
+    assert(RetSym && "RetVal must be a symbol here.");
+  }
+
+  operator bool() const { return CE; }
+
+  const CallExpr *getCallExpr() const {
+    assert(CE && "Return value not computed.");
+    return CE;
+  }
+  const DefinedSVal &getRetVal() const {
+    assert(RetVal && "Return value not computed.");
+    return *RetVal;
+  }
+  SymbolRef getRetSym() const {
+    assert(RetVal && "Return value not computed.");
+    return RetSym;
+  }
+};
+
 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;
 
 public:
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
 
 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}},
       {{"fread", 4}, {&StreamChecker::preDefault, nullptr, 3}},
       {{"fwrite", 4}, {&StreamChecker::preDefault, nullptr, 3}},
-      {{"fseek", 3}, {&StreamChecker::preFseek, nullptr, 0}},
+      {{"fseek", 3}, {&StreamChecker::preFseek, &StreamChecker::evalFseek, 0}},
       {{"ftell", 1}, {&StreamChecker::preDefault, nullptr, 0}},
       {{"rewind", 1}, {&StreamChecker::preDefault, nullptr, 0}},
       {{"fgetpos", 2}, {&StreamChecker::preDefault, nullptr, 0}},
       {{"fsetpos", 2}, {&StreamChecker::preDefault, nullptr, 0}},
       {{"clearerr", 1}, {&StreamChecker::preDefault, nullptr, 0}},
       {{"feof", 1}, {&StreamChecker::preDefault, nullptr, 0}},
       {{"ferror", 1}, {&StreamChecker::preDefault, nullptr, 0}},
       {{"fileno", 1}, {&StreamChecker::preDefault, nullptr, 0}},
   };
 
   void preDefault(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
-  void preFseek(const FnDescription *Desc, const CallEvent &Call,
+
+  void evalFopen(const FnDescription *Desc, const CallEvent &Call,
                  CheckerContext &C) const;
+
   void preFreopen(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
+  void evalFreopen(const FnDescription *Desc, const CallEvent &Call,
+                   CheckerContext &C) const;
 
-  void evalFopen(const FnDescription *Desc, const CallEvent &Call,
-                 CheckerContext &C) const;
-  void evalFreopen(const FnDescription *Desc, const CallEvent &Call,
-                   CheckerContext &C) const;
+  void preFseek(const FnDescription *Desc, const CallEvent &Call,
+  void evalFseek(const FnDescription *Desc, const CallEvent &Call,
+
   void evalFclose(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
 
   /// Check that the stream (in StreamVal) is not NULL.
   /// If it can only be NULL a fatal error is emitted and nullptr returned.
   /// Otherwise a new state where the stream is constrained to be non-null.
   ProgramStateRef ensureStreamNonNull(SVal StreamVal, CheckerContext &C,
                                       ProgramStateRef State) const;
if (!FD || FD->getKind() != Decl::Function)
       return nullptr;
 
     return FnDescriptions.lookup(Call);
   }
 };
 
 } // end anonymous namespace
 
+// Store the state of the stream.
 REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
 
+// Store the error state of the stream.
+// Used only for streams in opened state.
+// The entry exists only if there is error for the stream.
+REGISTER_MAP_WITH_PROGRAMSTATE(StreamErrorMap, SymbolRef, StreamErrorState)
+
 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);
 }
if (!State)
     return;
   State = ensureStreamOpened(StreamVal, C, State);
   if (!State)
     return;
 
   C.addTransition(State);
 }
 
-void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call,
-                             CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-  SVal StreamVal = getStreamArg(Desc, Call);
-  State = ensureStreamNonNull(StreamVal, C, State);
-  if (!State)
-    return;
-  State = ensureStreamOpened(StreamVal, C, State);
-  if (!State)
-    return;
-  State = ensureFseekWhenceCorrect(Call.getArgSVal(2), C, State);
-  if (!State)
-    return;
-
-  C.addTransition(State);
-}
-
-void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call,
-                               CheckerContext &C) const {
-  // Do not allow NULL as passed stream pointer but allow a closed stream.
-  ProgramStateRef State = C.getState();
-  State = ensureStreamNonNull(getStreamArg(Desc, Call), C, State);
-  if (!State)
-    return;
-
-  C.addTransition(State);
-}
-
 void StreamChecker::evalFopen(const FnDescription *Desc, const CallEvent &Call,
                               CheckerContext &C) const {
   ProgramStateRef State = C.getState();
-  SValBuilder &SVB = C.getSValBuilder();
-  const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
 
-  auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
-  if (!CE)
+  MakeRetVal RV(Call, C);
+  if (!RV)
     return;
 
-  DefinedSVal RetVal = SVB.conjureSymbolVal(nullptr, CE, LCtx, C.blockCount())
-                           .castAs<DefinedSVal>();
-  SymbolRef RetSym = RetVal.getAsSymbol();
-  assert(RetSym && "RetVal must be a symbol here.");
-
-  State = State->BindExpr(CE, C.getLocationContext(), RetVal);
+  State =
+      State->BindExpr(RV.getCallExpr(), C.getLocationContext(), RV.getRetVal());
 
   // 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);
+      C.getConstraintManager().assumeDual(State, RV.getRetVal());
 
-  StateNotNull = StateNotNull->set<StreamMap>(RetSym, StreamState::getOpened());
-  StateNull = StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed());
+  StateNotNull =
+      StateNotNull->set<StreamMap>(RV.getRetSym(), StreamState::getOpened());
+  StateNull =
+      StateNull->set<StreamMap>(RV.getRetSym(), StreamState::getOpenFailed());
 
   C.addTransition(StateNotNull);
   C.addTransition(StateNull);
 }
 
+void StreamChecker::preFreopen(const FnDescription *Desc, const CallEvent &Call,
+                               CheckerContext &C) const {
+  // Do not allow NULL as passed stream pointer but allow a closed stream.
+  ProgramStateRef State = C.getState();
+  State = ensureStreamNonNull(getStreamArg(Desc, Call), C, State);
+  if (!State)
+    return;
+
+  C.addTransition(State);
+}
+
 void StreamChecker::evalFreopen(const FnDescription *Desc,
                                 const CallEvent &Call,
                                 CheckerContext &C) const {
   ProgramStateRef State = C.getState();
 
   auto *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
   if (!CE)
     return;
 
   Optional<DefinedSVal> StreamVal =
       getStreamArg(Desc, Call).getAs<DefinedSVal>();
   if (!StreamVal)
     return;
 
   SymbolRef StreamSym = StreamVal->getAsSymbol();
   // Do not care about concrete values for stream ("(FILE *)0x12345"?).
   // FIXME: Are stdin, stdout, stderr such values?
   if (!StreamSym)
     return;
 
+  // Reset the error state.
+  // After reopen, the EOF and I/O error is not set.
+  State = State->remove<StreamErrorMap>(StreamSym);
+
   // Generate state for non-failed case.
   // Return value is the passed stream pointer.
   // According to the documentations, the stream is closed first
   // but any close error is ignored. The state changes to (or remains) opened.
   ProgramStateRef StateRetNotNull =
       State->BindExpr(CE, C.getLocationContext(), *StreamVal);
   // Generate state for NULL return value.
   // Stream switches to OpenFailed state.
   ProgramStateRef StateRetNull = State->BindExpr(CE, C.getLocationContext(),
                                                  C.getSValBuilder().makeNull());
 
   StateRetNotNull =
       StateRetNotNull->set<StreamMap>(StreamSym, StreamState::getOpened());
   StateRetNull =
       StateRetNull->set<StreamMap>(StreamSym, StreamState::getOpenFailed());
 
   C.addTransition(StateRetNotNull);
   C.addTransition(StateRetNull);
 }
 
+void StreamChecker::preFseek(const FnDescription *Desc, const CallEvent &Call,
+                             CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  SVal StreamVal = getStreamArg(Desc, Call);
+  State = ensureStreamNonNull(StreamVal, C, State);
+  if (!State)
+    return;
+  State = ensureStreamOpened(StreamVal, C, State);
+  if (!State)
+    return;
+  State = ensureFseekWhenceCorrect(Call.getArgSVal(2), C, State);
+  if (!State)
+    return;
+
+  C.addTransition(State);
+}
+
+void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
+                              CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  SymbolRef Sym = getStreamArg(Desc, Call).getAsSymbol();
+  if (!Sym)
+    return;
+
+  MakeRetVal RV(Call, C);
+  if (!RV)
+    return;
+
+  // Clear error state.
+  State = State->remove<StreamErrorMap>(Sym);
+
+  // Make expression result.
+  State =
+      State->BindExpr(RV.getCallExpr(), C.getLocationContext(), RV.getRetVal());
+
+  // Bifurcate the state into failed and non-failed.
+  // Return zero on success, nonzero on error.
+  ProgramStateRef StateNotFailed, StateFailed;
+  std::tie(StateFailed, StateNotFailed) =
+      C.getConstraintManager().assumeDual(State, RV.getRetVal());
+
+  // Record the failed status, only if failed.
+  // fseek clears the EOF flag, sets only error flag.
+  StateFailed = StateFailed->set<StreamErrorMap>(RV.getRetSym(),
+                                                 StreamErrorState::getOther());
+
+  C.addTransition(StateNotFailed);
+  C.addTransition(StateFailed);
+}
+
 void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call,
                                CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SymbolRef Sym = Call.getArgSVal(Desc->StreamArgNo).getAsSymbol();
   if (!Sym)
     return;
 
   const StreamState *SS = State->get<StreamMap>(Sym);
   if (!SS)
     return;
 
+  // Clear error state.
+  State = State->remove<StreamErrorMap>(Sym);
+
   // Close the File Descriptor.
   // Regardless if the close fails or not, stream becomes "closed"
   // and can not be used any more.
   State = State->set<StreamMap>(Sym, StreamState::getClosed());
 
   C.addTransition(State);
 }
 

 ProgramStateRef StreamChecker::ensureStreamOpened(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;
 
   if (SS->isClosed()) {
     // Using a stream pointer after 'fclose' causes undefined behavior
     // according to cppreference.com .
     ExplodedNode *N = C.generateErrorNode();
     if (N) {
       if (!BT_UseAfterClose)
         BT_UseAfterClose.reset(new BuiltinBug(this, "Closed stream",