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

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

Szelethus added a subscriber: NoQ.
Szelethus added a comment.

I have more questions than actual objections, but here we go! The patch looks nice overall, we just need to iron a few things out ahead of time.



================
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;
----------------
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?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:124
+                               const StreamErrorState &ES = ErrorNone,
+                               bool FPI = false) {
+    return StreamState{L, Opened, ES, FPI};
----------------
Let's not cheap out on this parameter name :^)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:191-193
   mutable std::unique_ptr<BuiltinBug> BT_nullfp, BT_illegalwhence,
-      BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak, BT_StreamEof;
+      BT_UseAfterClose, BT_UseAfterOpenFailed, BT_ResourceLeak, BT_StreamEof,
+      BT_IndeterminatePosition;
----------------
In time, we could create a new checker for each of these bug types, similar to how D77866 does it. But this is clearly a low prio issue, I'm just talking aloud.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:306
+  ProgramStateRef
+  ensureNoFilePositionIndeterminate(SVal StreamVal, CheckerContext &C,
+                                    ProgramStateRef State) const;
----------------
Ooooor `ensureFilePositionDeterminate`? :D


================
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())
----------------
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?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:854
+      }
+      return State; // FIXME: nullptr?
+    }
----------------
Good question. It would make sense... @NoQ, @xazax.hun?


================
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>(
----------------
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.


================
Comment at: clang/test/Analysis/stream-error.c:182
+    } else {
+      fwrite(Buf, 1, 10, F); // expected-warning {{might be 'indeterminate'}}
+    }
----------------
Here the user checked whether F is in eof or in ferror, and its in neither. Isn't it reasonable to assume then that the stream is fine?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80018