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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
