Java/C++/C# DataFlow: Add support for in/out barriers on sources and sinks.#1639
Merged
yh-semmle merged 7 commits intogithub:masterfrom Aug 7, 2019
Merged
Conversation
Contributor
Author
|
Hmm.. this does not seem like the right approach, as it becomes too easy for something that was intended to be a full barrier to become an out barrier. E.g. if a guarded variable access occurs in a sink position. |
Contributor
Author
|
Changed to keep semantics of |
jbj
reviewed
Aug 5, 2019
Contributor
jbj
left a comment
There was a problem hiding this comment.
I can't vouch for the correctness of all the new places where inBarrier or outBarrier are used, but the API and implementation structure LGTM.
| @@ -57,8 +57,14 @@ abstract class Configuration extends string { | |||
| /** Holds if data flow through `node` is prohibited. */ | |||
Contributor
There was a problem hiding this comment.
The QLDoc on isBarrier uses terminology that isn't easy to relate to the two new predicates. I suggest changing "through" to "into or out of".
Same thing with isSanitizer.
hvitved
approved these changes
Aug 6, 2019
yh-semmle
approved these changes
Aug 7, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This replaces the
isBarrierEdgefunctionality with in/out barriers on sources and sinks.Previously, any source or sink that was also a barrier would be disregarded, but now that is instead interpreted as an in-barrier on a source and an out-barrier on a sink.This allows e.g. for configurations that stop flow on a sink even if that particular data-flow node can reach a subsequent sink and vice-versa for sources. The concept of barrier edges was only ever supported for local edges, required intimate knowledge of the actual step relation used internally by the library, and its main use case was exactly in/out barriers on sources and sinks, which should be handled a lot better now.This feature also makes it easy to extend the configuration interface with general in/out barriers on any node if that's something we want.<-- That doesn't make sense anyway, a non-source in-barrier is equivalent to a full barrier.Update:
Having in/out barriers be implicitly specified turned out to be problematic when e.g. specifying a variable access as a barrier because it was guarded and that variable access then occurred in a sink position, as that created a non-intuitive dependence between
isSinkandisBarrier. So the better solution is simply to keep the semantics ofisBarrieras-is and add two new predicates to configurations to specify in/out barriers.