NoQ added a comment.
Thanks again for the awesome stuff! It took years for me to even come up with
the idea.
================
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:494
+ SymbolManager &SM = C.getSymbolManager();
+ return SM.getDerivedSymbol(Sym, LCV.getRegion());
}
----------------
I'd think about this a bit more and come back.
I need to understand how come that constructing a symbol manually is the right
thing to do; that doesn't happen very often, but it seems correct here.
================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:678
+ if (contains<DerivedSymTaint>(SD->getParentSymbol()))
+ SymRegions = *get<DerivedSymTaint>(SD->getParentSymbol());
+
----------------
Just see if this pointer is null instead of a separate `contains<>` check?
================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:746
+ const TypedValueRegion *R = SD->getRegion();
+ const TaintedSymRegionsRef *SymRegions =
+ get<DerivedSymTaint>(SD->getParentSymbol());
----------------
Just see if this pointer is null instead of a separate `contains<>` check?
================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:751
+ I != E; ++I) {
+ if (R == *I || R->isSubRegionOf(*I))
+ return true;
----------------
This could be made even stronger when there are multiple ways of constructing
the same sub-region. For instance,
```
union {
int x;
char y[4];
} u;
u.x = taint();
u.y[0]; // is tainted?
```
To handle such cases, we could try to see if byte offsets are nested, instead
of checking `isSubRegionOf()`.
I suggest adding a TODO (and maybe a FIXME-test), because it gets more and more
complicated. Especially with symbolic offsets.
================
Comment at: test/Analysis/taint-generic.c:210
+ read(sock, &tainted.st, sizeof(tainted.st));
+ __builtin_memcpy(buffer, tainted.buf, tainted.st[0].length); //
expected-warning {{Untrusted data is used to specify the buffer size}}
}
----------------
Are we already supporting the case when we're tainting some elements of an
array but not all of them, and this works as expected? Could we add such tests
(regardless of whether we already handle them)?
https://reviews.llvm.org/D30909
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
