[PATCH] D72705: [analyzer] Added new checker 'alpha.unix.ErrorReturn'.
Kristóf Umann via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 7 08:20:16 PDT 2020
Szelethus added a comment.
In D72705#2199333 <https://reviews.llvm.org/D72705#2199333>, @balazske wrote:
> Test results for tmux are available here <https://codechecker-demo.eastus.cloudapp.azure.com/Default/reports?run=tmux_errorreturn&review-status=Unreviewed&review-status=Confirmed%20bug&detection-status=New&detection-status=Reopened&detection-status=Unresolved>. (Could not manage to get other results uploaded.)
I guess this is a somewhat smarter version, correct? You seem to have `NoteTag`s and handling for `localtime` etc. Anyways, I trust that the logic of mostly the same.
I don't know how likely it is for time management functions to fail, I guess its a rarity, but I guess this is exactly what static analysis is for. I think these results are nice, but as cheap of a response this is, I crave some more. You specifically mentioned a few false positives, I wonder what is up with them.
In D72705#2161576 <https://reviews.llvm.org/D72705#2161576>, @balazske wrote:
> This checker can have two purposes, one is to verify that all paths have the error check, other is to find at least one path without error check. The first is the one that can be done with dataflow based analysis but looks like a difficult problem. For example is it possible to handle this case (`X` does not change)?
>
> int Ret;
> if (X)
> Ret = fgetc(fd);
> else
> Ret = strtol(SomeString);
> ...
> bool IsError;
> if (X)
> IsError = (Ret == EOF);
> else
> IsError = (Ret < -100 || Ret > 100);
>
> The other "purpose" is to find one path with missing error check, the current code should do something like that. The path sensitivity is used here to store for a symbol from which function call it comes from, and probably to determine value of other symbols in the program if they appear in a comparison.
You are right, dataflow definitely has shortcomings that pathsensitivity doesn't, but it is true the other way around as well, which is why I believe (D72705#2141439 <https://reviews.llvm.org/D72705#2141439>) the combination of the two would be the correct solution, even if the question is whether the return value is checked on a given path of execution (that is precisely what the example in the comment demonstrates, not to mention that it could be far more obfuscated).
---
I think we have dedicated a lot of time to discuss our long term plans. I feel confident in my stance regarding the future of this checker, you seem to have a good grasp on it, we've gotten feedback a wide range of reviewers, and we also have this as a feature request outside Ericsson. Since this patch intends to lend an alpha checker, I think its about time we start talking about moving forward.
Here is what I like about this patch and want to see it in clang sooner rather than later:
- Documentation is great. Its hard to overstate the value of that, when I literally have to sink in weeks into a codebase to understand whats going on due to the lack of it.
- @NoQ mentioned the callback choice may not be ideal/wrong, but I personally disagree. Its far easier, and I think cheaper to climb up on the AST. The high level idea behind the visitor analyzing whether a statement constitutes as a check is great, I don't we have anything similar.
- I think you tested the added functionality for an alpha checker quite well.
- If my worries (did we think about all the cases that could constitute as a check?) won't be an issue, the infrastructure proposed by this patch seems easily expandable.
Here is what I want to see moving forward:
- This comment D72705#2088319 <https://reviews.llvm.org/D72705#2088319> worries me. @NoQ, could you please expand on this? I feel like you have a legitimate worry I'm not seeing, and I perceive you to be a bit skeptical about this whole shebang. While I'd hate to stall the progress of this patch, it'd be nice to be settled on this before moving too fast forward.
- More evaluations. I realize that reviewers don't demand too much on this front for an initial patch for an alpha checker, but both the problem and our different visions on the solution could use some backing up. It would be a shame if an issue we could've easily foreseen came about that would require serious architectural changes. What was the problem in uploading the results you already had on hand?
I left some inlines, but I didn't go too nitty just yet. For the time being, lets have these two goals met, and after that I'll definitely do my best to help you get this landed ASAP.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:55-60
+ /// Test if an encountered binary operator where the return value is involved
+ /// is a valid check statement. The return value appears in one side of the
+ /// operator (`ChildIsLHS` indicates if it is on the LHS). If the other side
+ /// contains a known (mostly constant) value, it is already calculated in
+ /// `KnownValue`. `RetTy` is the type of the return value (return type of the
+ /// function call in the code to check).
----------------
So, for the time being we're saying that a checking must involve a binary operator. That sounds about right, but I wonder if there is a case we're not thinking about.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:63
+ const BinaryOperator *BinOp,
+ const llvm::APSInt *KnownValue,
+ QualType RetTy,
----------------
This is the value we're checking //against//, I think the name `KnownValue` doesn't convey this.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:79-80
+ bool ChildIsLHS) const override {
+ if (!KnownValue)
+ return false;
+
----------------
So if we failed to get retrieve the value we're checking against, we automatically assume that its not a proper check? Shouldn't we be conservative here? What if that value is something like `getEOFValue()`?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:83
+ bool KnownNull = KnownValue->isNullValue();
+ bool KnownEOF = ((*KnownValue) == BVF.getValue(-1, RetTy));
+
----------------
We have a smarter EOF getter thanks to @martong, don't we?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:284-287
+ // It is assumed that any system function does not accept value that is
+ // an error return value from any other function.
+ // In other words error check is required before system function is called
+ // with the value.
----------------
This is a very broad statement. I can't come up with a counterexample on top of my head, but we should keep this in mind. What this totally deserves however, is a distinct warning message, like "xth argument to system/standard function call is an unchecked return value".
Please put a TODO here.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:295-296
+
+ // Value appears in other expression.
+ VisitResult VisitExpr(const Stmt *S) { return NoCheckUseFound; }
+
----------------
Aha, interesting. So the thing to note here is that if the parent (which is stripped of all the cast/paren expressions) in a non-call expression, it constitutes as an improper use before checking. Again, I don't have an immediate counter example, but let's keep this in mind as a likely source of false positives. Which may not be too bad, we'll at least immediately be notified that there are additional cases to handle.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:329-330
+ // Value appears in other statement.
+ // FIXME: Statements that affects control-flow should be checked separately.
+ // For example `Child` may appear as a condition of `if`.
+ VisitResult VisitStmt(const Stmt *S) { return StopExamineNoError; }
----------------
Why?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrorReturnChecker.cpp:428-432
+ // Check for explicit cast to void.
+ if (auto *Cast = dyn_cast<const CStyleCastExpr>(S)) {
+ if (Cast->getTypeAsWritten().getTypePtr()->isVoidType())
+ return nullptr;
+ }
----------------
Ah, okay, so you mean to check whether someone did something like this:
```lang=c++
// Silence compiler warning.
(void)localtime(...);
```
I don't think that the parent though will get you the correct answer, how about this:
```lang=c++
// Silence compiler warning.
(void) (coin() ? NULL : localtime(...));
```
Please put a TODO here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72705/new/
https://reviews.llvm.org/D72705
More information about the cfe-commits
mailing list