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

Thu Aug 5 06:28:00 PDT 2021

Szelethus added a comment.

How about we just change the NoteTag to this: "Failed stream operation could have left the error or eof flags set, or the file position indicator indeterminate"? We could add a `NoteTags` to `feof` and `ferror` that narrows this down, for example:

  fread(F); // note: Failed stream operation could have left the error or eof flags set
  // ...
  if (ferror(F)) // note: Assuming F does not have the error flag set
    return;
  fread(F); // warning: Read function called when stream is in EOF state. Function has no effect.

  fread(F); // note: Failed stream operation could have left the error or eof flags set
  // ...
  if (feof(F)) // note: Assuming F does not have the eof flag set
    return;
  fread(F); // warning: Read function called when stream has the error flag set. Maybe call clearerr()?

  fseek(F); // note: Failed stream operation could have left the error or eof flags set, or the file position indicator indeterminate
  // ...
  if (feof(F)) // note: Assuming F does not have the eof flag set
    return;
  //...
  fread(F); // warning: File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior.

Seems like solid future proofing is you change your mind on emitting warnings on FERROR or we realized if some operation could ensure that the file position indicator is determinate.

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

>> Do we ever intend to create a warning for FERROR?
>
> Probably not (but there may be some not yet modeled stream operation where this may be useful.). This is when FERROR is set but not the indeterminate position. It indicates that the last operation failed but it is OK to use the stream. But this can be a "ErrorReturn" checker problem (a failed operation was not observed by the program).

I guess its a bad practice to not check whether the stream has FERROR, and I guess using another stream operation is as good of a point to check as any, so I'd definitely do it here than in ErrorReturn. But for now, alright, lets leave this for another day.

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

>> Will there ever be a way to get rid of the indeterminate position indicator tag?
>
> Currently the `fseek` can reset this flag, and the `freopen` is another case for it.

Well, sure, but they would also become the last stream operation. These Schrödinger cases are a problem because failed stream operation #1 could leave the stream object in a number of error states (e.g. `fseek`), and event #2 (e.g. checking whether the return value from `fgetc` equals to EOF)  and stream operation #3 (e.g. using `StreamTesterChecker_remove_indeterminate_file_pos_tag()`) narrowed this down to one specific error kind (e.g. its in FERROR). Basically, events #2 and #3 determine what happened at #1, and since #1 is the error causing event, we need to highlight it somehow. If #2 is an `fseek`, `freopen`, or other call that resets the entire stream object, then it will become the last event the highlight, making #1 no longer interesting at all.

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

>> No matter what happens during analysis, as of now, the NoteTag message is unambiguous at the point of the tags construction.
>
> [...]
> In this code the NoteTag message is unambiguous. The code above is a "compression" of this and at the construction of the NoteTag message is in "superposition" state too (like the error state that determines the message).

Can you give me a few test cases on this patch where the `NoteTag` indeed changes on a stream operation? My long comment meant to demonstrate that there doesn't exist such, but I'm happy to be proven wrong. My questions are about whether there *could* exist such a case in the future.

> `freopen` is a different case: At failure it returns a NULL pointer and the stream becomes in an invalid state. But if the return value of `freopen` is not assigned to it, the stream pointer is not set to NULL:
>
> - `F = freopen(0, "w", F);` F will be NULL at failure, this makes a bug with `BT_FileNull` bug type.
> - `freopen(0, "w", F);` F becomes invalid but not NULL, if used then a `BT_UseAfterOpenFailed` bug is detected (this is the only case for this bug type).
>
> At failure of `freopen` we can not tell which case will occur later (it depends on the analyzed code). (It may be possible to have only one bug for these cases but a differentation is needed to tell the user if a file was NULL or invalid but not NULL.)

Sure, but the `NoteTag` assigned to `freopen` still wouldn't change (it just wouldn't show for the former case, as writing to `F` would clear the interestingness on it).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106262