[PATCH] D80018: [Analyzer][StreamChecker] Added check for "indeterminate file position".

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:107
+  /// This value applies to all error states in ErrorState except FEOF.
+  /// An EOF+indeterminate state is the same as EOF state.
+  bool FilePositionIndeterminate = false;
----------------
balazske wrote:
> Szelethus wrote:
> > balazske wrote:
> > > Szelethus wrote:
> > > > Szelethus wrote:
> > > > > What does this mean? "An EOF+indeterminate state is the same as EOF state." I don't understand the message you want to convey here -- is it that we cannot have an indeterminate file position indicator if we hit EOF, hence we regard a stream that is **known** to be EOF to have its file position indicator determinate?
> > > > > 
> > > > > A good followup question for the uninitiated would be that "Well why is it ever legal to construct a `StreamState` object that can both have the `FilePositionIndeterminate` set to true and the `ErrorState` indicate that the steam is **known** to be in EOF?" Well, the answer is that we may only realize later that the error state can only be EOF, like in here:
> > > > > ```lang=c++
> > > > > void f() {
> > > > >  FILE *F = fopen(...);
> > > > >  if (fseek(F, ...)) {
> > > > >     // Could be either EOF, ERROR, and ofc indeterminate
> > > > >     if (eof(F)) {
> > > > >       // This is where we have a seemingly impossible stream state, but its not a programming error, its a design decision.
> > > > >     }
> > > > > }
> > > > > ```
> > > > > This might warrant a bit on explanation either here, or in `ensureNoFilePositionIndeterminate`. Probably the latter.
> > > > > 
> > > > > With that said, can `SteamState`'s constructor ensure that we do not create a known to be EOF stream that is indeterminate?
> > > > Actually, not enforcing this could leave to false positives:
> > > > 
> > > > ```
> > > > void f() {
> > > >  FILE *F = fopen(...);
> > > >  if (fseek(F, ...)) {
> > > >     // Could be either EOF, ERROR, and ofc indeterminate
> > > >     if (eof(F)) {
> > > >       clearerr(F);
> > > >       fseek(F, ...); // false positive warning
> > > >     }
> > > > }
> > > > ```
> > > The comment wants to say only that if the **ErrorState** contains the **ErrorFEof** the value of `filePositionIndeterminate` is to be ignored for the EOF case. If the file is in EOF it does not matter what value `filePositionIndeterminate` has. The cause for this handling is that ErrorState handles multiple possible errors together but the indeterminate position does not apply to all. If **ErrorState** contains **ErrorFEof** and **ErrorFError** together and the `filePositionIndeterminate` is set, the position is not indeterminate in the EOF case. For EOF case we should know that the position is at the end of the file, not indeterminate.
> > > 
> > > Another solution for this problem can be to have a "ErrorFErrorIndeterminate" and "ErrorNoneIndeterminate" error type but this makes things more difficult to handle.
> > What do you mean under the term "the EOF case"? When we **know** the stream to only be in the EOF state? The overall modeling seems correct, its just that little corner case that if we **know** that the stream hit EOF, the file position must be determinate.
> The "difficult" case is when we do not know if the stream is in error or EOF state. We know only that it is in one of these. And the `FilePositionIndeterminate` is set to true. In this state `FEof` and `FError` are true in `ErrorState`. If the program should know what the error is, it needs to generate 2 new states, one with only `FEof` true and other with `FError`. And how to set the `FilePositionIndeterminate` for the new states? For `FError` it must be set to true because it was true before, for `FEof` it is set to false because by definition it is not applied to `FEof` case. (This last is //the EOF case// above.)
> 
> This table shows what (real) stream state is represented (values at right) by what values in `StreamState` (values at left side):
> | FEof | FError | FilePositionIndeterminate | stream feof? | stream ferror? | position indeterminate? |
> | false | false | true | false | false | true |
> | true | false | true | true | false | false |
> | false | true | true | false | true | true |
> | true | true | true | - | - | - |
Right. How about we make this field private and create a an `isFilePositionIndeterminate()` method that ensures that for the second row of that table we always return false? That would take some responsibility off of this interface's users.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:841-842
+    // FEof is ignored).
+    if (SS->ErrorState.isFEof())
+      return State->set<StreamMap>(Sym, NewState);
+
----------------
This branch could be removed in its entirety.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:854
+      }
+      return State; // FIXME: nullptr?
+    }
----------------
Szelethus wrote:
> Good question. It would make sense... @NoQ, @xazax.hun?
Well, looking at other checkers, they usually early return and don't modify the state if the creation of the error node failed, so a nullptr return would be more appropriate.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:857-868
+    // FEof and other states are possible.
+    // The path with FEof is the one that can continue.
+    // For this reason a non-fatal error is generated to continue the analysis
+    // with only FEof state set.
+    ExplodedNode *N = C.generateNonFatalErrorNode(State);
+    if (N) {
+      C.emitReport(std::make_unique<PathSensitiveBugReport>(
----------------
Szelethus wrote:
> Ugh, tough one. I think this just isn't really *the* error to highlight here, but rather that its bad practice to not check on the stream after an operation. But I'm willing to accept I might be wrong here.
Actually, I take this back. If we had `NoteTag`s to explain that "this stream operation failed and left the stream file position indication in an indeterminate state", this would be great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80018