[PATCH] D55734: [analyzer] Revise GenericTaintChecker's internal representation

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Hi, thanks again for looking into this! I have a single piece of doubt that this doesn't change the behavior (the comment about `pread`), but other than that, this diff is good to go, in my opinion.



================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:212
+      llvm::StringSwitch<TaintPropagationRule>(Name)
+          .Case("atoi", TaintPropagationRule({0}, {ReturnValueIndex}))
+          .Case("atol", TaintPropagationRule({0}, {ReturnValueIndex}))
----------------
`llvm::ArrayRef` has a fancy feature: it can be constructed not only from an initializer list, but also from a single element. This could have allowed skipping `{}` in these constructors when there is just one value in the list. That's what i would have done, but i guess it's up to you to decide which approach is prettier.


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:224
+          .Case("read", TaintPropagationRule({0, 2}, {1, ReturnValueIndex}))
+          .Case("pread", TaintPropagationRule({0, 2, 3}, {1, ReturnValueIndex}))
+          .Case("gets", TaintPropagationRule({}, {0, ReturnValueIndex}))
----------------
Now `pread` doesn't produce a tainted result when only the buffer is tainted but other arguments are not. Is it a functional change? I guess we should be able to add tests for it. You can also commit this patch with `{0, 1, 2, 3}` and remove `1` in the next patch that also adds a test.


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:466
   bool IsTainted = false;
-  for (ArgVector::const_iterator I = SrcArgs.begin(),
-                                 E = SrcArgs.end(); I != E; ++I) {
+  for (ArgVector::const_iterator I = SrcArgs.begin(), E = SrcArgs.end(); I != E;
+       ++I) {
----------------
`auto` here as well?


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:469
     unsigned ArgNum = *I;
-
-    if (ArgNum == InvalidArgIndex) {
-      // Check if any of the arguments is tainted, but skip the
-      // destination arguments.
-      for (unsigned int i = 0; i < CE->getNumArgs(); ++i) {
-        if (isDestinationArgument(i))
-          continue;
-        if ((IsTainted = isTaintedOrPointsToTainted(CE->getArg(i), State, C)))
-          break;
-      }
-      break;
-    }
-
     if (CE->getNumArgs() < (ArgNum + 1))
       return State;
----------------
The more natural way to write this is `ArgNum >= CE->getNumArgs()`, as opposed to `ArgNum < CE->getNumArgs()` which is the usual boundary when dealing with arrays that start at 0.


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:488
   // Mark the arguments which should be tainted after the function returns.
-  for (ArgVector::const_iterator I = DstArgs.begin(),
-                                 E = DstArgs.end(); I != E; ++I) {
+  for (ArgVector::const_iterator I = DstArgs.begin(), E = DstArgs.end(); I != E;
+       ++I) {
----------------
`auto` here as well?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55734/new/

https://reviews.llvm.org/D55734