[PATCH] D80015: [Analyzer][StreamChecker] Added support for 'fread' and 'fwrite'.

Tue May 19 05:22:29 PDT 2020

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

This patch is great. LGTM!

In D80015#2043263 <https://reviews.llvm.org/D80015#2043263>, @balazske wrote:

> If the unit of the change is adding `fread` and `fwrite` completely, the warning with FEOF at `fread` is correct to add because it belongs to `fread`. I was planning to add some file handling functions to the checker that have a basic functionality implemented. In this case, it is the fread-fwrite functions, but the "indeterminate file position" is still missing here (that is a bigger change), so it is the "basic functionality" that is added. The FEOF warning is probably too small to add it in a separate patch, also it is added only to one place now.

Very well, I'm sold. I didn't check that we don't really model another function that needs it. Woops :^)

> There is intentionally no warning about ferror state, first reason is that often ferror and "indeterminate file position" comes together and then the warning will be produced for "indeterminate position" in the next change. Next reason is that ferror in itself is not always cause for warning and we can currently not decide about it. There was the example about write into a file that is open in read mode that results in a ferror state, then the next read should work (with ferror state at start). Another reason, the program should still always handle if a file operation fails. If an operation starts in ferror state, it will probably fail again but that error should be handled by the application.
> 
> Intent of these warnings is not to check that the error case was handled by the user code, that is a more difficult thing. We could make a new warning that warns if between two possible failing operations no `ferror` or `feof` was called, this would check if the user handled the error. But not accurate because the error can be handled based on the return value of the failed function, without calling `ferror` or `feof` (except some special cases like the `fgetc`). The only safe warning is to add if a function is called in really unuseful way, for example fread in already FEOF state that do never succeed, and the "indeterminate file position" case.

The question is what is a good practice on ferror -- a check on `errno` and then a `clearerr`? In any case, you are right, this another discussion for another time.

> My goal with these changes was to have a check for the CERT FIO34-C case, not for the complete `StreamChecker` that could contain many other features. So probably not even all file handling functions will be added for now.

Sure, shoot me down if you feel I'm trying to dictate the direction of your project! :)

================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:523
+  // FEOF).
+  if (!IsFread || (SS->ErrorState != ErrorFEof)) {
+    ProgramStateRef StateNotFailed =
----------------
balazske wrote:
> Szelethus wrote:
> > Aaaah okay it took me a while. I think `SS->ErrorState != ErrorFEof` isn't as talkative as `isConstrained.*`, could you add just a line like "if we **know** the state to be EOF..."?
> > 
> > On another note, why is fwrite so special in this context?
> If it is an `fwrite`, the function call can always succeed (even after a previously EOF or error).
> If it is an `fread`, it can succeed but not after an exactly-EOF state.
> 
> A success transition is not needed in an `fread` with exactly EOF condition. (If we have fread with non-exactly EOF, for example `ErrorFEof` and `ErrorFError`, the success is needed because the read in `ErrorFError` can succeed.)
> Aaaah okay it took me a while. I think SS->ErrorState != ErrorFEof isn't as talkative as isConstrained.*, could you add just a line like "if we know the state to be EOF..."?

Could you please add these few words before commiting?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80015