[PATCH] D30909: [Analyzer] Finish taint propagation to derived symbols of tainted regions

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Thanks again for the awesome stuff! It took years for me to even come up with the idea.



================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:494
+  SymbolManager &SM = C.getSymbolManager();
+  return SM.getDerivedSymbol(Sym, LCV.getRegion());
 }
----------------
I'd think about this a bit more and come back.

I need to understand how come that constructing a symbol manually is the right thing to do; that doesn't happen very often, but it seems correct here.


================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:678
+    if (contains<DerivedSymTaint>(SD->getParentSymbol()))
+      SymRegions = *get<DerivedSymTaint>(SD->getParentSymbol());
+
----------------
Just see if this pointer is null instead of a separate `contains<>` check?


================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:746
+        const TypedValueRegion *R = SD->getRegion();
+        const TaintedSymRegionsRef *SymRegions =
+            get<DerivedSymTaint>(SD->getParentSymbol());
----------------
Just see if this pointer is null instead of a separate `contains<>` check?


================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:751
+             I != E; ++I) {
+          if (R == *I || R->isSubRegionOf(*I))
+            return true;
----------------
This could be made even stronger when there are multiple ways of constructing the same sub-region. For instance,

```
union {
  int x;
  char y[4];
} u;

u.x = taint();
u.y[0]; // is tainted?
```

To handle such cases, we could try to see if byte offsets are nested, instead of checking `isSubRegionOf()`.

I suggest adding a TODO (and maybe a FIXME-test), because it gets more and more complicated. Especially with symbolic offsets.


================
Comment at: test/Analysis/taint-generic.c:210
+  read(sock, &tainted.st, sizeof(tainted.st));
+  __builtin_memcpy(buffer, tainted.buf, tainted.st[0].length); // expected-warning {{Untrusted data is used to specify the buffer size}}
 }
----------------
Are we already supporting the case when we're tainting some elements of an array but not all of them, and this works as expected? Could we add such tests (regardless of whether we already handle them)?


https://reviews.llvm.org/D30909