boga95 marked 4 inline comments as done.
boga95 added a comment.
Is it ready to land?
================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:212
+ llvm::StringSwitch<TaintPropagationRule>(Name)
+ .Case("atoi", TaintPropagationRule({0}, {ReturnValueIndex}))
+ .Case("atol", TaintPropagationRule({0}, {ReturnValueIndex}))
----------------
NoQ wrote:
> `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.
Those who don't know that feature will be confused. I should make changes in
the constructor to support it. I'm going to leave it.
================
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}))
----------------
NoQ wrote:
> 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.
I changed it to `{0, 1, 2, 3}`. I don't want to make any change in the behavior
in this patch. I think who made it use InvalidArgIndex because the previous
implementation wasn't expressive enough.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55734/new/
https://reviews.llvm.org/D55734
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
