[Analyzer][StreamChecker] Check for opened stream before operations.

[balazske]

Summary

According to documentations, after an fclose call any other stream
operations cause undefined behaviour, regardless if the close failed
or not.
This change adds the check for the opened state before all other
(applicable) operations.

[Szelethus]

Amazing, thanks!

[balazske]

rebase to master

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 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); }
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"}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
       {{"freopen", 3},
        {&StreamChecker::preFreopen, &StreamChecker::evalFreopen, 2}},
       {{"tmpfile"}, {nullptr, &StreamChecker::evalFopen, ArgNone}},
       {{"fclose", 1},
-       {&StreamChecker::preFclose, &StreamChecker::evalFclose, 0}},
+       {&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 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 preFclose(const FnDescription *Desc, const CallEvent &Call,
-                 CheckerContext &C) const;
   void evalFclose(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
 
   void preFseek(const FnDescription *Desc, const CallEvent &Call,
                 CheckerContext &C) const;
 
   void preDefault(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;  
 
-  /// Check that the stream is not closed.
-  /// Return a state where the stream is guaranteed to not in closed state
-  /// (if data about it exists).
-  /// Generate error if the stream is provable in closed state.
-  ProgramStateRef ensureStreamNotClosed(SVal StreamVal, CheckerContext &C,
-                                        ProgramStateRef State) const;
+  /// 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,
 
   /// 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;  
 
StateRetNotNull =
       StateRetNotNull->set<StreamMap>(StreamSym, StreamState::getOpened());
   StateRetNull =
       StateRetNull->set<StreamMap>(StreamSym, StreamState::getOpenFailed());
 
   C.addTransition(StateRetNotNull);
   C.addTransition(StateRetNull);
 }
 
-void StreamChecker::preFclose(const FnDescription *Desc, const CallEvent &Call,
-                              CheckerContext &C) const {
-  ProgramStateRef State = C.getState();
-  State = ensureStreamNotClosed(getStreamArg(Desc, Call), C, State);
-  if (!State)
-    return;
-
-  C.addTransition(State);
-}
-
 void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call,
                                CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SymbolRef Sym = getStreamArg(Desc, Call).getAsSymbol();
   if (!Sym)
     return;
 
   const StreamState *SS = State->get<StreamMap>(Sym);

 
 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::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;
 
   C.addTransition(State);
 }
 
 ProgramStateRef
 StreamChecker::ensureStreamNonNull(SVal StreamVal, CheckerContext &C,
                                    ProgramStateRef State) const {
   auto Stream = StreamVal.getAs<DefinedSVal>();
if (ExplodedNode *N = C.generateErrorNode(StateNull)) {
           *BT_nullfp, BT_nullfp->getDescription(), N));
     }
     return nullptr;
   }
 
   return StateNotNull;
 }
 
-ProgramStateRef
-StreamChecker::ensureStreamNotClosed(SVal StreamVal, CheckerContext &C,
+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;
 
-  // 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;
   }
 
+  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;
 }
 
 ProgramStateRef
 StreamChecker::ensureFseekWhenceCorrect(SVal WhenceVal, CheckerContext &C,
                                         ProgramStateRef State) const {
   Optional<nonloc::ConcreteInt> CI = WhenceVal.getAs<nonloc::ConcreteInt>();
   if (!CI)

clang/test/Analysis/stream.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-store region -verify %s
 
 typedef __typeof__(sizeof(int)) size_t;
 typedef __typeof__(sizeof(int)) fpos_t;
 typedef struct _IO_FILE FILE;
-#define SEEK_SET  0 /* Seek from beginning of file.  */
-#define SEEK_CUR  1 /* Seek from current position.  */
-#define SEEK_END  2 /* Seek from end of file.  */
+#define SEEK_SET  0  /* Seek from beginning of file.  */
+#define SEEK_CUR  1  /* Seek from current position.  */
+#define SEEK_END  2  /* Seek from end of file.  */
 extern FILE *fopen(const char *path, const char *mode);
 extern FILE *tmpfile(void);
 extern int fclose(FILE *fp);
 extern size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
 extern size_t fwrite(const void *ptr, size_t size, size_t nmemb, FILE *stream);
 extern int fseek (FILE *__stream, long int __off, int __whence);
 extern long int ftell (FILE *__stream);
 extern void rewind (FILE *__stream);
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);
   }
-}
+}
 No newline at end of file