steakhal added a comment.
To conclude the review, please respond to the "Not Done" inline comments, and
mark them "Done" if you think they are resolved.
Thank you for your patience.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp:108-109
+ if ((stateNotZero && stateZero)) {
+ std::vector<SymbolRef> taintedSyms = getTaintedSymbols(C.getState(), *DV);
+ if (!taintedSyms.empty()) {
+ reportTaintBug("Division by a tainted value, possibly zero", stateZero,
C,
----------------
dkrupp wrote:
> steakhal wrote:
> >
> We cannot get rid off the getTaintedSymbols() call, as we need to pass all
> tainted symbols to reportTaintBug if we want to track back multiple
> variables. taintedSyms is a parameter of reportTaintBug(...)
Yes, makes sense. mb.
One more thing: if `reportTaintBug()` takes the `taintedSyms` vector
"by-value", you should express your intent by `std::move()`-ing your collection
expressing that it's meant to be consumed instead of taking a copy.
Otherwise, you could express this intent if the `reportTaintBug()` take a
//view// type for the collection, such as `llvm::ArrayRef<SymbolRef>` - which
would neither copy nor move from the callsite's vector, being more performant
and expressive.
I get that this vector is small and bugreport construction will dominate the
runtime anyway so I'm not complaining about this specific case, I'm just noting
this for the next time. So, here I'm not expecting any actions.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:875-879
+ std::vector<SymbolRef> TaintedSyms = getTaintedSymbols(State, *V);
+ if (!TaintedSyms.empty()) {
+ TaintedSymbols.push_back(TaintedSyms[0]);
+ TaintedIndexes.push_back(ArgNum);
+ }
----------------
dkrupp wrote:
> steakhal wrote:
> > In these cases, the code would acquire all the tainted subsymbols, which
> > then we throw away and keep only the first one.
> > This is why I suggested the approach I did I'm my last review.
> I think this suggested solution would not be correct here, as ArgSym might
> not be the actual _tainted_ symbol (inside a more complex expression).
>
> So I would prefer to leave it like this for correctness.
Okay, I also checked out the code and verified this. Indeed we would have
failing tests with my recommendation.
I still think it's suboptimal. This is somewhat related to the tainted API. It
shouldn't make sense to have tainted regions in the first place, which bites
here again. Let's keep it as-is, no actions are required.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:947-948
+ if (!TaintedArgSyms.empty()) {
+ TaintedSymbols.insert(TaintedSymbols.begin(), TaintedArgSyms.begin(),
+ TaintedArgSyms.end());
+ TaintedIndexes.push_back(I);
----------------
steakhal wrote:
>
I observed you didn't take any action about this suggestion.
It leaves me wonder if this suggestion - in general - makes sense or if there
are other reasons what I cannot foresee.
I've seen you using the fully spelled-out version in total 8 times.
Shouldn't we prefer the shorter, more expressive version instead?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:920
/// Check for taint filters.
ForEachCallArg([this, &C, &State](ArgIdxTy I, const Expr *E, SVal S) {
if (FilterArgs.contains(I)) {
----------------
This unused variable generates a compiler warning.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:995
+ const NoteTag *InjectionTag = taintOriginTrackerTag(
+ C, TaintedSymbols, TaintedIndexes, Call.getCalleeStackFrame(0));
+ C.addTransition(State, InjectionTag);
----------------
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Taint.cpp:275
+ TaintedSymbols.push_back(*SI);
+ if (returnFirstOnly && !TaintedSymbols.empty())
+ return TaintedSymbols; // return early if needed
----------------
The second part of the conjunction should be tautologically true.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144269/new/
https://reviews.llvm.org/D144269
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
