[PATCH] D106262: [clang][analyzer] Use generic note tag in alpha.unix.Stream .

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

Szelethus added a comment.

In D106262#2915764 <https://reviews.llvm.org/D106262#2915764>, @balazske wrote:

> The bug type is passed to `constructNoteTag` only to identify what message will be displayed. The bug types that are related to the current function (a message should be here if the bug is happening) are passed in. It should be enough to look at comment before `constructNoteTag` to check this (and to the bug type -> message map).
>
>   C.addTransition(StateFailed, constructNoteTag(C, StreamSym,
>                                                 {&BT_StreamEof,
>                                                  &BT_IndeterminatePosition}));
>
> This indicates that if later an EOF error happens (function called in EOF state) this is the place to display a message "stream becomes EOF here". And if later an error "stream position indeterminate" happens the note should be here too, but with other message. So we can not have a single string as last parameter instead of the bug types.

Right, okay, I see that I looked over something important.

I forgot our optimization trickery. It is reminiscent of a Schrödinger's cat: after a failed stream operation, the stream object could simultaneously be in either of these states:

- Reached EOF,
- Have an indeterminate file position,
- Have ferror set.

TL;DR:
------

No matter what happens during analysis, as of now, the NoteTag message is unambiguous at the point of the tags construction. Do we have plans to change our current modeling around indeterminate file positions, or add a warning for FERROR that would necessitate the proposed complexity?

Onto the rest:
--------------

Its at the next stream operation where its decided what state it was actually put in. So, if a stream operation like `fseek` fails, it may leave the stream object in any of the above states. Later, when we analyze `fread`, we check in `preFread` //in order// whether the stream is null, is opened, its file position indicator is determinate, and whether the stream has EOF set. Whichever error is detected first will be the one to get emitted, and in this case, its going to be an indeterminate file position.

  void f(FILE *f) {
    fseek(f, offset, origin); // note: stream operation failed
    fread(ptr, size, count, f); // warn: indeterminate file position
  }

Now, lets check explicitly whether `fseek` left the stream in FERROR, and return early if so. Lets discuss how `fseek` should work another time, but for the sake of this example, *suppose* that it only leaves the file position indicator indeterminate if it also sets FERROR, and `ferror` respects this supposed behaviour. In that case, only the EOF flag remains.

  void f(FILE *f) {
    fseek(f, offset, origin); // note: assuming stream reached eof
    if (ferror(f)) // note: assuming the condition is false
      return;
    fread(ptr, size, count, f); // warn: stream already in eof
  }

The bug type changed, and it would be handy if the note tag changed as well. It really is the inspection of the bug type (hence the Schrödinger-like behaviour) that decides what error was actually inflicted upon the stream object at `fseek`.

---

Now, its worth noting that in practice, NoteTags never change their message. Lets take a survey:

  BugType BT_FileNull{this, "NULL stream pointer", "Stream handling error"};
  BugType BT_UseAfterOpenFailed{this, "Invalid stream",
                                "Stream handling error"};

The only meaningful NoteTag is for both of these is added around a failed (re)open, and the emitted string shouldn't depend on which one it is. Stream objects are never in a "schrödinger-like" state in terms of these bugs -- a stream is either NULL, or NULL as a result of a failed open.

  BugType BT_IllegalWhence{this, "Illegal whence argument",
                           "Stream handling error"};

This is practically a statement-local issue, and even if the whence argument was calculated elsewhere, its hardly the job of this checker to track down.

  BugType BT_UseAfterClose{this, "Closed stream", "Stream handling error"};

The only noteworthy event here is where was the stream was closed. Stream closure cannot fail, the only state a stream can remain in after a call to `close` is it being closed, so the note message is unambiguous.

  BugType BT_ResourceLeak{this, "Resource leak", "Stream handling error",
                          /*SuppressOnSink =*/true};

The only noteworthy event is stream opening. Now, opening can fail, but we split the state immediately there, and we can either write "Opened stream here", or "Opening stream failed here". On each of these path, this message is unambiguous.

  BugType BT_IndeterminatePosition{this, "Invalid stream state",
                                   "Stream handling error"};
  BugType BT_StreamEof{this, "Stream already in EOF", "Stream handling error"};

Alright, so this is the juicy one. As demonstrated above, a number of stream operations can result in 3 types of erroneous stream states. However, even here, the note tag message in unambiguous, mostly because of two reasons:

- We don't emit warnings for FERROR (as you've said in D80015#2043263 <https://reviews.llvm.org/D80015#2043263>),
- As of now, you cannot get rid of indeterminate file position indicator. `clearerr()` (rightfully) leaves it as-is, branching on `ferror` leaves it on unconditionally.

So, because we check indeterminate file position is a more serious error then an EOF error (or FERROR, if we were to emit a warning for it), if a stream operation *could* result in an indetermiante file position, the note tag will *always* be that the stream was left in it. In any other case, the note message will be that the operation left the stream object at EOF.

In this table, you can see the possible flags the stream object can hold at the NoteTag construction point, what the note tag message *will* be, and what the resulting bug type *will* be.

| EOF   | FERROR | Indet. pos | Note message at the stream operation                          | Possible bug types       |
| true  | true   | true       | stream object's file position indicator is left indeterminate | BT_IndeterminatePosition |
| true  | false  | true       | stream object's file position indicator is left indeterminate | BT_IndeterminatePosition |
| false | true   | true       | stream object's file position indicator is left indeterminate | BT_IndeterminatePosition |
| false | false  | true       | stream object's file position indicator is left indeterminate | BT_IndeterminatePosition |
| true  | false  | false      | stream object reached EOF here                                | BT_StreamEof             |
| true  | true   | false      | stream object reached EOF here                                | BT_StreamEof             |
| false | true   | false      | nothing                                                       | nothing                  |
|

Note that the only way to get of the indeterminate file position is either to reopen the stream, but if that succeeds, all stream error flags are reset, or call `fseek` or something similar again, but then that would be the latest event, and the NoteTag would be placed there.

---

Lets conclude with a few questions:

- Do we ever intend to create a warning for FERROR? If so, can we add it before this patch to test the change on the NoteTag message?
- Will there ever be a way to get rid of the indeterminate position indicator tag (possibly by getting rid of the FERROR flag at the same time)? If so, can we add a `void StreamTesterChecker_remove_indeterminate_file_pos_tag(FILE *)` function and test the change on the NoteTag message?

If the answer to both of those is no, then we don't need this complexity ;)



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:499
+                  constructNoteTag(C, RetSym, {&BT_ResourceLeak}));
+  C.addTransition(StateNull, constructNoteTag(C, RetSym, {&BT_FileNull}));
 }
----------------
Maybe we could add `BT_UseAfterOpenFailed`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106262