[PATCH] D27753: [analyzer] alpha.security.DirtyScalar Checker

[Aleksei Sidorin via Phabricator via cfe-commits]

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