[Analyzer][StreamChecker] Track streams that were not found to be opened.

[balazske]

Summary

If a stream operation is encountered with unknown stream (to the checker)
the stream is recognized and added to the data structure.
(Instead of ignoring the call.)

This patch in only here for reference purposes, to not forget that this can be a useful feature. Probably at a later time we can think about this problem again and adapt this change to the StreamChecker at that time. The problem is that if we find a stream function call without a stream open call, the stream was opened presumably at another function. So it is assumable that the other function (where it was opened) handles some of the stream calls and error handling, probably together with the current function, so analysing the current function only may produce wrong results (but still it is possible to discover real errors). (And if we have luck the current function is inlined from that other function.)

[balazske]

I am not sure if this is the best approach: In a similar case (no opening function encountered) it is possible that the stream is already "escaped", there could be references to it from outside the analyzed function (to which it was passed, or got from another non-analyzed function). Maybe it is better to ignore such cases as it is already without this change, then we do not need to handle "escape" of the stream pointer specially (only remove it from the data).

[Szelethus]

The high level idea seems great after surveying the analyzer for similar issues, but I might need to think about this a bit longer. @baloghadamsoftware, IteratorChecker needs to solve similar problems, right? Do you have any input on this?

[baloghadamsoftware]

I like this idea, except the word OpenEncountered, but that might be a matter of taste. Please try this patch on several open-source projects, such as BitCoin, CURL, OpenSSL, PostGreS, TMux and Xerces. Then compare the results whether we have more or less true and false positives.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

36

Maybe SawOpened or ExplicitlyOpened?

[NoQ]

clang/test/Analysis/stream.c

11–13

What do these tests test?

[balazske]

Currently priority of this change is lower than D78374.
If a similar change is done a "escaped" stream state will be needed too.

clang/test/Analysis/stream.c

11–13

There should be no warning about resource leak (file not closed). This may be not the best approach to show what the checker does. Some other detectable problem should be added to show that the functions are not ignored by the checker.
(I do not like that the buffer argument is 0 here, probably other checker should check this.)

[Szelethus]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

36

Shouldn't this be in KindTy?

[Szelethus]

Its a bit hard to judge this. Have you tested this on open source projects?

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

 namespace {
 
 struct FnDescription;
 
 /// Full state information about a stream pointer.
 struct StreamState {
   /// The last file operation called in the stream.
   const FnDescription *LastOperation;
+  /// Was a opening function encountered for this stream?
+  /// (False if the stream was first encountered in a non-opening function.)
+  bool OpenEncountered;
 
   /// State of a stream symbol.
   /// FIXME: We need maybe an "escaped" state later.
   enum KindTy {
     Opened, /// Stream is opened.
     Closed, /// Closed stream (an invalid stream pointer after it was closed).
     OpenFailed /// The last open operation has failed.
   } State;
struct StreamState {
   }
 
   bool operator==(const StreamState &X) const {
     // In not opened state error should always NoError.
     return LastOperation == X.LastOperation && State == X.State &&
            ErrorState == X.ErrorState;
   }
 
-  static StreamState getOpened(const FnDescription *L) {
-    return StreamState{L, Opened};
+  static StreamState getOpened(const FnDescription *L,
+                               const StreamState *PrevState,
+                               ErrorKindTy NewError) {
+    if (PrevState)
+      return StreamState{L, PrevState->OpenEncountered, Opened, NewError};
+    else
+      return StreamState{L, false, Opened, NewError};
   }
-  static StreamState getOpened(const FnDescription *L, ErrorKindTy E) {
-    return StreamState{L, Opened, E};
+  static StreamState getOpened(const FnDescription *L,
+                               bool OpenEncountered = true) {
+    return StreamState{L, OpenEncountered, Opened, NoError};
   }
   static StreamState getClosed(const FnDescription *L) {
-    return StreamState{L, Closed};
+    return StreamState{L, false, Closed};
   }
   static StreamState getOpenFailed(const FnDescription *L) {
-    return StreamState{L, OpenFailed};
+    return StreamState{L, false, OpenFailed};
   }
 
   /// Return if the specified error kind is possible on the stream in the
   /// current state.
   /// This depends on the stored `LastOperation` value.
   /// If the error is not possible returns empty value.
   /// If the error is possible returns the remaining possible error type
   /// (after taking out `ErrorKind`). If a single error is possible it will
   /// return that value, otherwise unknown error.
   Optional<ErrorKindTy> getRemainingPossibleError(ErrorKindTy ErrorKind) const;
 
   void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddPointer(LastOperation);
+    ID.AddBoolean(OpenEncountered);
     ID.AddInteger(State);
     ID.AddInteger(ErrorState);
   }
 };
 
 class StreamChecker;
 using FnCheck = std::function<void(const StreamChecker *, const FnDescription *,
                                    const CallEvent &, CheckerContext &)>;
private:
   template <StreamState::ErrorKindTy ErrorKind>
   void evalFeofFerror(const FnDescription *Desc, const CallEvent &Call,
                       CheckerContext &C) const;
 
   template <StreamState::ErrorKindTy EK>
   void evalSetFeofFerror(const FnDescription *Desc, const CallEvent &Call,
                          CheckerContext &C) const;
 
+  void evalDebugDumpState(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 the opened state.
private:
   }
 };
 
 } // end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(StreamMap, SymbolRef, StreamState)
 
 inline void assertStreamStateOpened(const StreamState *SS) {
-  assert(SS->isOpened() &&
+  assert((!SS || SS->isOpened()) &&
          "Previous create of error node for non-opened stream failed?");
 }
 
 void StreamChecker::checkPreCall(const CallEvent &Call,
                                  CheckerContext &C) const {
   const FnDescription *Desc = lookupFn(Call);
   if (!Desc || !Desc->PreFn)
     return;
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;
 
+  bool OpenEncountered = false;
+  if (const StreamState *SS = State->get<StreamMap>(StreamSym))
+    OpenEncountered = SS->OpenEncountered;
+
   // 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(Desc));
+  StateRetNotNull = StateRetNotNull->set<StreamMap>(
+      StreamSym, StreamState::getOpened(Desc, OpenEncountered));
   StateRetNull =
       StateRetNull->set<StreamMap>(StreamSym, StreamState::getOpenFailed(Desc));
 
   C.addTransition(StateRetNotNull);
   C.addTransition(StateRetNull);
 }
 
 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);
-  if (!SS)
-    return;
 
   assertStreamStateOpened(SS);
 
   // 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(Desc));
 
void StreamChecker::evalFseek(const FnDescription *Desc, const CallEvent &Call,
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   if (!StreamSym)
     return;
 
   const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
   if (!CE)
     return;
 
-  // Ignore the call if the stream is not tracked.
-  if (!State->get<StreamMap>(StreamSym))
-    return;
+  const StreamState *SS = State->get<StreamMap>(StreamSym);
 
-  DefinedSVal RetVal = makeRetVal(C, CE);
+  assertStreamStateOpened(SS);
 
   // Make expression result.
+  DefinedSVal RetVal = makeRetVal(C, CE);
   State = State->BindExpr(CE, C.getLocationContext(), RetVal);
 
   // 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, RetVal);
 
   // Reset the state to opened with no error.
-  StateNotFailed =
-      StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
+  StateNotFailed = StateNotFailed->set<StreamMap>(
+      StreamSym, StreamState::getOpened(Desc, SS, StreamState::NoError));
   // We get error.
   StateFailed = StateFailed->set<StreamMap>(
-      StreamSym, StreamState::getOpened(Desc, StreamState::Unknown));
+      StreamSym, StreamState::getOpened(Desc, SS, StreamState::Unknown));
 
   C.addTransition(StateNotFailed);
   C.addTransition(StateFailed);
 }
 
 void StreamChecker::evalClearerr(const FnDescription *Desc,
                                  const CallEvent &Call,
                                  CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   if (!StreamSym)
     return;
 
   const StreamState *SS = State->get<StreamMap>(StreamSym);
-  if (!SS)
-    return;
 
   assertStreamStateOpened(SS);
 
-  State = State->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
+  State = State->set<StreamMap>(
+      StreamSym, StreamState::getOpened(Desc, SS, StreamState::NoError));
+
   C.addTransition(State);
 }
 
 template <StreamState::ErrorKindTy ErrorKind>
 void StreamChecker::evalFeofFerror(const FnDescription *Desc,
                                    const CallEvent &Call,
                                    CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   if (!StreamSym)
     return;
 
   const CallExpr *CE = dyn_cast_or_null<CallExpr>(Call.getOriginExpr());
   if (!CE)
     return;
 
-  auto AddTransition = [&C, Desc, StreamSym](ProgramStateRef State,
-                                             StreamState::ErrorKindTy SSError) {
-    StreamState SSNew = StreamState::getOpened(Desc, SSError);
+  const StreamState *SS = State->get<StreamMap>(StreamSym);
+
+  assertStreamStateOpened(SS);
+
+  auto AddTransition = [&C, Desc, StreamSym,
+                        SS](ProgramStateRef State,
+                            StreamState::ErrorKindTy NewError) {
+    StreamState SSNew = StreamState::getOpened(Desc, SS, NewError);
     State = State->set<StreamMap>(StreamSym, SSNew);
     C.addTransition(State);
   };
 
-  const StreamState *SS = State->get<StreamMap>(StreamSym);
-  if (!SS)
+  if (!SS) {
+    AddTransition(bindInt(0, State, C, CE), StreamState::Unknown);
+    AddTransition(bindAndAssumeTrue(State, C, CE), ErrorKind);
     return;
-
-  assertStreamStateOpened(SS);
+  }
 
   if (!SS->isUnknown()) {
     // The stream is in exactly known state (error or not).
     // Check if it is in error of kind `ErrorKind`.
     if (SS->ErrorState == ErrorKind)
       State = bindAndAssumeTrue(State, C, CE);
     else
       State = bindInt(0, State, C, CE);

 void StreamChecker::evalSetFeofFerror(const FnDescription *Desc,
                                       const CallEvent &Call,
                                       CheckerContext &C) const {
   ProgramStateRef State = C.getState();
   SymbolRef StreamSym = getStreamArg(Desc, Call).getAsSymbol();
   assert(StreamSym && "Operation not permitted on non-symbolic stream value.");
   const StreamState *SS = State->get<StreamMap>(StreamSym);
   assert(SS && "Stream should be tracked by the checker.");
-  State = State->set<StreamMap>(StreamSym,
-                                StreamState::getOpened(SS->LastOperation, EK));
+  State = State->set<StreamMap>(
+      StreamSym, StreamState::getOpened(SS->LastOperation, SS, EK));
   C.addTransition(State);
 }
 
 ProgramStateRef
 StreamChecker::ensureStreamNonNull(SVal StreamVal, CheckerContext &C,
                                    ProgramStateRef State) const {
   auto Stream = StreamVal.getAs<DefinedSVal>();
   if (!Stream)
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) {
     SymbolRef Sym = I.first;
     const StreamState &SS = I.second;
-    if (!SymReaper.isDead(Sym) || !SS.isOpened())
+    if (!SymReaper.isDead(Sym) || !SS.isOpened() || !SS.OpenEncountered)
       continue;
 
     ExplodedNode *N = C.generateErrorNode();
     if (!N)
       continue;
 
     if (!BT_ResourceLeak)
       BT_ResourceLeak.reset(

clang/test/Analysis/stream-error.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=core -analyzer-checker=debug.StreamTester,debug.ExprInspection -analyzer-store region -verify %s
 
 #include "Inputs/system-header-simulator.h"
 
 void clang_analyzer_eval(int);
+void clang_analyzer_warnIfReached();
 void StreamTesterChecker_make_feof_stream(FILE *);
 void StreamTesterChecker_make_ferror_stream(FILE *);
 
 void error_fopen() {
   FILE *F = fopen("file", "r");
   if (!F)
     return;
   clang_analyzer_eval(feof(F)); // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
   fclose(F);
 }
 
-void error_freopen() {
-  FILE *F = fopen("file", "r");
-  if (!F)
-    return;
+void error_freopen(FILE *F) {
   F = freopen(0, "w", F);
   if (!F)
     return;
   clang_analyzer_eval(feof(F)); // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
-  fclose(F);
 }
 
 void stream_error_feof() {
   FILE *F = fopen("file", "r");
   if (!F)
     return;
   StreamTesterChecker_make_feof_stream(F);
   clang_analyzer_eval(feof(F));   // expected-warning {{TRUE}}
   clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
   clearerr(F);
   clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
   fclose(F);
 }
 
+void stream_error_feof_noopen(FILE *F) {
+  if (feof(F))
+    clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+  else if (ferror(F))
+    clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  else
+    clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
+
 void stream_error_ferror() {
   FILE *F = fopen("file", "r");
   if (!F)
     return;
   StreamTesterChecker_make_ferror_stream(F);
   clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
   clearerr(F);
   clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
   clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
   fclose(F);
 }
 
-void error_fseek() {
-  FILE *F = fopen("file", "r");
-  if (!F)
-    return;
+void stream_error_ferror_noopen(FILE *F) {
+  if (ferror(F))
+    clang_analyzer_eval(feof(F)); // expected-warning {{FALSE}}
+  else if (feof(F))
+    clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+  else
+    clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+}
+
+void error_fseek(FILE *F) {
   int rc = fseek(F, 0, SEEK_SET);
   if (rc) {
     int IsFEof = feof(F), IsFError = ferror(F);
     // Get feof or ferror or no error.
     clang_analyzer_eval(IsFEof || IsFError);
     // expected-warning@-1 {{FALSE}}
     // expected-warning@-2 {{TRUE}}
     clang_analyzer_eval(IsFEof && IsFError); // expected-warning {{FALSE}}
     // Error flags should not change.
     if (IsFEof)
       clang_analyzer_eval(feof(F)); // expected-warning {{TRUE}}
     else
       clang_analyzer_eval(feof(F)); // expected-warning {{FALSE}}
     if (IsFError)
       clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
     else
       clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
   } else {
     clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
     clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
     // Error flags should not change.
     clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
     clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
   }
-  fclose(F);
 }

clang/test/Analysis/stream.c

 // RUN: %clang_analyze_cc1 -analyzer-checker=alpha.unix.Stream -analyzer-store region -verify %s
 
 #include "Inputs/system-header-simulator.h"
 
 void check_fread() {
   FILE *fp = tmpfile();
   fread(0, 0, 0, fp); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
 
+void check_feread_noopen(FILE *fp) {
+  fread(0, 0, 0, fp);
+}
+
 void check_fwrite() {
   FILE *fp = tmpfile();
   fwrite(0, 0, 0, fp); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
 
+void check_fwrite_noopen(FILE *fp) {
+  fwrite(0, 0, 0, fp);
+}
+
 void check_fseek() {
   FILE *fp = tmpfile();
   fseek(fp, 0, 0); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
 
+void check_fseek_noopen(FILE *fp) {
+  fseek(fp, 0, 0);
+}
+
 void check_ftell() {
   FILE *fp = tmpfile();
   ftell(fp); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
 
+void check_ftell_noopen(FILE *fp) {
+  ftell(fp);
+}
+
 void check_rewind() {
   FILE *fp = tmpfile();
   rewind(fp); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
 
+void check_rewind_noopen(FILE *fp) {
+  rewind(fp);
+}
+
 void check_fgetpos() {
   FILE *fp = tmpfile();
   fpos_t pos;
   fgetpos(fp, &pos); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
 
+void check_fgetpos_noopen(FILE *fp) {
+  fpos_t pos;
+  fgetpos(fp, &pos);
+}
+
 void check_fsetpos() {
   FILE *fp = tmpfile();
   fpos_t pos;
   fsetpos(fp, &pos); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
 
+void check_fsetpos_noopen(FILE *fp) {
+  fpos_t pos;
+  fsetpos(fp, &pos);
+}
+
 void check_clearerr() {
   FILE *fp = tmpfile();
   clearerr(fp); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
 
+void check_clearerr_noopen(FILE *fp) {
+  clearerr(fp);
+}
+
 void check_feof() {
   FILE *fp = tmpfile();
   feof(fp); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
 
+void check_feof_noopen(FILE *fp) {
+  feof(fp);
+}
+
 void check_ferror() {
   FILE *fp = tmpfile();
   ferror(fp); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
 
+void check_ferror_noopen(FILE *fp) {
+  ferror(fp);
+}
+
 void check_fileno() {
   FILE *fp = tmpfile();
   fileno(fp); // expected-warning {{Stream pointer might be NULL}}
   fclose(fp);
 }
 
+void check_fileno_noopen(FILE *fp) {
+  fileno(fp);
+}
+
 void f_open(void) {
   FILE *p = fopen("foo", "r");
   char buf[1024];
   fread(buf, 1, 1, p); // expected-warning {{Stream pointer might be NULL}}
   fclose(p);
 }
 
 void f_seek(void) {
void check_freopen_3() {
   if (f1) {
     // Unchecked result of freopen.
     // The f1 may be invalid after this call.
     freopen(0, "w", f1);
     rewind(f1); // expected-warning {{Stream might be invalid}}
     fclose(f1);
   }
 }
 No newline at end of file