[Analyzer][StreamChecker] Added evaluation of fseek.

[balazske]

Summary

Function fseek is now evaluated with setting error return value
and error flags.

[Szelethus]

We should totally dedicate an error kind of fseek, see (and please respond, if you could to) D75356#inline-689287.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

382–384

This seems a bit excessive, we could merge the last two into FSeekError.

[balazske]

I do not like to make difference between error from fseek and error from any other function that may fail. AnyError can be used to handle every case. After a failed file operation we do not know if it was feof or ferror kind of error. fseek is only special because it can fail additionally without feof or ferror. If the kind of the last operation needs to be known later, it can be saved in the stream state.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

382–384

There are 3 cases:

• fseek did not fail at all. Return value is zero. This is StateNotFailed.
• fseek failed but none of the error flags is true afterwards. Return value is nonzero but feof and ferror are not true. This is StateFailedWithoutFError.
• fseek failed and we have feof or ferror set (never both). Return value is nonzero and feof or ferror will be true. This is StateFailedWithFError. And an use of AnyError, otherwise we need here 2 states, one for feof and one for ferror. But it is not important here if feof or ferror is actually true, so the special value AnyError is used and only one new state instead of two.

[Szelethus]

In D75851#1914874, @balazske wrote:

I do not like to make difference between error from fseek and error from any other function that may fail. AnyError can be used to handle every case.

Okay, it took me a while, but I think I understand where you're coming from. Lets preserve this enum. I think NoteTags would be a better fit to pinpoint which function left the stream in a state that should be checked before usage. This patch is quite small, could we add one then?

After a failed file operation we do not know if it was feof or ferror kind of error.

Is this true for that many stream operations?

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

382–384

Sure, but from the point of the analyzer the latter two are the same, isn't it? Its never a good idea to use a stream on which fseek failed without checking.

State splits are the most expensive things the analyzer can make, which is why I'm cautious here.

[Szelethus]

In D75851#1915014, @Szelethus wrote:

In D75851#1914874, @balazske wrote:

After a failed file operation we do not know if it was feof or ferror kind of error.

Is this true for that many stream operations?

I looked it up, and it totally is.

[balazske]

Probably we need to clarify when to make warnings. Originally I do not wanted a warning after for example failed read, because it is possible to retry the operation. But the standard says that after failed fread the file position is indeterminate, so a new read may get indeterminate values that is a case for warning. The character I/O functions may work not this way so after failed fgetc it may be possible to retry it and no warning is needed. Some operations such as fclose and fseek can be called in error (including EOF) state.

For this to work we need more kinds of error states, or save the type of the last operation in the state (and use it to determine if warning is needed). Probably the best is to have only a single "failed" state and determine what kind of error is possible based on the saved last operation type. A "feof" and "ferror" kind of error state is still needed because after a call to feof if it returned true the error type is fixed to EOF error.

[baloghadamsoftware]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

382–384

Somehow, this should be done with just two new states. Maybe there should be an error state "Unknown" instead of OtherError (or FeoFOrFError what I suggested at the other patch) which can be any of the errors plus the NoError value. Later, when ferror() of feof() is checked we can do a second state split, but not earlier.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

382–384

Yes it is possible to use two new states only. For example by having a new FSeekError that can "manifest" in feof, ferror or no error. Better is to have only one "last-operation-failed-with-any-error" state and determine later if needed what errors are possible (based on the last operation). For fseek it is possible to get feof, ferror, or none of these (but still failed fseek). For reading operations the error can be feof or ferror. After write only ferror is possible.

[balazske]

Rebased, added handling of UnknownError.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

427–486

Please explain the high-level idea behind this code.

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

427–486

The higher level idea is that if a stream function fails we do not create a new state for every type of error (EOF and "other" error). Instead there will be an "unknown error" state. The description for each stream function contains what errors are possible after that function (PossibleErrors).

If it is needed to know the exact error (like here, what should feof return?) we look at the previous function to see what errors are possible after it. If EOF is not possible at all, the feof returns false. If EOF is possible and only one other type of error, we make a state split with EOF error and the other error set. If EOF and two possible other errors are possible there is state split again but the non-EOF state contains UnknownError.

In PossibleErrors the NoError state is possible. This indicates that the function failed (returned an error value) but the stream error flags are not set (can happen at fseek). There are 3 possible error values (EOF, "other" and no error), if after a feof there is UnknownError we know that the remaining 2 error types are possible.

[balazske]

Rebased.

[balazske]

Simplified code.

[Szelethus]

In D75851#1933538, @balazske wrote:

Simplified code.

I can tell! The patch is amazing, the code practically reads itself aloud. I have some inlines but nothing major, the high level idea is great.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

76–83

This feels clunky to me.

Suppose that we're curious about the stream reaching EOF. We know that the last operation on the stream could cause EOF and ERROR. This function then would return FError. That doesn't really feel like the answer to the question "isErrorPossible".

In what contexts do you see calling this function that isn't like this?:

Optional<StreamState::ErrorKindTy> NewError =
    SS->isErrorPossible(ErrorKind);
if (!NewError) {
  // This kind of error is not possible, function returns zero.
  // Error state remains unknown.
  AddTransition(bindInt(0, State, C, CE), StreamState::UnknownError);
} else {
  // Add state with true returned.
  AddTransition(bindAndAssumeTrue(State, C, CE), ErrorKind);
  // Add state with false returned and the new remaining error type.
  AddTransition(bindInt(0, State, C, CE), *NewError);
}

Would it make more sense to move the logic of creating state transitions into a function instead?

424–439

It took me a while to realize that this also includes the case where the stat is not in any error :) Could you add a line of comment please?

448–451

I gave this plenty of thought, and I now believe that the state split here is appropriate. We really should ensure that the user checked both feof and ferror.

clang/test/Analysis/stream-error.c

48

That is a bit misleading, because Eof isn't EOF as specified in the standard. How about IsEof?

[balazske]

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

76–83

The name of the function may be misleading. The non-empty Optional means "yes" and an empty means "no". And if yes additional information is returned in the optional value. If the transitions would be included in this function it needs more parameters. This function is for the part of the operation that computes the remaining error ( getRemainingPossibleError is a better name?). Later it will turn out in what way the function is used and if there is code repetition a new function can be introduced, but not now (the needed state transitions may be different).

[balazske]

• Rebase
• Improved comments
• Changed UnknownError to Unknown
• Test changes (removed unneeded test)

[Szelethus]

LGTM! You seem to have a firm grasp on where you want this checker to go, and I like everything about it.

The code needs seme clang-format treatment, and another eye might not hurt, but as far as I'm concerned, I'm super happy how this patch turned out.

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

76–83

getRemainingPossibleError sounds great! You convinced me, if we ever need to stuff more things into this function, we will do that then.

[balazske]

Addressed review comments (reformat and rename).

clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp

struct StreamState {
   }
   static StreamState getOpenedWithOtherError() {
     return StreamState{Opened, OtherError};
   }
 
   void Profile(llvm::FoldingSetNodeID &ID) const {
     ID.AddInteger(State);
     ID.AddInteger(ErrorState);
   }
 };
 
 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;
 };
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, &StreamChecker::evalClearerr, 0}},
       {{"feof", 1}, {&StreamChecker::preDefault, &StreamChecker::evalFeof, 0}},
       {{"ferror", 1},
private:
   void evalFreopen(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 evalFseek(const FnDescription *Desc, const CallEvent &Call,
+                 CheckerContext &C) const;
 
   void preDefault(const FnDescription *Desc, const CallEvent &Call,
                   CheckerContext &C) const;
 
   void evalClearerr(const FnDescription *Desc, const CallEvent &Call,
                     CheckerContext &C) const;
 
   void evalFeof(const FnDescription *Desc, const CallEvent &Call,
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 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;
+
+  DefinedSVal RetVal = makeRetVal(C, CE);
+
+  // Make expression result.
+  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());
+  // There are two ways of failure:
+  // We get some error (ferror or feof).
+  ProgramStateRef StateFailedWithFError = StateFailed->set<StreamMap>(
+      StreamSym, StreamState::getOpenedWithAnyError());
+  // We get none of the error flags.
+  ProgramStateRef StateFailedWithoutFError =
+      StateFailed->set<StreamMap>(StreamSym, StreamState::getOpened());
+
+  C.addTransition(StateNotFailed);
+  C.addTransition(StateFailedWithFError);
+  C.addTransition(StateFailedWithoutFError);
+}
+
 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;
 
void StreamChecker::evalFeof(const FnDescription *Desc, const CallEvent &Call,
 
   const StreamState *SS = State->get<StreamMap>(StreamSym);
   // Ignore the call if the stream is not tracked.
   if (!SS)
     return;
 
   DefinedSVal RetVal = makeRetVal(C, CE);
 
   // Make expression result.
   State = State->BindExpr(CE, C.getLocationContext(), RetVal);
 
   if (SS->isAnyError()) {
     // We do not know yet what kind of error is set.
     // Differentiate between EOF and other error.
     ProgramStateRef StateEof, StateOther;
     std::tie(StateEof, StateOther) = State->assume(RetVal);
     assert(StateEof && StateOther &&
            "Return value should not be constrained already.");
     StateEof = StateEof->set<StreamMap>(StreamSym,
                                         StreamState::getOpenedWithEofError());
     StateOther = StateOther->set<StreamMap>(
         StreamSym, StreamState::getOpenedWithOtherError());
     C.addTransition(StateEof);
     C.addTransition(StateOther);
   } else {
     // We know what error is set, make the return value accordingly.
     State = State->assume(RetVal, SS->isEofError());
     assert(State && "Return value should not be constrained already.");
     C.addTransition(State);
   }
 }
 
 void StreamChecker::evalFerror(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;
 
   const StreamState *SS = State->get<StreamMap>(StreamSym);
   // Ignore the call if the stream is not tracked.
   if (!SS)
     return;
 
   // Make expression result.
   DefinedSVal RetVal = makeRetVal(C, CE);
   State = State->BindExpr(CE, C.getLocationContext(), RetVal);
 
   if (SS->isAnyError()) {
     // We do not know yet what kind of error is set.
     // Differentiate between EOF and other error.
     ProgramStateRef StateEof, StateOther;
     std::tie(StateOther, StateEof) = State->assume(RetVal);
     assert(StateEof && StateOther &&
            "Return value should not be constrained already.");
     StateEof = StateEof->set<StreamMap>(StreamSym,
                                         StreamState::getOpenedWithEofError());
     StateOther = StateOther->set<StreamMap>(
         StreamSym, StreamState::getOpenedWithOtherError());
     C.addTransition(StateEof);
     C.addTransition(StateOther);
   } else {
     // We know what error is set, make the return value accordingly.
     State = State->assume(RetVal, SS->isOtherError());
     assert(State && "Return value should not be constrained already.");
     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)

clang/test/Analysis/stream-error.c

void error_clearerr() {
   if (ch == EOF) {
     // FIXME: fputc not handled by checker yet, should expect TRUE
     clang_analyzer_eval(feof(F) || ferror(F)); // expected-warning {{FALSE}}
     clearerr(F);
     clang_analyzer_eval(feof(F) || 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 Eof = feof(F), Error = ferror(F);
+    clang_analyzer_eval(Eof || Error); // expected-warning {{FALSE}} \
+                                       // expected-warning {{TRUE}}
+    clang_analyzer_eval(Eof && Error); // expected-warning {{FALSE}}
+  } else {
+    clang_analyzer_eval(feof(F));   // expected-warning {{FALSE}}
+    clang_analyzer_eval(ferror(F)); // expected-warning {{FALSE}}
+  }
+  fclose(F);
+}