a.sidorin added a comment.
Sorry for the delay, I have commented inline.
For me, this patch looks like a strict improvement. I think, after some clean
up this can be accepted.
================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:442
+
+ const RecordDecl *RD = RT->getDecl()->getDefinition();
+ for (const auto *I : RD->fields()) {
----------------
vlad.tsyrklevich wrote:
> 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.
Checking a default binding symbol here works because we're in PostStmt of the
call that creates this default binding. I think this machinery deserves a
comment, it was not evident for me initially.
However, I don't like to create a SymbolDerived manually. The first idea is to
just use getSVal(MemRegion*) which will return a SymbolDerived if it is. The
second is to create... SymbolMetadata that will carry a taint (if the value is
not a symbol, for example). Not sure if second idea is sane enough, I need some
comments on it. Artem, Anna?
Also, instead of referring to the base region, we may need to walk parents
one-by-one.
https://reviews.llvm.org/D28445
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
