[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:478
+
+  // FIXME: Check for stream in EOF state?
+
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > Will that be the next patch? :D Eager to see it!
> > It may be too big change to add it as well here (or cause more difficulties). But it should be a  check for `ErrorFEof` in the `ErrorState` (and add another bug type). This is not a fatal error.
> > 
> > It may be more difficult to make `fread` return the feof error again if it starts already with feof state. (Because the state with FEof should be split from the generic error state that can contain other possible errors.)
> > It may be too big change to add it as well here (or cause more difficulties). But it should be a check for ErrorFEof in the ErrorState (and add another bug type). This is not a fatal error.
> 
> Oh, yea, right, we went over this once :) What I really meant, is an EOF related error, like reading a variable with an EOF value in any context that isn't a comparison to the actual `EOF` (which in many contexts still isn't a //fatal// error, but is far more an indicator of code smell). But I'm just thinking aloud, please proceed with this project however it is convenient to you!
> 
> > It may be more difficult to make fread return the feof error again if it starts already with feof state. (Because the state with FEof should be split from the generic error state that can contain other possible errors.)
> 
> I gave this plenty of thought, how do you imagine the problem of us not splitting up to 3 states to show up? Suppose we're calling fread on a stream where the error state is either EOF or ERROR, but we don't know which. We could just leave the `StreamErrorState` as-is, couldn't we?
> reading a variable with an EOF value in any context that isn't a comparison to the actual EOF
This could be another checker, or it can be integrated somehow into `ErrorReturnChecker` (that does something similar already). I mean, remembering if a variable contains **EOF** that comes from a function that may return **EOF** (otherwise the program can do nothing with a simple -1 numerical value without getting the warning), then if any other thing is done with it than comparison to **EOF** (or passing it to other function) it can be an error case.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478
+
+  // FIXME: Check for stream in EOF state?
+
----------------
balazske wrote:
> Szelethus wrote:
> > balazske wrote:
> > > Szelethus wrote:
> > > > Will that be the next patch? :D Eager to see it!
> > > It may be too big change to add it as well here (or cause more difficulties). But it should be a  check for `ErrorFEof` in the `ErrorState` (and add another bug type). This is not a fatal error.
> > > 
> > > It may be more difficult to make `fread` return the feof error again if it starts already with feof state. (Because the state with FEof should be split from the generic error state that can contain other possible errors.)
> > > It may be too big change to add it as well here (or cause more difficulties). But it should be a check for ErrorFEof in the ErrorState (and add another bug type). This is not a fatal error.
> > 
> > Oh, yea, right, we went over this once :) What I really meant, is an EOF related error, like reading a variable with an EOF value in any context that isn't a comparison to the actual `EOF` (which in many contexts still isn't a //fatal// error, but is far more an indicator of code smell). But I'm just thinking aloud, please proceed with this project however it is convenient to you!
> > 
> > > It may be more difficult to make fread return the feof error again if it starts already with feof state. (Because the state with FEof should be split from the generic error state that can contain other possible errors.)
> > 
> > I gave this plenty of thought, how do you imagine the problem of us not splitting up to 3 states to show up? Suppose we're calling fread on a stream where the error state is either EOF or ERROR, but we don't know which. We could just leave the `StreamErrorState` as-is, couldn't we?
> > reading a variable with an EOF value in any context that isn't a comparison to the actual EOF
> This could be another checker, or it can be integrated somehow into `ErrorReturnChecker` (that does something similar already). I mean, remembering if a variable contains **EOF** that comes from a function that may return **EOF** (otherwise the program can do nothing with a simple -1 numerical value without getting the warning), then if any other thing is done with it than comparison to **EOF** (or passing it to other function) it can be an error case.
> Suppose we're calling fread on a stream where the error state is either EOF or ERROR, but we don't know which. We could just leave the StreamErrorState as-is, couldn't we?
 * If `fread` is called with **FEOF** flag, it returns 0 and **FEOF** remains.
 * If `fread` is called with **FERROR** flag, it may fail (with any error) or not. This is similar as calling `fread` in no-error state.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374