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

[Daniel Krupp via Phabricator via cfe-commits]

dkrupp added a comment.

@steakhal  thanks for the review. I fixed all outstanding remarks.
I left the test taint-diagnostic-visitor.c formatting as is to remain consistent with the rest of the file. I think we should keep it as is, or reformat the whole file.



================
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,
----------------
steakhal wrote:
> 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.
Fixed as suggested. thanks.


================
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:
> 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?
Sorry I overlooked this comment. I like this shorter version. It is so much consize!  Changed at all places. Thanks for the suggestion.


================
Comment at: clang/test/Analysis/taint-diagnostic-visitor.c:53-67
+//Tests if the originated note is correctly placed even if the path is
+//propagating through variables and expressions
+char* taintDiagnosticPropagation(){
+  char *pathbuf;
+  char *pathlist=getenv("PATH"); // expected-note {{Taint originated here}}
+                                 // expected-note at -1 {{Taint propagated to the return value}}
+  if (pathlist){ // expected-note {{Assuming 'pathlist' is non-null}}
----------------
steakhal wrote:
> steakhal wrote:
> > I know this is subjective, but I'd suggest to reformat the tests to match LLVM style guidelines, unless the formatting is important for the test.
> > Consistency helps the reader and reviewer, as code and tests are read many more times than written.
> > 
> > This applies to the rest of the touched tests.
> Originally I meant this to the rest of the test cases you change or add part of this patch. I hope it clarifies.
I made some formatting changes you suggested, but 
I would like to leave the //expected-note tags as they are now, because then it remains consistent with the rest of the test cases.

Would it be okay like this, or should I reformat the whole file (untouched parts too)?


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

https://reviews.llvm.org/D144269