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

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 4 inline comments as done.
balazske 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;
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:306
+  ProgramStateRef
+  ensureNoFilePositionIndeterminate(SVal StreamVal, CheckerContext &C,
+                                    ProgramStateRef State) const;
----------------
Szelethus wrote:
> Ooooor `ensureFilePositionDeterminate`? :D
It is better to call "Invalid" or "Unknown" position, "indeterminate" is taken from the text of the C standard. I think "indeterminate" is a special name here that is better to have in this form always.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:589
+  // other code.
+  StreamState NewState = StreamState::getOpened(Desc, NewES, true);
   StateFailed = StateFailed->set<StreamMap>(StreamSym, NewState);
----------------
This is a place where `FilePositionIndeterminate` is set in the state. We do not know if FEOF or FERROR will happen. If it is FERROR the position is indeterminate, if it is FEOF the position is not indeterminate.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:839-840
+
+    // If only FEof is possible, report no warning (indeterminate position in
+    // FEof is ignored).
+    if (SS->ErrorState.isFEof())
----------------
Szelethus wrote:
> I think we shouldn't say its ignored, but rather its impossible. I mean, if we have reached the end of the file, how could the file position indicator be indeterminate?
The previous comment tells why it is set. The FEOF case is represented in the data structure with indeterminate flag set or not set. Both mean the same state of the modeled stream: FEOF and not indeterminate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80018