Szelethus added a comment. In D75045#1891064 <https://reviews.llvm.org/D75045#1891064>, @balazske wrote:
> In D75045#1891056 <https://reviews.llvm.org/D75045#1891056>, @NoQ wrote: > > > In D75045#1891055 <https://reviews.llvm.org/D75045#1891055>, @NoQ wrote: > > > > > How many such platforms can you name? > > > > > > I mean, does Clang even support such targets? > > > The problem can occur with the "wide" variants of the getc functions when > `wchar_t` is same width as `wint_t`, this can be more often (but I am not > sure). Even then the character set should contain a character similar to > `WEOF`, that is not true for UTF-16 and 32. So there may be low probability > for the problem to occur. Is this reason to not have this check at all? If we can detect that the code is non-portable, it doesn't really matter in my opinion if we don't support the target on which the problem would occur. Also, I can't come up with a specific platform that would have an issue like this (maybe in a dishwasher? (: ), but CERT is known to create rules based on real vulnerabilities. A quick glance lead me to this page, though these vulnerabilities may not be directly related to this rule: https://wiki.sei.cmu.edu/confluence/display/c/AA.+Bibliography#AA.Bibliography-ISO/IECTS17961-2013 In D75045#1891032 <https://reviews.llvm.org/D75045#1891032>, @balazske wrote: > An improvement can be made if the check runs only when size of `char` and > `int` are equal. Since the rule specifically states that the problem here is portability related, I think we shouldn't do that. > The code can be improved somewhat but there are now too many special cases > and there is already a default eval function. Probably it is better to have > every "check" function take and return `ExplodedNode` instead of state. This really ties into my main concern, that this **isn't** a stream management function specific problem, but rather an unchecked stream problem **after** we know that it returns `EOF`. I think we should first emit warnings on operations on `EOF`-returning streams, and implement this rule after that. WDYT? ================ 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; + } ---------------- balazske wrote: > Szelethus wrote: > > 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. > I am not sure if this would be good. The `Preprocessor` is not available > easily and the "register" function is probably there to register a checker > and do nothing else. Probably the source code is not even known when it is > called. And how to pass the EOF value to the created checker in the register > function (if not a static variable is used)? `CheckerManager::registerChecker` in fact returns the non-const checker object. The registry function is responsible for the registration //and// initialization of the checker object. Though you have a point in the sense that this isn't terribly well defined (I should really finish D58065...). I had a look, and my usual motto of //"Any manager can be retrieved in clang at arbitrary locations if you try hard enough"// seems to have been wrong. I don't see how I could get my hands on a `Preprocessor` in the checker registry function. So, feel free to disregard this comment. ================ 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); ---------------- 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`. ================ Comment at: clang/test/Analysis/stream.c:246-247 + if (C == EOF) { + feof(fp); + ferror(fp); + } ---------------- balazske wrote: > Szelethus wrote: > > 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. > If `fgetc` returns non-EOF we can be sure that there is no error, so no need > to call `ferror` and `feof`. If it returns EOF we must "double-check" that it > was a real error or an EOF-looking character that was read. Yea, but shouldn't we check the return values of `ferror` and `feof`? Otherwise whats the point? 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
