[PATCH] D75045: [analyzer] Improved check of `fgetc` in StreamChecker.

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

Szelethus requested changes to this revision.
Szelethus added a comment.
This revision now requires changes to proceed.

I'm a bit confused as to what this patch aims to do. Once again, I'd kindly ask you to discuss for a bit before updating this patch.

---

The rule states that after reaching `EOF` **both** `ferror` and `feof` should be called. I'm a bit confused here -- isn't this problem a property of `EOF` rather then `getc()` specifically? Also, expecting each maintainer of this code to call `checkErrorState` whenever a new stream function is implemented //as well as// keeping in mind that state transitions should be made against a new `ExplodedNode` makes me think that we could do better.

I think we should do some ground work before progressing further. The checker currently doesn't implement the state where we know that the stream is an error state (we got `EOF` from it). Shouldn't we do that first? After that, the rule you want to enforce may be more appropriate to originate from `checkDeadSymbols` when a stream in said state isn't checked properly, which seems to be a lot more in line with the rule.

---

The test file contains tests where `EOF` isn't encountered once but we're emitting a warning that `ferror` and `feof` should've been called, yet the rule states that

> Calling both functions on each iteration of a loop adds significant overhead, so a good strategy is to temporarily trust EOF and WEOF within the loop but verify them with feof() and ferror() following the loop.

Could we have the test cases from the CERT site, or something similar with a loop?

---

One thing that worries me is compliant non-portable solution:

  #include <assert.h>
  #include <stdio.h>
  #include <limits.h>
   
  void func(void) {
    int c;
    static_assert(UCHAR_MAX < UINT_MAX, "FIO34-C violation");
   
    do {
      c = getchar();
    } while (c != EOF);
  }

We would totally emit errors for this. There are some solutions to this:

- Assume that the user will turn off this checker in this case.
- Check the `TranslationUnitDecl` for static asserts. If not present, suggest to put it in. This would make this warning more appropriate to come from the `portability` package.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:255-261
+  if (EOFv == 0) {
+    if (const llvm::Optional<int> OptInt =
+            tryExpandAsInteger("EOF", C.getPreprocessor()))
+      EOFv = *OptInt;
+    else
+      EOFv = -1;
+  }
----------------
I'm 99% sure that the `Preprocessor` is retrievable in the checker registry function (`register*Checker`), and this code should be moved there, and `EOFv ` be made non-mutable. 


================
Comment at: clang/test/Analysis/stream.c:188
+    C = fgetc(fp);
+    fread(0, 0, 0, fp); // expected-warning {{Should call}}
+    fread(0, 0, 0, fp);
----------------
Are we sure that this is the error message we want to emit here? Sure, not checking whether the stream is in an error state is a recipe for disaster, but this seems to clash with
> Should call 'feof' and 'ferror' after failed character read.
as error here is not even checking the return value. Shouldn't we say
> Should check the return value of `fgetc` before ...
instead?


================
Comment at: clang/test/Analysis/stream.c:246-247
+    if (C == EOF) {
+      feof(fp);
+      ferror(fp);
+    }
----------------
I bet this isn't how we envision how these function to be used. Maybe `ReturnValueChecker` could be deployed here? In any case, that would be a topic of a different revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75045