[PATCH] D28445: [Analyzer] Extend taint propagation and checking

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

Hi Artem,
Thank you for adding me, I missed this patch. I have few comments below. If you (and Vlad) can wait for two or  three days, I will re-check the place I'm worrying about and post the results.



================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:442
+
+    const RecordDecl *RD = RT->getDecl()->getDefinition();
+    for (const auto *I : RD->fields()) {
----------------
NoQ wrote:
> We need to be careful in the case when we don't have the definition in the current translation unit. In this case we may still have derived symbols by casting the pointer into some blindly guessed type, which may be primitive or having well-defined primitive fields.
> 
> By the way, in D26837 i'm suspecting that there are other errors of this kind in the checker, eg. when a function returns a void pointer, we put taint on symbols of type "void", which is weird.
> 
> Adding Alexey who may recall something on this topic.
I will check the correctness of this code sample because I have some doubts about it.
The problem of casts is hard because of our approach to put taints on 0th elements. We lose casts and may get some strange void symbols. However, I guess this patch isn't related to this problem directly.


================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:502
+    RegionBindingsRef B = getRegionBindings(S);
+    const MemRegion *MR  = L.getRegion()->getBaseRegion();
+    if (Optional<SVal> V = B.getDefaultBinding(MR))
----------------
We get the LazyCompoundVal for some region but return the symbol for its base. It means that at least method name is very confusing.


https://reviews.llvm.org/D28445