[PATCH] D78374: [Analyzer][StreamChecker] Added evaluation of fread and fwrite.

Fri Apr 24 09:09:39 PDT 2020

Szelethus added a comment.

I gave a lot of thought to the high level logic, but I need some time to really internalize whats happening here.

In D78374#1993858 <https://reviews.llvm.org/D78374#1993858>, @balazske wrote:

> Finally I had to make the decision to remove the `ErrorKindTy` enum and use boolean flags instead for every possible error (including no error). This is needed because we do not know always what error is possible if it is "unknown". It could be determined from the last operation but now not any more. The documentation for `fread` says that if 0 is passed as the size or count argument the function returns zero and does not change the state. So the error state before `fread` remains active. The new design is to have bool values for every error kind (**feof** and **ferror** and a no-error state) so multiple can be active in a state to indicate that more than one error state is possible. If no-error or **feof** is possible these flags are turned on. If we need to know in such a state if the stream is in **EOF** a state split is needed to handle both cases (one with **EOF** and one with no-error). This split must be done in the pre-call handler, for example if we want a warning that the operation is not safe to use in **EOF** state. (But in this case really no split is needed, only clear the EOF flag and make a warning.)

I guess one solution would be to have a history of last operations on the stream, but overall, it makes sense to have a make a shift to calculate the possible errors as we're evaluating the functions. Great idea!

> We can have another approach if we do not set return values of the functions at all, instead save the symbol of the function and determine the returned value later from the constraints actually applied on it. This may save state splits, but only until a condition is encountered that checks for the function's return value.

Or any stream modifying operation. If we don't have branches for the return value, that is almost a bug in itself. Also, digging the return value out of a branch condition may be difficult (see `ReturnValueChecker`). Lets stick with the state splits at the function call for now.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:222
         {StreamState::FEof, StreamState::FError, StreamState::NoError}}},
-      {{"ftell", 1}, {&StreamChecker::preDefault, nullptr, 0, {}}},
+      // Note: ftell sets errno only.
+      {{"ftell", 1},
----------------
balazske wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > C'99 standard [[ http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | §7.19.9.4.3]], about the `ftell function`:
> > > 
> > > > If successful, the `ftell` function returns the current value of the file position indicator for the stream. On failure, the `ftell` function returns `-1L` and stores an implementation-defined positive value in `errno`.
> > > 
> > > So we need an evalCall for this.
> > I mean, this function can only fail if the stream itself is an erroneous or closed state, right? So we can associate the -1 return value with the stream being in that, and the other with the stream being opened.
> The `ftell` is part of a later patch that is adding `ftell` (and probably other functions). Only the "PossibleErrors" was updated in this patch because its meaning was changed (it contains value even if only one error kind is possible, before it was used only if at least two errors are possible).
> It is not sure how the file operations work, maybe the `ftell` needs access to some data that is not available at the moment (and can fail even if the stream was not OK). If the stream is already in failed state the question is: Do ftell fail (then the we can make it return -1) or it does not fail but gives an unusable value (then generate a checker warning and stop the analysis).
In any case, this should be removed from the patch, because adding it seems to be an orthogonal change to this one.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:37
+/// Note: A stream has either FEOF or FERROR set but not both at the same time.
+struct StreamErrorState {
+  /// There is an execution path with none of the error flags set.
----------------
We're not describing the error state of a stream here, but rather //possible// error states, so the name should reflect that.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:207-210
+      {{"fread", 4},
+       {&StreamChecker::preFread,
+        std::bind(&StreamChecker::evalFreadFwrite, _1, _2, _3, _4,
+                  ErrorFEof | ErrorFError),
----------------
What does this mean? I guess not the possible states after an fread call, but rather the possible states after a **failed** fread call, but then...

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:225-228
       {{"feof", 1},
        {&StreamChecker::preDefault,
-        &StreamChecker::evalFeofFerror<StreamState::FEof>,
-        0,
-        {StreamState::FError, StreamState::NoError}}},
-      // Note: ferror can result in Unknown if at the call there is a
-      // PossibleErrors with all 3 error states (including NoError).
-      // Then if ferror is false the remaining error could be FEof or NoError.
+        std::bind(&StreamChecker::evalFeofFerror, _1, _2, _3, _4, ErrorFEof),
+        0}},
----------------
...this doesn't make much sense, `feof` doesn't **cause** and error, it merely detects it.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478
+
+  // FIXME: Check for stream in EOF state?
+
----------------
Will that be the next patch? :D Eager to see it!

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:512
+
+  Optional<NonLoc> SizeVal = Call.getArgSVal(1).castAs<NonLoc>();
+  if (!SizeVal)
----------------
`castAs` doesn't return an `Optional`.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:533-539
+  ProgramStateRef StateNotFailed =
+      State->BindExpr(CE, C.getLocationContext(), *CountVal);
+  if (StateNotFailed) {
+    StateNotFailed =
+        StateNotFailed->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
+    C.addTransition(StateNotFailed);
+  }
----------------
This is where I'd really like to see a precise quote from the standard.

[[http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf | 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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78374/new/

https://reviews.llvm.org/D78374