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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
