[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

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

balazske marked an inline comment as done.
balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:393-394
+
+  // Make the return value accordingly to the error.
+  State = State->assume(RetVal, (SS->*IsOfError)());
+  assert(State && "Return value should not be constrained already.");
----------------
Szelethus wrote:
> Is this correct? We set the `feof()` return value whether we know the state of the stream to be EOF, but in this case we would incorrectly mark it as non-EOF: 
> ```lang=bash
> $ cat a.txt # contains a single newline
> 
> $ cat test.cpp 
> ```
> ```lang=cpp
> #include <cstdio>
> 
> void tryRead(FILE *F) {
>   char C = fgetc(F);
> 
>   if (feof(F))
>     printf("The stream is EOF!\n");
>   else
>     printf("The stream is good! Read '%c'\n", C);
> }
> 
> int main() {
>   FILE *F = fopen("a.txt", "r");
>   tryRead(F);
>   tryRead(F); // Incorrectly mark F to be non-EOF
> }
> ```
> ```lang=bash
> $ clang test.cpp -fsanitize=address && ./a.out 
> The stream is good! Read '
> '
> The stream is EOF!
> ```
> 
> Wouldn't it be safer to assume it to be `true` if we **know** its EOF and do nothing otherwise? How about a state split?
> 
> (I know this functions also handler FError, but you see what my point is.)
The problem in this code seems to be that `fgetc` is not handled (yet) by the checker. The plan is to handle every possible file operation. The handler of `fgetc` would split the state to EOF and non-EOF case. In the current state this is a real "bug", it could be handled somehow by checking for escaped stream (the `fgetc` here should cause escape of the file pointer if not recognized as file operation). But this means again a new patch before this one. Or add every file operation to this patch with some simple implementation or with "forgetting" the stream?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75682