C++: Post-update flow through &, *, +, ... #3389
Conversation
Flow from a definition by reference of a field into its object was working inconsistently and in a very syntax-dependent way. For a function `f` receiving a reference, `f(a->x)` could propagate data back to `a` via the _reverse read_ mechanism in the shared data-flow library, but for a function `g` receiving a pointer, `g(&a->x)` would not work. And `f((*a).x)` would not work either. In all cases, the issue was that the shared data-flow library propagates data backwards between `PostUpdateNode`s only, but there is no `PostUpdateNode` for `a->x` in `g(&a->x)`. This pull request inserts such post-update nodes where appropriate and links them to their neighbors. In this exapmle, flow back from the output parameter of `g` passes first to the `PostUpdateNode` of `&`, then to the (new) `PostUpdateNode` of `a->x`, and finally, as a _reverse read_ with the appropriate field projection, to `a`.
| * - AddressConstantExpression.qll | ||
| * - AddressFlow.qll | ||
| * - EscapesTree.qll | ||
| */ |
There was a problem hiding this comment.
I think it's possible for these three files to share code, but I don't think it would make this PR any easier to review.
| private predicate lvalueToPointerStep(Expr lvalueIn, Expr pointerOut) { | ||
| lvalueIn.getConversion() = pointerOut.(ArrayToPointerConversion) | ||
| or | ||
| lvalueIn = pointerOut.(AddressOfExpr).getOperand().getFullyConverted() |
There was a problem hiding this comment.
Would it make sense to add std::addressof to this list?
There was a problem hiding this comment.
Done. It turned out to be a bit more involved than adding it to this list.
Apparently it's because both there's a huge slowdown both before and after this PR! A performance regression might have crept into |
|
I've addressed the performance problem in #3400. |
I didn't add this support in `AddressConstantExpression.qll` since I think it would require extra work and testing to get the constexprness right. My long-term plan for `AddressConstantExpression.qll` is to move its functionality to the extractor.
…-defbyref-to-field This is a partial merge from master. In particular, it takes in github#3382 and github#3385.
Adding a new test case leads to changes in all `.expected` files in its directory. The new results show that the `DefinitionsAndUses` library does not model `std::addressof` correctly, but that library is not intended to be used for new code.
|
The CPP-Differences job has finished. The performance changes seem to be within the usual noise level. |
From the main commit:
Cc @lcartey: this addresses https://github.slack.com/archives/CP0LHP150/p1587393520434500.
Cc @aschackmull: the first commit implements what you suggested in #3162 (comment). Also, the issue addressed by this PR is also present in Java data flow. If you change
f(a.x)tof((X)a.x)you no longer get reverse-read flow intoabecause the argument (a cast) is not equal to the field read.