Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JS: Replace barrier edges with barrier nodes #13719

Merged
merged 14 commits into from Jul 13, 2023
Merged

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Jul 11, 2023

In preparation for moving JS to the shared data flow library, this PR removes all uses of barrier edges, replacing them with barrier nodes.

Theoretically it is not always possible to replace arbitrary barrier edges with barrier nodes, but for our use cases it seems to be possible. The shared library does not support barrier edges, and given that they can in practice become barrier nodes, there isn't a very strong use case for adding them.

The PR also adds isBarrierIn and isBarrierOut to the DataFlow::Configuration class, mimicking those from the shared library. These sometimes come in handy when replacing barrier edges.

Clear-text logging was a bit problematic as it opts out of the default PropRead taint step x -> x.prop by adding such steps as barrier edges. Neither ends of such an edge could be mapped to a barrier node, because the predecessor can have other outgoing edges, and the successor can have other ingoing edges. The solution I went with was to block the predecessor, but exclude cases where it might have other outgoing edges (and in those cases, we retain flow through the default x -> x.prop edge). It's not an elegant solution, though I'm not sure barrier edges were the best way to opt out of such taint steps in the first place. When and if we need a better solution, I'd rather work it out after transitioning to the shared data flow library.

Evaluation looks good.

  • Some duplicate results from js/insecure-randomness have been removed/fixed. The new out-barrier more accurately handles what the barrier edge tried to do, which is to prevent flow out of sinks. The old barrier edge needed to restrict succ to avoid a blow-up, but this restriction only included local flow steps.

@asgerf asgerf added the JS label Jul 11, 2023
@github-actions github-actions bot added the ATM label Jul 11, 2023
@asgerf asgerf added the no-change-note-required This PR does not need a change note label Jul 12, 2023
@asgerf asgerf marked this pull request as ready for review July 12, 2023 18:51
@asgerf asgerf requested review from a team as code owners July 12, 2023 18:51
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was an easy commit-by-commit read, nice 👍

LGTM.

@asgerf asgerf merged commit d57276c into github:main Jul 13, 2023
16 checks passed
@asgerf asgerf removed the ATM label Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants