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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.


================
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);
----------------
Szelethus wrote:
> balazske wrote:
> > Szelethus wrote:
> > > 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?
> > No, it is not enough to check the return value of `fgetc`, exactly this is the reason for the checker. "Should call 'feof' and 'ferror' after failed character read." is more exact: The read was suspected to fail because EOF was returned but should call feof (...) to verify it. A better message can be:
> > 
> > If 'fgetc' returns EOF call 'feof' and 'ferror' to check for error.
> The compliant solution in the rule description has the following example:
> 
> ```lang=c++
> 
> #include <stdio.h>
>  
> void func(void) {
>   int c;
>  
>   do {
>     c = getchar();
>   } while (c != EOF);
>   if (feof(stdin)) {
>     /* Handle end of file */
>   } else if (ferror(stdin)) {
>     /* Handle file error */
>   } else {
>     /* Received a character that resembles EOF; handle error */
>   }
> }
> ```
> because if the character resembles `EOF` that is an error too, and also
> 
> > 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.
> 
> Which makes me thing that the error here is not checking against `EOF`, first and foremost.
> 
> The warning message
> 
> >If 'fgetc' returns EOF call 'feof' and 'ferror' to check for error.
> 
> sounds great, I would just prefer to see it //after// we **know** that the stream returned `EOF`.
I agree. The checker should emit a warning if and only if the return value of `fgetc()` equals to `EOF`. Another checker should ensure that the return value of `fgetc()` is indeed compared to `EOF`. However: are we sure that there are no cases were no other error may happen than //EOF// thus we intentionally omit further checking? (Imagine a simple read from the keyboard in a command line utility.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75045