[analyzer][StreamChecker] Adding precall and refactoring.

[balazske]

Summary

The validity checks are performed in precall callback.

Semantic of checking for opened stream has changed:
It is not allowed at all to use the stream after 'fclose'.
This is what cppreference.com says at fclose.
So the double-close error changes to use-after-close.

[balazske]

Code is not reformatted and tests are to be added.

[Szelethus]

Aha, so we're moving every check as to whether the stream is closed to preCall? Makes sense, if not much else changed. Could you please run clang-format-diff.py after adding the tests?

[balazske]

Reformat code and fixed errors.

[balazske]

Really reformat, bugfixes, and updated tests.

[balazske]

Plan:

• Add an error state to StreamState.
• Model every stream function that can fail. Split the state into failed and non-failed and set the return value accordingly (as done at fopen now, but not only for stream return values). This is needed here to have a correlation between the return value and the stream state.

Probably the check for null stream pointer can be removed from here if the StdLibraryFunctionsChecker will do this job.

[Szelethus]

To summarize, we're moving every stream check (illegal call to a stream management function with a null stream, or a closed stream, etc) to preCall, and leave only the modeling portions in evalCall. Makes sense!

You did an amazing job here! I like the new infrastructure and everything.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

84–101

Hmm, all of these braces are kinda hard to navigate, but its not too bad. Could you check whether changing them to nullptr improves readability? I'll leave it to your judgement.

117–122

Could we add/move some comments for these? :)

Checks whether the stream is non-null. If it is null, an fatal bug report is emitted and the return value is nullptr. Otherwise in the returned state StreamVal is non-null.

180

This would assert in Expr::getArg if Desc->StreamArgNo == ArgNone, but that shouldn't ever happen in this function, correct? Could we add an assert here as well as a poor man's documentation?

222–224

According to my research, yes, closed streams are okay. Also, the rest of the comment can be removed as well -- it is for sure not okay to pass NULL to freopen, even if it doesn't crash on some systems.

300–304

Ah, we're moving the closing of the stream from checkDoubleClose to here. Makes much more sense!

359–363

Could we move this to the declaration and start it with ///?

367–373

Previously, we only returned a nullptr after generating a fatal error node. Why do we need to change this?

378–379

How about

"Closing an already closed file descriptor."

395–396

How about

"Using a file descriptor after (re-)opening it failed."

[balazske]

• Improved comments in the code.
• Added function to get stream arg.
• Changed error messages.

[balazske]

Error messages are now in similar form as the null stream pointer case.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

84–101

nullptr looks better. (Missing value at "fseek" will be updated.)

367–373

This is fixed.

[balazske]

Added a missing argument, lint warning reformat.

[Szelethus]

LGTM! It makes much more sense to do the checking in preCall and the modeling in evalCall, the comments in the patch are precise and helpful, the tests cover everything. While I feel fairly confident that this patch is great both in concept and in execution, please leave some time for others to respond -- if that doesn't happen in a day or two, I think this is ready to land.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

64–74

You could make this a method of FnDescription.

119

/// If it can only be NULL a fatal error is emitted and nullptr returned.
/// Otherwise the return value is a new state where the stream is constrained to be non-null.

130

/// Check that the stream is in an opened state.

148–151

I vaguely recall us having this conversation once, but isn't this redundant with

if (!Call.isGlobalCFunction())
  return nullptr;

? I don't mind not addressing this before commiting.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

64–74

Originally I wanted it too but there is no strong relation to it, somehow I like better to have it as separate function.

148–151

According to the documentation isGlobalCFunction should filter out member functions.

[balazske]

Comment changes, updated lookupFn, other small change.

[xazax.hun]

If we were to refactor this check I wonder if it would make sense to port evalCall to postCall, so the analyzer engine will conjure the symbol for us.
I wonder what @NoQ thinks about the pros and cons of the approaches.

As far as I understand the con of evalCall is that there is only one check that can "win" to do the modelling. Also, it is error prone as everything including binding the return values and invalidating some state needs to be done by hand. If all we need to do is to record some state for the returned symbol, postCall should be sufficient. But this can be problematic when we see the body of the function and hence we might not see a returned symbol (but a constant or something else).

The name of the patch implies refactoring but some functional changes were also done. Is it possible to separate the functional changes into a separate patch? If it is not a big effort I would prefer that way.

[balazske]

In D75163#1902816, @xazax.hun wrote:

If we were to refactor this check I wonder if it would make sense to port evalCall to postCall, so the analyzer engine will conjure the symbol for us.
I wonder what @NoQ thinks about the pros and cons of the approaches.

Once I wanted to remove evalCall but it was no success:
https://reviews.llvm.org/D69662

[balazske]

Split the patch into two is really start it from "scratch" again, copy the code, decide what to go into the first or second part (and what order) (really what?), make some mess with git commands and branches and rebase, probably rebuild whole llvm, if review changes are to be done again comes the "git mess" problem (change code in a commit that belongs logically before another). It can be more work to make changes in the "old way" that are done in the second part in a new way. Or somebody could say "this thing makes no sense, I want to see the next diff how about it is used", and commit can happen really only if the next change is accepted too. So I do not prefer to split it, the change is not a very complicated one and the modifications belong somewhat together.

[xazax.hun]

In D75163#1902921, @balazske wrote:

In D75163#1902816, @xazax.hun wrote:

If we were to refactor this check I wonder if it would make sense to port evalCall to postCall, so the analyzer engine will conjure the symbol for us.
I wonder what @NoQ thinks about the pros and cons of the approaches.

Once I wanted to remove evalCall but it was no success:
https://reviews.llvm.org/D69662

Sounds fair.

[Szelethus]

In D75163#1902816, @xazax.hun wrote:

The name of the patch implies refactoring but some functional changes were also done. Is it possible to separate the functional changes into a separate patch? If it is not a big effort I would prefer that way.

It would indeed be nicer, but given that the patch is relatively small both in LOC and in scope, I think its okay. In the future, I would also prefer to separate functional and non-functional changes.

[balazske]

I had no other work to do so here is the change for the non-functional part of this:
https://reviews.llvm.org/D75612

[balazske]

And the second part:
https://reviews.llvm.org/D75614

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

64–74

Okay, I'll leave that to you.

[balazske]

This comment was removed by balazske.

[Szelethus]

Okay, very well, then lets abandon this patch. Please keep in mind that phabricator can mark whether a revision was mentioned somewhere else if you only copy the revision ID, not the entire link: D75163 vs. https://reviews.llvm.org/D75163.

[balazske]

The patch was split into D75612 and D75614.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

 
 struct StreamState {
   enum Kind { Opened, Closed, OpenFailed, Escaped } K;
 
   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 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);
-  }
+  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 FnCheck = std::function<void(const StreamChecker *, const CallEvent &,
-                                   CheckerContext &)>;
+using ArgNoTy = unsigned int;
+static const ArgNoTy ArgNone = std::numeric_limits<ArgNoTy>::max();
 
 struct FnDescription {
+  FnCheck PreFn;
   FnCheck EvalFn;
+  ArgNoTy StreamArgNo;
 };
 
-class StreamChecker : public Checker<eval::Call,
-                                     check::DeadSymbols > {
+/// 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 StreamChecker
+    : public Checker<check::PreCall, eval::Call, check::DeadSymbols> {
   mutable std::unique_ptr<BuiltinBug> BT_nullfp, BT_illegalwhence,
-      BT_doubleclose, BT_ResourceLeak;
+      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"}, {&StreamChecker::evalFopen}},
-      {{"freopen", 3}, {&StreamChecker::evalFreopen}},
-      {{"tmpfile"}, {&StreamChecker::evalFopen}},
-      {{"fclose", 1}, {&StreamChecker::evalFclose}},
-      {{"fread", 4},
-       {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 3)}},
-      {{"fwrite", 4},
-       {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 3)}},
-      {{"fseek", 3}, {&StreamChecker::evalFseek}},
-      {{"ftell", 1},
-       {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
-      {{"rewind", 1},
-       {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
-      {{"fgetpos", 2},
-       {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
-      {{"fsetpos", 2},
-       {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
-      {{"clearerr", 1},
-       {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
-      {{"feof", 1},
-       {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
-      {{"ferror", 1},
-       {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
-      {{"fileno", 1},
-       {std::bind(&StreamChecker::checkArgNullStream, _1, _2, _3, 0)}},
+      {{"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}},
+      {{"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 evalFopen(const CallEvent &Call, CheckerContext &C) const;
-  void evalFreopen(const CallEvent &Call, CheckerContext &C) const;
-  void evalFclose(const CallEvent &Call, CheckerContext &C) const;
-  void evalFseek(const CallEvent &Call, CheckerContext &C) const;
-  void checkArgNullStream(const CallEvent &Call, CheckerContext &C,
-                          unsigned ArgI) const;
-
-  ProgramStateRef checkNullStream(SVal SV, CheckerContext &C,
+  void preDefault(const FnDescription *Desc, const CallEvent &Call,
+                  CheckerContext &C) const;
+  void preFseek(const FnDescription *Desc, const CallEvent &Call,
+                CheckerContext &C) const;
+  void preFreopen(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 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 the return value is a new state where the stream is constrained
+  /// to be non-null.
+  ProgramStateRef ensureStreamNonNull(SVal StreamVal, CheckerContext &C,
                                       ProgramStateRef State) const;
-  ProgramStateRef checkFseekWhence(SVal SV, CheckerContext &C,
+
+  /// 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.
+  /// (State is not changed here because the "whence" value is already known.)
+  ProgramStateRef ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
                                            ProgramStateRef State) const;
-  ProgramStateRef checkDoubleClose(const CallEvent &Call, CheckerContext &C,
+
+  /// Check that the stream is the opened state.
+  /// If the stream is known to be not opened an error is generated
+  /// and nullptr returned, otherwise the original state is returned.
+  ProgramStateRef ensureStreamOpened(SVal StreamVal, CheckerContext &C,
                                      ProgramStateRef State) const;
+
+  /// Find the description data of the function called by a call event.
+  /// Returns nullptr if no function is recognized.
+  const FnDescription *lookupFn(const CallEvent &Call) const {
+    // Recognize "global C functions" with only integral or pointer arguments
+    // (and matching name) as stream functions.
+    if (!Call.isGlobalCFunction())
+      return nullptr;
+    for (auto P : Call.parameters()) {
+      QualType T = P->getType();
+      if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
+        return nullptr;
+    }
+
+    return FnDescriptions.lookup(Call);
+  }
 };
 
 } // end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
 
+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);
+}
 
 bool StreamChecker::evalCall(const CallEvent &Call, CheckerContext &C) const {
-  const auto *FD = dyn_cast_or_null<FunctionDecl>(Call.getDecl());
-  if (!FD || FD->getKind() != Decl::Function)
+  const FnDescription *Desc = lookupFn(Call);
+  if (!Desc || !Desc->EvalFn)
     return false;
 
-  // Recognize "global C functions" with only integral or pointer arguments
-  // (and matching name) as stream functions.
-  if (!Call.isGlobalCFunction())
-    return false;
-  for (auto P : Call.parameters()) {
-    QualType T = P->getType();
-    if (!T->isIntegralOrEnumerationType() && !T->isPointerType())
-      return false;
-  }
+  Desc->EvalFn(this, Desc, Call, C);
+
+  return C.isDifferent();
 
-  const FnDescription *Description = FnDescriptions.lookup(Call);
-  if (!Description)
-    return false;
+void StreamChecker::preDefault(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;
 
-  (Description->EvalFn)(this, Call, C);
+  C.addTransition(State);
+}
 
-  return C.isDifferent();
+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 CallEvent &Call, CheckerContext &C) const {
+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)
     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);
 
   // 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());
   StateNull = StateNull->set<StreamMap>(RetSym, StreamState::getOpenFailed());
 
   C.addTransition(StateNotNull);
   C.addTransition(StateNull);
 }
 
-void StreamChecker::evalFreopen(const CallEvent &Call,
+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 = Call.getArgSVal(2).getAs<DefinedSVal>();
+  Optional<DefinedSVal> StreamVal =
+      getStreamArg(Desc, Call).getAs<DefinedSVal>();
   if (!StreamVal)
     return;
-  // Do not allow NULL as passed stream pointer.
-  // This is not specified in the man page but may crash on some system.
-  State = checkNullStream(*StreamVal, C, State);
-  if (!State)
-    return;
 
   SymbolRef StreamSym = StreamVal->getAsSymbol();
-  // Do not care about special values for stream ("(FILE *)0x12345"?).
+  // Do not care about concrete values for stream ("(FILE *)0x12345"?).
+  // FIXME: Are stdin, stdout, stderr such values?
   if (!StreamSym)
     return;
 
   // 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::evalFclose(const CallEvent &Call, CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-  State = checkDoubleClose(Call, C, State);
-  if (State)
-    C.addTransition(State);
-}
-
-void StreamChecker::evalFseek(const CallEvent &Call, CheckerContext &C) const {
-  const Expr *AE2 = Call.getArgExpr(2);
-  if (!AE2)
-    return;
-
+void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call,
+                               CheckerContext &C) const {
   ProgramStateRef State = C.getState();
-
-  State = checkNullStream(Call.getArgSVal(0), C, State);
-  if (!State)
+  SymbolRef Sym = getStreamArg(Desc, Call).getAsSymbol();
+  if (!Sym)
     return;
 
-  State =
-      checkFseekWhence(State->getSVal(AE2, C.getLocationContext()), C, State);
-  if (!State)
+  const StreamState *SS = State->get<StreamMap>(Sym);
+  if (!SS)
     return;
 
-  C.addTransition(State);
-}
+  // 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());
 
-void StreamChecker::checkArgNullStream(const CallEvent &Call, CheckerContext &C,
-                                       unsigned ArgI) const {
-  ProgramStateRef State = C.getState();
-  State = checkNullStream(Call.getArgSVal(ArgI), C, State);
-  if (State)
-    C.addTransition(State);
 }
 
-ProgramStateRef StreamChecker::checkNullStream(SVal SV, CheckerContext &C,
-                                               ProgramStateRef State) const {
-  Optional<DefinedSVal> DV = SV.getAs<DefinedSVal>();
-  if (!DV)
+ProgramStateRef
+StreamChecker::ensureStreamNonNull(SVal StreamVal, CheckerContext &C,
+  auto Stream = StreamVal.getAs<DefinedSVal>();
+  if (!Stream)
     return State;
 
   ConstraintManager &CM = C.getConstraintManager();
+
   ProgramStateRef StateNotNull, StateNull;
-  std::tie(StateNotNull, StateNull) = CM.assumeDual(C.getState(), *DV);
+  std::tie(StateNotNull, StateNull) = CM.assumeDual(C.getState(), *Stream);
 
   if (!StateNotNull && StateNull) {
     if (ExplodedNode *N = C.generateErrorNode(StateNull)) {
       if (!BT_nullfp)
         BT_nullfp.reset(new BuiltinBug(this, "NULL stream pointer",
                                        "Stream pointer might be NULL."));
       C.emitReport(std::make_unique<PathSensitiveBugReport>(
           *BT_nullfp, BT_nullfp->getDescription(), N));
     }
     return nullptr;
   }
 
   return StateNotNull;
 }
 
-// Check the legality of the 'whence' argument of 'fseek'.
-ProgramStateRef StreamChecker::checkFseekWhence(SVal SV, CheckerContext &C,
-                                                ProgramStateRef State) const {
-  Optional<nonloc::ConcreteInt> CI = SV.getAs<nonloc::ConcreteInt>();
+ProgramStateRef
+StreamChecker::ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
+  Optional<nonloc::ConcreteInt> CI = WhenceVal.getAs<nonloc::ConcreteInt>();
   if (!CI)
     return State;
 
   int64_t X = CI->getValue().getSExtValue();
   if (X >= 0 && X <= 2)
     return State;
 
   if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
     if (!BT_illegalwhence)
       BT_illegalwhence.reset(
           new BuiltinBug(this, "Illegal whence argument",
                          "The whence argument to fseek() should be "
                          "SEEK_SET, SEEK_END, or SEEK_CUR."));
     C.emitReport(std::make_unique<PathSensitiveBugReport>(
         *BT_illegalwhence, BT_illegalwhence->getDescription(), N));
     return nullptr;
   }
 
   return State;
 }
 
-ProgramStateRef StreamChecker::checkDoubleClose(const CallEvent &Call,
+ProgramStateRef StreamChecker::ensureStreamOpened(SVal StreamVal,
                                                   CheckerContext &C,
                                                   ProgramStateRef State) const {
-  SymbolRef Sym = Call.getArgSVal(0).getAsSymbol();
+  SymbolRef Sym = StreamVal.getAsSymbol();
   if (!Sym)
     return State;
 
   const StreamState *SS = State->get<StreamMap>(Sym);
-
-  // If the file stream is not tracked, return.
   if (!SS)
     return State;
 
-  // Check: Double close a File Descriptor could cause undefined behaviour.
-  // Conforming to man-pages
   if (SS->isClosed()) {
+    // Using a stream pointer after 'fclose' causes undefined behavior
+    // according to cppreference.com .
     ExplodedNode *N = C.generateErrorNode();
     if (N) {
-      if (!BT_doubleclose)
-        BT_doubleclose.reset(new BuiltinBug(
-            this, "Double fclose", "Try to close a file Descriptor already"
-                                   " closed. Cause undefined behaviour."));
+      if (!BT_UseAfterClose)
+        BT_UseAfterClose.reset(new BuiltinBug(this, "Closed stream",
+                                              "Stream might be already closed. "
+                                              "Causes undefined behaviour."));
       C.emitReport(std::make_unique<PathSensitiveBugReport>(
-          *BT_doubleclose, BT_doubleclose->getDescription(), N));
+          *BT_UseAfterClose, BT_UseAfterClose->getDescription(), N));
       return nullptr;
     }
-
     return State;
   }
 
-  // Close the File Descriptor.
-  State = State->set<StreamMap>(Sym, StreamState::getClosed());
+  if (SS->isOpenFailed()) {
+    // Using a stream that has failed to open is likely to cause problems.
+    // This should usually not occur because stream pointer is NULL.
+    // But freopen can cause a state when stream pointer remains non-null but
+    // failed to open.
+    ExplodedNode *N = C.generateErrorNode();
+    if (N) {
+      if (!BT_UseAfterOpenFailed)
+        BT_UseAfterOpenFailed.reset(
+            new BuiltinBug(this, "Invalid stream",
+                           "Stream might be invalid after "
+                           "(re-)opening it has failed. "
+                           "Can cause undefined behaviour."));
+      C.emitReport(std::make_unique<PathSensitiveBugReport>(
+          *BT_UseAfterOpenFailed, BT_UseAfterOpenFailed->getDescription(), N));
+      return nullptr;
+    }
+
+    return State;
+  }
 
   return State;
 }
 
 void StreamChecker::checkDeadSymbols(SymbolReaper &SymReaper,
                                      CheckerContext &C) const {
   ProgramStateRef State = C.getState();
 
   // TODO: Clean up the state.
   const StreamMapTy &Map = State->get<StreamMap>();
-  for (const auto &I: Map) {
+  for (const auto &I : Map) {
     SymbolRef Sym = I.first;
     const StreamState &SS = I.second;
     if (!SymReaper.isDead(Sym) || !SS.isOpened())
       continue;
 
     ExplodedNode *N = C.generateErrorNode();
     if (!N)
       continue;
 
     if (!BT_ResourceLeak)
       BT_ResourceLeak.reset(
           new BuiltinBug(this, "Resource Leak",
                          "Opened File never closed. Potential Resource leak."));
     C.emitReport(std::make_unique<PathSensitiveBugReport>(
         *BT_ResourceLeak, BT_ResourceLeak->getDescription(), N));
   }
 }
 
 void ento::registerStreamChecker(CheckerManager &mgr) {
   mgr.registerChecker<StreamChecker>();
 }
 
-bool ento::shouldRegisterStreamChecker(const LangOptions &LO) {
-  return true;
-}
+bool ento::shouldRegisterStreamChecker(const LangOptions &LO) { return true; }

clang/test/Analysis/stream.c

if (!p)
     return;
   fseek(p, 1, SEEK_SET); // no-warning
   fseek(p, 1, 3); // expected-warning {{The whence argument to fseek() should be SEEK_SET, SEEK_END, or SEEK_CUR}}
   fclose(p);
 }
 
 void f_double_close(void) {
   FILE *p = fopen("foo", "r");
-  fclose(p); 
-  fclose(p); // expected-warning {{Try to close a file Descriptor already closed. Cause undefined behaviour}}
+  if (!p)
+    return;
+  fclose(p);
+  fclose(p); // expected-warning {{Stream might be already closed}}
 }
 
 void f_double_close_alias(void) {
   FILE *p1 = fopen("foo", "r");
+  if (!p1)
+    return;
   FILE *p2 = p1;
   fclose(p1);
-  fclose(p2); // expected-warning {{Try to close a file Descriptor already closed. Cause undefined behaviour}}
+  fclose(p2); // expected-warning {{Stream might be already closed}}
+}
+
+void f_use_after_close(void) {
+  FILE *p = fopen("foo", "r");
+  if (!p)
+    return;
+  fclose(p);
+  clearerr(p); // expected-warning {{Stream might be already closed}}
+}
+
+void f_open_after_close(void) {
+  FILE *p = fopen("foo", "r");
+  if (!p)
+    return;
+  fclose(p);
+  p = fopen("foo", "r");
+  if (!p)
+    return;
+  fclose(p);
+}
+
+void f_reopen_after_close(void) {
+  FILE *p = fopen("foo", "r");
+  if (!p)
+    return;
+  fclose(p);
+  // Allow reopen after close.
+  p = freopen("foo", "w", p);
+  if (!p)
+    return;
+  fclose(p);
 }
 
 void f_leak(int c) {
   FILE *p = fopen("foo.c", "r");
+  if (!p)
+    return;
   if(c)
     return; // expected-warning {{Opened File never closed. Potential Resource leak}}
   fclose(p);
 }
 
 FILE *f_null_checked(void) {
   FILE *p = fopen("foo.c", "r");
   if (p)

 
 void check_freopen_2() {
   FILE *f1 = fopen("foo.c", "r");
   if (f1) {
     FILE *f2 = freopen(0, "w", f1);
     if (f2) {
       // Check if f1 and f2 point to the same stream.
       fclose(f1);
-      fclose(f2); // expected-warning {{Try to close a file Descriptor already closed. Cause undefined behaviour}}
+      fclose(f2); // expected-warning {{Stream might be already closed.}}
     } else {
       // Reopen failed.
-      // f1 points now to a possibly invalid stream but this condition is currently not checked.
-      // f2 is NULL.
-      rewind(f1);
-      rewind(f2); // expected-warning {{Stream pointer might be NULL}}
+      // f1 is non-NULL but points to a possibly invalid stream.
+      rewind(f1); // expected-warning {{Stream might be invalid}}
+      // f2 is NULL but the previous error stops the checker.
+      rewind(f2);
     }
   }
 }
 
 void check_freopen_3() {
   FILE *f1 = fopen("foo.c", "r");
   if (f1) {
     // Unchecked result of freopen.
-    // The f1 may be invalid after this call (not checked by the checker).
+    // The f1 may be invalid after this call.
     freopen(0, "w", f1);
-    rewind(f1);
+    rewind(f1); // expected-warning {{Stream might be invalid}}
     fclose(f1);
   }
 }