C++: Rudimentary support for IR data flow virtual dispatch#2262
C++: Rudimentary support for IR data flow virtual dispatch#2262rdmarsh2 merged 3 commits intogithub:masterfrom
Conversation
This is just good enough to cause no performance regressions and pass the virtual-dispatch tests we have for `security.TaintTracking`. In particular, it fixes the tests for `UncontrolledProcessOperation.ql` when enabling `DefaultTaintTracking.qll`.
This makes IR data flow behave more like AST data flow, and it makes IR virtual dispatch work without further changes.
This bit is only used by the compatibility code that sends flow into parameters of functions without body.
| * as a chain of `ConvertToBaseInstruction`s and will cause the search to start | ||
| * from each of them and pass through subsequent ones. There might be | ||
| * performance to gain by stopping before a second upcast and reconstructing | ||
| * the full chain in a "big-step" recursion after this one. |
There was a problem hiding this comment.
Isn't there also a precision improvement to be made there, if there's a cast through more than one class that overrides the same virtual function?
| * the full chain in a "big-step" recursion after this one. | ||
| */ | ||
| private predicate nodeMayHaveClass(Class derived, DataFlow::Node node) { | ||
| exists(ConvertToBaseInstruction toBase | |
There was a problem hiding this comment.
Should probably also consider ConvertToVirtualBaseInstruction. Perhaps we need a common base class for that and ConvertToBaseInstruction.
| * performance to gain by stopping before a second upcast and reconstructing | ||
| * the full chain in a "big-step" recursion after this one. | ||
| */ | ||
| private predicate nodeMayHaveClass(Class derived, DataFlow::Node node) { |
There was a problem hiding this comment.
Given a function declared void foo(Base* p) { p->vfunc(); }, will this consider that p->vfunc() could be any override of Base::vfunc()? Or do we only think that it could call Derived::vfunc() if we see a derived-to-base cast from Derived*?
There was a problem hiding this comment.
The latter. To make this interprocedural will be follow-up work. I don't think we should make it overapproximating to begin with since the goal for now is just to replace security.TaintTracking.
3 participants
This virtual dispatch support is not up to par with the one in
security.TaintTracking, but it's enough to handle the two examples of virtual dispatch we have in our qltests:UncontrolledProcessOperation/test.cppin this repo andUncontrolledProcessOperation/test.cppin the internal repo.