[Analyzer][StreamChecker] Added support for 'fread' and 'fwrite'.

[balazske]

Summary

Stream functions fread and fwrite are evaluated
and preconditions checked.
A new bug type is added for a (non fatal) warning if fread
is called in EOF state.

[Szelethus]

I think the warning about EOF could be a separate patch, and it could be implemented for most stream operations. The patch in large looks great, but I'm just not sure why we handle fwrite so differently. Could you please explain?

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

55

How about return !(*this == ES)?

453–460

Do we not want this for fwrite as well?

492

// The standard refers to this parameter as "nmemb", but almost everywhere else its called "count".

520

Aaaah okay it took me a while. I think SS->ErrorState != ErrorFEof isn't as talkative as isConstrained.*, could you add just a line like "if we know the state to be EOF..."?

On another note, why is fwrite so special in this context?

[balazske]

The difference of fread and fwrite comes from the fact that fwrite can always succeed, fread does not succeed in EOF state.
The plan is to add the file functions sequentially. From the currently implemented functions only fread needs the warning in EOF state. When any later function is added that fails in EOF state, the warning should be added for that function at that time.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

55

Probably better (does not look better but is more bug-safe).

453–460

It should be no problem to write into a file at the end of the file, the file is extended.

492

I looked up the functions in cppreference.com, there it is called count. But probably it is better to use the standard name here?

520

If it is an fwrite, the function call can always succeed (even after a previously EOF or error).
If it is an fread, it can succeed but not after an exactly-EOF state.

A success transition is not needed in an fread with exactly EOF condition. (If we have fread with non-exactly EOF, for example ErrorFEof and ErrorFError, the success is needed because the read in ErrorFError can succeed.)

[Szelethus]

In D80015#2041653, @balazske wrote:

The difference of fread and fwrite comes from the fact that fwrite can always succeed, fread does not succeed in EOF state.
The plan is to add the file functions sequentially. From the currently implemented functions only fread needs the warning in EOF state. When any later function is added that fails in EOF state, the warning should be added for that function at that time.

Ah, silly me. Like, obviously, where would you like write the file, if not at the end of it...

In any case, the warning would be a better fit as a standalone revision because FERROR also deserves a similar warning I think, not to mention that plenty of other stream operations should check against this.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

492

Its not a big deal, either a comment or a variable name change should be fine. I'll leave it to your judgement, but its not a bad idea to leave a line about the most commonly referred name ("count") is not the same as the standard one ("nmemb").

[Szelethus]

But just to assure you, this patch is practically perfect. I don't think we're going to have any hiccups moving forward with this. Again, thank you for all the patience!

[balazske]

If the unit of the change is adding fread and fwrite completely, the warning with FEOF at fread is correct to add because it belongs to fread. I was planning to add some file handling functions to the checker that have a basic functionality implemented. In this case, it is the fread-fwrite functions, but the "indeterminate file position" is still missing here (that is a bigger change), so it is the "basic functionality" that is added. The FEOF warning is probably too small to add it in a separate patch, also it is added only to one place now.

There is intentionally no warning about ferror state, first reason is that often ferror and "indeterminate file position" comes together and then the warning will be produced for "indeterminate position" in the next change. Next reason is that ferror in itself is not always cause for warning and we can currently not decide about it. There was the example about write into a file that is open in read mode that results in a ferror state, then the next read should work (with ferror state at start). Another reason, the program should still always handle if a file operation fails. If an operation starts in ferror state, it will probably fail again but that error should be handled by the application.

Intent of these warnings is not to check that the error case was handled by the user code, that is a more difficult thing. We could make a new warning that warns if between two possible failing operations no ferror or feof was called, this would check if the user handled the error. But not accurate because the error can be handled based on the return value of the failed function, without calling ferror or feof (except some special cases like the fgetc). The only safe warning is to add if a function is called in really unuseful way, for example fread in already FEOF state that do never succeed, and the "indeterminate file position" case.

My goal with these changes was to have a check for the CERT FIO34-C case, not for the complete StreamChecker that could contain many other features. So probably not even all file handling functions will be added for now.

[balazske]

Renamed "Count" to "NMemb", fixed operator !=.

[Szelethus]

This patch is great. LGTM!

In D80015#2043263, @balazske wrote:

If the unit of the change is adding fread and fwrite completely, the warning with FEOF at fread is correct to add because it belongs to fread. I was planning to add some file handling functions to the checker that have a basic functionality implemented. In this case, it is the fread-fwrite functions, but the "indeterminate file position" is still missing here (that is a bigger change), so it is the "basic functionality" that is added. The FEOF warning is probably too small to add it in a separate patch, also it is added only to one place now.

Very well, I'm sold. I didn't check that we don't really model another function that needs it. Woops :^)

There is intentionally no warning about ferror state, first reason is that often ferror and "indeterminate file position" comes together and then the warning will be produced for "indeterminate position" in the next change. Next reason is that ferror in itself is not always cause for warning and we can currently not decide about it. There was the example about write into a file that is open in read mode that results in a ferror state, then the next read should work (with ferror state at start). Another reason, the program should still always handle if a file operation fails. If an operation starts in ferror state, it will probably fail again but that error should be handled by the application.

Intent of these warnings is not to check that the error case was handled by the user code, that is a more difficult thing. We could make a new warning that warns if between two possible failing operations no ferror or feof was called, this would check if the user handled the error. But not accurate because the error can be handled based on the return value of the failed function, without calling ferror or feof (except some special cases like the fgetc). The only safe warning is to add if a function is called in really unuseful way, for example fread in already FEOF state that do never succeed, and the "indeterminate file position" case.

The question is what is a good practice on ferror -- a check on errno and then a clearerr? In any case, you are right, this another discussion for another time.

My goal with these changes was to have a check for the CERT FIO34-C case, not for the complete StreamChecker that could contain many other features. So probably not even all file handling functions will be added for now.

Sure, shoot me down if you feel I'm trying to dictate the direction of your project! :)

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

520

Aaaah okay it took me a while. I think SS->ErrorState != ErrorFEof isn't as talkative as isConstrained.*, could you add just a line like "if we know the state to be EOF..."?

Could you please add these few words before commiting?

[balazske]

I want to test the StreamChecker for false positives. Page http://clang.llvm.org/analyzer/open_projects.html says that it has too much false positives because state splitting (I did not see this page before.)

There is an alternative way of implementation: Store a (ordered) list of previous stream operations and data what we know about the operation. This data contains if the operation failed or not, the symbol that is the return value of the function, error state before and after the operation, what bug report was made already. This list can be populated during analysis, without state splits. At every time when a new information is put into the list it is possible to check for problems.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

520

It is now "If we know the state to be FEOF at fread, do not add a success state.".

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

struct StreamErrorState {
   bool isNoError() const { return NoError && !FEof && !FError; }
   bool isFEof() const { return !NoError && FEof && !FError; }
   bool isFError() const { return !NoError && !FEof && FError; }
 
   bool operator==(const StreamErrorState &ES) const {
     return NoError == ES.NoError && FEof == ES.FEof && FError == ES.FError;
   }
 
+  bool operator!=(const StreamErrorState &ES) const { return !(*this == ES); }
+
   StreamErrorState operator|(const StreamErrorState &E) const {
     return {NoError || E.NoError, FEof || E.FEof, FError || E.FError};
   }
 
   StreamErrorState operator&(const StreamErrorState &E) const {
     return {NoError && E.NoError, FEof && E.FEof, FError && E.FError};
   }
 
ProgramStateRef bindInt(uint64_t Value, ProgramStateRef State,
   State = State->BindExpr(CE, C.getLocationContext(),
                           C.getSValBuilder().makeIntVal(Value, false));
   return State;
 }
 
 class StreamChecker
     : public Checker<check::PreCall, eval::Call, check::DeadSymbols> {
   mutable std::unique_ptr<BuiltinBug> BT_nullfp, BT_illegalwhence,
-      BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak;
+      BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak, BT_StreamEof;
 
 public:
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const;
   bool evalCall(const CallEvent &Call, CheckerContext &C) const;
   void checkDeadSymbols(SymbolReaper &SymReaper, CheckerContext &C) const;
 
   /// If true, evaluate special testing stream functions.
   bool TestMode = false;
 
 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}},
+      {{"fread", 4},
+       {&StreamChecker::preFread,
+        std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4, true), 3}},
+      {{"fwrite", 4},
+       {&StreamChecker::preFwrite,
+        std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4, false), 3}},
       {{"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, &StreamChecker::evalClearerr, 0}},
       {{"feof", 1},
private:
   void preFreopen(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;
 
+  void preFread(const FnDescription *Desc, const CallEvent &Call,
+                CheckerContext &C) const;
+
+  void preFwrite(const FnDescription *Desc, const CallEvent &Call,
+                 CheckerContext &C) const;
+
+  void evalFreadFwrite(const FnDescription *Desc, const CallEvent &Call,
+                       CheckerContext &C, bool IsFread) const;
+
   void preFseek(const FnDescription *Desc, const CallEvent &Call,
                 CheckerContext &C) const;
   void evalFseek(const FnDescription *Desc, const CallEvent &Call,
                  CheckerContext &C) const;
 
   void preDefault(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
 
private:
 
   /// 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;
 
+  /// Generate warning about stream in EOF state.
+  /// There will be always a state transition into the passed State,
+  /// by the new non-fatal error node or (if failed) a normal transition,
+  /// to ensure uniform handling.
+  void reportFEofWarning(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()) {
void StreamChecker::evalFclose(const FnDescription *Desc, const CallEvent &Call,
   // 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));
 
   C.addTransition(State);
 }
 
+void StreamChecker::preFread(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;
+
+  SymbolRef Sym = StreamVal.getAsSymbol();
+  if (Sym && State->get<StreamMap>(Sym)) {
+    const StreamState *SS = State->get<StreamMap>(Sym);
+    if (SS->ErrorState & ErrorFEof)
+      reportFEofWarning(C, State);
+  } else {
+    C.addTransition(State);
+  }
+}
+
+void StreamChecker::preFwrite(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);
+}
+
+void StreamChecker::evalFreadFwrite(const FnDescription *Desc,
+                                    const CallEvent &Call, CheckerContext &C,
+                                    bool IsFread) 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;
+
+  Optional<NonLoc> SizeVal = Call.getArgSVal(1).getAs<NonLoc>();
+  if (!SizeVal)
+    return;
+  Optional<NonLoc> NMembVal = Call.getArgSVal(2).getAs<NonLoc>();
+  if (!NMembVal)
+    return;
+
+  const StreamState *SS = State->get<StreamMap>(StreamSym);
+  if (!SS)
+    return;
+
+  assertStreamStateOpened(SS);
+
+  // C'99 standard, §7.19.8.1.3, the return value of fread:
+  // The fread function returns the number of elements successfully read, which
+  // may be less than nmemb if a read error or end-of-file is encountered. If
+  // size or nmemb is zero, fread returns zero and the contents of the array and
+  // the state of the stream remain unchanged.
+
+  if (State->isNull(*SizeVal).isConstrainedTrue() ||
+      State->isNull(*NMembVal).isConstrainedTrue()) {
+    // This is the "size or nmemb is zero" case.
+    // Just return 0, do nothing more (not clear the error flags).
+    State = bindInt(0, State, C, CE);
+    C.addTransition(State);
+    return;
+  }
+
+  // Generate a transition for the success state.
+  // If we know the state to be FEOF at fread, do not add a success state.
+  if (!IsFread || (SS->ErrorState != ErrorFEof)) {
+    ProgramStateRef StateNotFailed =
+        State->BindExpr(CE, C.getLocationContext(), *NMembVal);
+    if (StateNotFailed) {
+      StateNotFailed = StateNotFailed->set<StreamMap>(
+          StreamSym, StreamState::getOpened(Desc));
+      C.addTransition(StateNotFailed);
+    }
+  }
+
+  // Add transition for the failed state.
+  Optional<NonLoc> RetVal = makeRetVal(C, CE).castAs<NonLoc>();
+  assert(RetVal && "Value should be NonLoc.");
+  ProgramStateRef StateFailed =
+      State->BindExpr(CE, C.getLocationContext(), *RetVal);
+  if (!StateFailed)
+    return;
+  auto Cond = C.getSValBuilder()
+                  .evalBinOpNN(State, BO_LT, *RetVal, *NMembVal,
+                               C.getASTContext().IntTy)
+                  .getAs<DefinedOrUnknownSVal>();
+  if (!Cond)
+    return;
+  StateFailed = StateFailed->assume(*Cond, true);
+  if (!StateFailed)
+    return;
+
+  StreamErrorState NewES;
+  if (IsFread)
+    NewES = (SS->ErrorState == ErrorFEof) ? ErrorFEof : ErrorFEof | ErrorFError;
+  else
+    NewES = ErrorFError;
+  StreamState NewState = StreamState::getOpened(Desc, NewES);
+  StateFailed = StateFailed->set<StreamMap>(StreamSym, NewState);
+  C.addTransition(StateFailed);
+}
+
 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 (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
     C.emitReport(std::make_unique<PathSensitiveBugReport>(
         *BT_illegalwhence, BT_illegalwhence->getDescription(), N));
     return nullptr;
   }
 
   return State;
 }
 
+void StreamChecker::reportFEofWarning(CheckerContext &C,
+                                      ProgramStateRef State) const {
+  if (ExplodedNode *N = C.generateNonFatalErrorNode(State)) {
+    if (!BT_StreamEof)
+      BT_StreamEof.reset(
+          new BuiltinBug(this, "Stream already in EOF",
+                         "Read function called when stream is in EOF state. "
+                         "Function has no effect."));
+    C.emitReport(std::make_unique<PathSensitiveBugReport>(
+        *BT_StreamEof, BT_StreamEof->getDescription(), N));
+    return;
+  }
+  C.addTransition(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) {
     SymbolRef Sym = I.first;

clang/test/Analysis/stream-error.c

 // RUN: %clang_analyze_cc1 -verify %s \
 // RUN: -analyzer-checker=core \
 // RUN: -analyzer-checker=alpha.unix.Stream \
 // RUN: -analyzer-checker=debug.StreamTester \
 // RUN: -analyzer-checker=debug.ExprInspection
 
 #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}}
void stream_error_ferror() {
   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_fread() {
+  FILE *F = tmpfile();
+  if (!F)
+    return;
+  char Buf[10];
+  int Ret = fread(Buf, 1, 10, F);
+  if (Ret == 10) {
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{TRUE}}
+    if (feof(F)) {
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+      fread(Buf, 1, 10, F);           // expected-warning {{Read function called when stream is in EOF state}}
+      clang_analyzer_eval(feof(F));   // expected-warning {{TRUE}}
+      clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+    }
+    if (ferror(F)) {
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+      fread(Buf, 1, 10, F);           // no warning
+    }
+  }
+  fclose(F);
+  Ret = fread(Buf, 1, 10, F); // expected-warning {{Stream might be already closed}}
+}
+
+void error_fwrite() {
+  FILE *F = tmpfile();
+  if (!F)
+    return;
+  const char *Buf = "123456789";
+  int Ret = fwrite(Buf, 1, 10, F);
+  if (Ret == 10) {
+    clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
+  } else {
+    clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+    clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+    fwrite(0, 1, 10, F);            // no warning
+  }
+  fclose(F);
+  Ret = fwrite(0, 1, 10, F); // expected-warning {{Stream might be already closed}}
+}
+
+void freadwrite_zerosize(FILE *F) {
+  fwrite(0, 1, 0, F);
+  fwrite(0, 0, 1, F);
+  fread(0, 1, 0, F);
+  fread(0, 0, 1, F);
+}
+
+void freadwrite_zerosize_eofstate(FILE *F) {
+  fwrite(0, 1, 0, F);
+  fwrite(0, 0, 1, F);
+  fread(0, 1, 0, F); // expected-warning {{Read function called when stream is in EOF state}}
+  fread(0, 0, 1, F); // expected-warning {{Read function called when stream is in EOF state}}
+}
+
+void error_fread_fwrite_zerosize() {
+  FILE *F = fopen("file", "r");
+  if (!F)
+    return;
+
+  freadwrite_zerosize(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+
+  StreamTesterChecker_make_ferror_stream(F);
+  freadwrite_zerosize(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{TRUE}}
+
+  StreamTesterChecker_make_feof_stream(F);
+  freadwrite_zerosize_eofstate(F);
+  clang_analyzer_eval(feof(F));   // expected-warning {{TRUE}}
+  clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+
+  fclose(F);
+}
+
 void error_fseek() {
   FILE *F = fopen("file", "r");
   if (!F)
     return;
   int rc = fseek(F, 0, SEEK_SET);
   if (rc) {
     int IsFEof = feof(F), IsFError = ferror(F);
     // Get feof or ferror or no error.