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

[Vlad Tsyrklevich via Phabricator via cfe-commits]

vlad.tsyrklevich added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:442
+
+    const RecordDecl *RD = RT->getDecl()->getDefinition();
+    for (const auto *I : RD->fields()) {
----------------
a.sidorin wrote:
> 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.
Not sure which form of correctness you're interested in here but I'll bring up one issue I'm aware of: currently this will create a new SymbolDerived for an LCV sub-region, but it won't be correctly matched against in `isTainted()` because subsequent references to the same region will have a different SymbolDerived. This is the FIXME I mentioned below in `taint-generic.c` I have some idea on how to fix this but I think it will probably require more back and forth, hence why I didn't include it in this change. As it stands now, the sub-region tainting could be removed without changing the functionality of the current patch.


================
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))
----------------
a.sidorin wrote:
> We get the LazyCompoundVal for some region but return the symbol for its base. It means that at least method name is very confusing.
I believe that default bindings are only on base regions, so if you pass a reference to `outer_struct.inner_struct` the default binding for that LCV will be over `outer_struct`. I'm basing this on other references to LCVs in Core/RegionStore.cpp but I could be wrong. Either way, I'd be happy to change the interface to have the caller pass the correct MemRegion here.


https://reviews.llvm.org/D28445