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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
