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

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

balazske marked 2 inline comments as done.
balazske added inline comments.


================
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},
----------------
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).


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:485
+    State = bindInt(0, State, C, CE);
+    State = State->set<StreamMap>(StreamSym, StreamState::getOpened(Desc));
+    C.addTransition(State);
----------------
Szelethus wrote:
> Isn't the state change redundant? We have a `preCall` to this function and we assert this as well.
We need to clarify what file operations are valid if the stream is in failed state or in EOF state. Again the question is if the function will simply fail again or it does not fail but does wrong things, or it will work correctly. Currently the initial stream error state is not taken into account for `fread` and `fwrite`, this needs to be fixed. For example for `fread`: "If an error occurs, the resulting value of the file position indicator for the stream is indeterminate." So at least a next read or write or `ftell` can not work correct after this (or works but with unusable result).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374