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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Okay, I think we're mostly in agreement so far -- could we implement a warning and add some test files for unchecked stream states after a failed `fseek` call?

The title of the revision is "[Analyzer][StreamChecker] Introduction of stream error state handling.", yet it is mostly about `fseek`, even if the intention is to demonstrate how error state handling would look like through its better modeling. How about we change the title to "[Analyzer][StreamChecker] Model fseek better by introducing stream error states"?



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:37-40
+  // NoError: No error flag is set or stream is not open.
+  // EofError: EOF condition (feof returns true)
+  // OtherError: other (non-EOF) error (ferror returns true)
+  // AnyError: EofError or OtherError
----------------
balazske wrote:
> Szelethus wrote:
> > These too. Also, I'm not yet sure whether we need `OtherError` and `AnyError`, as stated in a later inline.
> I plan to use `AnyError` for failing functions that can have **EOF** and other error as result. At a later `ferror` or `feof` call a new state split follows to differentiate the error (instead of having to add 3 new states after the function, for success, EOF error and other error). If other operation is called we know anyway that some error happened.
I think it would still be better to introduce them as we find uses for them :) Also, to stay in the scope of this patch, shouldn't we only introduce `FseekError`? If we did, we could make warning messages a lot clearer.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:333-335
+  // Ignore the call if the stream is is not tracked.
+  if (!State->get<StreamMap>(StreamSym))
+    return;
----------------
balazske wrote:
> balazske wrote:
> > Szelethus wrote:
> > > If we check in `preCall` whether the stream is opened why don't we conservatively assume it to be open?
> > If we do that we get a resource leak error for example in the test function `pr8081` (there is only a call to `fseek`). We can assume that if the function gets the file pointer as argument it does not "own" it so the close is not needed. Otherwise the false positive resource leaks come always in functions that take a file argument and use file operations. Or this case (the file pointer is passed as input argument, the file is not opened by the function that is analyzed) can be handled specially, we can assume that there is no error initially and closing the file is not needed.
> This can be done in a next change. It involves more changes at other places. I think of inserting the state for the stream if it was not there before. But we need to save if this was such an insert or a normal `fopen` (and do not report resource leak for the "insert" case).
> This can be done in a next change.

Consider me convinced :)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:351-354
+  // Set state to any error.
+  // Assume that EOF and other error is possible after the call.
+  StateFailed =
+      StateFailed->set<StreamMap>(StreamSym, StreamState::getOpenedWithError());
----------------
balazske wrote:
> Szelethus wrote:
> > But why? The standard suggests that the error state isn't changed upon failure. I think we should leave it as-is.
> The fseek can fail and set the error flag, see the example code here:
> https://en.cppreference.com/w/c/io/fseek
> Probably it can not set the **EOF** flag, according to that page it is possible to seek after the end of the file. So the function can succeed or fail with `OtherError`.
Yup, right, you won :) I tried some examples out on my system, and it could preserve or change the error state of the stream. To me it seems like that not checking the state of the stream after a failed `fseek` is surely unadvised.

This still should be `AnyError` (or `FseekError`), as according to the `OtherError`'s description `OtherError` may not refer to `EOF`, yet after a failed `fseek` call the stream can be in `EOF`:

```lang=bash
$ cat test.cpp
```
```lang=cpp
#include <cstdio>

int main() {
  FILE *F = fopen("test.cpp", "r");

  while (EOF != fgetc(F)) {}

  if (feof(F))
    printf("The file is closed!\n");

  // Return value is non-zero on failure.
  if (fseek(F, -100000, SEEK_END)) {
    if (feof(F))
      printf("The file is still closed!\n");
    else
      printf("The file is no longer closed!\n");
  }
}
```
```lang=bash
$ clang test.cpp && ./a.out
The file is closed!
The file is still closed!
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75356