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

Wed May 6 05:20:30 PDT 2020

Szelethus added inline comments.

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:478
+
+  // FIXME: Check for stream in EOF state?
+
----------------
balazske wrote:
> 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.
> 
> * 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.

Is it ever possible that an fread on a stream in FERROR returns non-zero and changes the streams state? Lets unpack this.

§7.19.8.1.2 states that
> The fread function reads, into the array pointed to by ptr, up to nmemb elements whose size is specified by size, from the stream pointed to by stream. **For each object, size calls are made to the fgetc function and the results stored, in the order read, in an array of unsigned char exactly overlaying the object.** The file position indicator for the stream (if defined) is advanced by the number of characters successfully read. If an error occurs, the resulting value of the file position indicator for the stream is indeterminate. If a partial element is read, its value is indeterminate.

§7.19.7.1.3 talks about the return value of fgetc:

> **If the end-of-file indicator for the stream is set, or if the stream is at end-of-file, the end-of-file indicator for the stream is set and the fgetc function returns EOF.**  Otherwise, the fgetc function returns the next character from the input stream pointed to by stream. **If a read error occurs, the error indicator for the stream is set and the fgetc function returns EOF.**

Now, speaking of fgetc, its true that the standard doesn't specifically talk about what happens if the stream is already in error state, yet it does mention what happens if it is in eof-state, but then again, I guess its strongly implied that if the stream is an error state, a read error will occur right away, and the error state is preserved as specified.

If fread is little more then sequential calls to fgetc, I would believe that a stream is in FERROR, FERROR will be preserved, because fgetc wouldn't be able to read a single character. Additionally, as mentioned in another inline, §7.19.8.1.3 talks about the return value of fread:

> **The fread function returns the number of elements successfully read**, which may be less than nmemb if a read error or end-of-file is encountered. If size or nmemb is zero, fread returns zero and the contents of the array and the state of the stream remain unchanged.

so that should be 0.

To conclude, unless I'm wrong (which is totally possible, I'm just throwing standard cites around wanting to prove how I hope these things work), both types of stream errors would be preserved after a call to fread, and the return value would always be zero.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78374