a.sidorin added a comment.
Thank you for your work, Zoltán!
Did you checked if same warnings may be emitted by another checkers? For
example, ArrayBoundChecker may warn if index is tainted.
I also have some comments below.
================
Comment at: lib/StaticAnalyzer/Checkers/DirtyScalarChecker.cpp:47
+ void checkPreStmt(const BinaryOperator *BO, CheckerContext &C) const;
+ void checkBranchCondition(const Stmt *Cond, CheckerContext &Ctx) const;
+
----------------
`CheckerContext &C` to unify naming.
================
Comment at: lib/StaticAnalyzer/Checkers/DirtyScalarChecker.cpp:51
+ void checkLoopCond(const Stmt *Cond, CheckerContext &C,
+ int RecurseLimit = 3) const;
+ void checkUnbounded(CheckerContext &C, ProgramStateRef PS,
----------------
The default choice of such a value needs some explanation. It is also good to
move a hard-coded value to a named constant, or, maybe a separate checker
option.
================
Comment at: lib/StaticAnalyzer/Checkers/DirtyScalarChecker.cpp:52
+ int RecurseLimit = 3) const;
+ void checkUnbounded(CheckerContext &C, ProgramStateRef PS,
+ const Stmt *ST) const;
----------------
`ProgramStateRef State, const Stmt *S)`.
These names are usually used in analyzer and LLVM for ProgramState and Stmt,
correspondingly.
================
Comment at: lib/StaticAnalyzer/Checkers/DirtyScalarChecker.cpp:54
+ const Stmt *ST) const;
+ bool isUnbounded(CheckerContext &C, ProgramStateRef PS, SVal &V) const;
+ void reportBug(CheckerContext &C, ProgramStateRef PS, SVal &V) const;
----------------
We pass `V` by non-const reference, but it is not changed inside the function.
So, we may use a constant reference here or even pass it by value (because it
is small enough).
================
Comment at: lib/StaticAnalyzer/Checkers/DirtyScalarChecker.cpp:62
+ CheckerContext &C) const {
+ const CallExpr *CE = dyn_cast<CallExpr>(Call.getOriginExpr());
+ const FunctionDecl *FDecl = C.getCalleeDecl(CE);
----------------
`CallEvent::getOriginExpr()` may return `nullptr` (in case of implicit
destructor calls, for example) so we need to check the result before `dyn_cast`
or dereference.
================
Comment at: lib/StaticAnalyzer/Checkers/DirtyScalarChecker.cpp:82
+ ProgramStateRef PS = C.getState();
+ for (unsigned int i = 0; i < CE->getNumArgs(); ++i) {
+ const Expr *Arg = CE->getArg(i);
----------------
1. Variable names should start with capital letter to follow LLVM naming
convention.
2. We may move CE->getNumArgs out of the loop in order not to re-evaluate it
every time so the code will look like
`for (unsigned int I = 0, E = CE->getNumArgs(); I < E; ++I) {`.
We also can C++11-fy this loop:
```
for (const Expr *Arg : CE->arguments())
checkUnbounded(C, State, Arg);
```
================
Comment at: test/Analysis/dirty-scalar-unbounded.cpp:1
+// RUN: %clang_cc1 -analyze
-analyzer-checker=alpha.security.taint,alpha.security.DirtyScalar -verify
-analyzer-config alpha.security.DirtyScalar:criticalOnly=false %s
+
----------------
1. It will be good to have tests for option set to true.
2. Is there any test that makes usage of 'RecurseLimit` variable?
https://reviews.llvm.org/D27753
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
