[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

[Balázs Benics via Phabricator via cfe-commits]

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