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

[Vlad Tsyrklevich via Phabricator via cfe-commits]

vlad.tsyrklevich added inline comments.


================
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))
----------------
vlad.tsyrklevich wrote:
> vlad.tsyrklevich wrote:
> > 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.
> One change we could introduce to make getDefaultBinding() more explicit is to have the caller pass LCV.getRegion()->getBaseRegion() instead of just passing the LCV. However, this has the disadvantage of hardcoding an implementation detail of RegionStoreManager into users of the StoreManager interface. I think the correct way forward here might just be to add a comment that mentions that currently RegionStoreManagers bindings are only over base regions. What do you think?
I spoke too soon, after digging a little deeper I realized that lookups using `RegionBindingsRef ::getDefaultBinding` converts to a base region check anyway in `BindingKey::Make`. Therefore it'd be better to just completely hide that implementation detail to the RegionBindings than hard coding it here unnecessarily. The updated revision corrects this.


https://reviews.llvm.org/D28445