Skip to content

Ruby: Extend barrier guards to handle phi inputs #15985

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

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

hvitved
Copy link
Contributor

@hvitved hvitved commented Mar 20, 2024

Barrier guards are implemented using SSA; a node is guarded if it is a read of an SSA definition def, where def is also read in a controlling condition:

x = taint
if safe(x) then
    sink(x) # guarded
end

However, this approach does not work when a condition controls not a read, but instead input into a phi node:

x = taint # 1
if not safe(x) then
    x = safe # 2
end
# phi
sink(x)

In the above, the input into phi from x = taint (1) is controlled by the true edge out of safe(x), but the read of x in sink(x) is not.

In order to handle this scenario (under-the-hood) with barrier guards, we add a new type of (hidden) data flow nodes that represent input into phi nodes, and then split the data flow step x -> phi into two steps x -> [input 1] phi and [input 1] phi -> phi, where [input 1] phi represents the input into phi from the basic block starting at x = taint. The barrier guard logic will then make sure to include [input 1] phi as a barrier node (but neither phi nor [input 2] phi).

A similar scenario can happen for phi reads:

x = taint
if b then
    if not safe(x) then # 1
        return
    end
else
    if not safe(x) then # 2
        return
    end
end
#phi_read
sink(x)

In the above, phi_read (and sink(x)) is not controlled by a safe(x) condition. However, both inputs into phi_read are, so the solution is to treat phi reads exactly as normal phi nodes.

@github-actions github-actions bot added the Ruby label Mar 20, 2024
@hvitved hvitved force-pushed the ruby/phi-barrier-guards branch from 0cffcf3 to 90779f4 Compare March 20, 2024 09:02
@hvitved hvitved added the no-change-note-required This PR does not need a change note label Mar 20, 2024
@hvitved hvitved marked this pull request as ready for review March 20, 2024 12:21
@hvitved hvitved requested a review from a team as a code owner March 20, 2024 12:21
@sidshank sidshank requested a review from asgerf April 2, 2024 10:56
Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

Agree with the overall approach, this is also something we'll need in the JS data flow migration since we currently rely on barrier edges to do this.

Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

LGTM

@hvitved hvitved merged commit 2d4cf55 into github:main Apr 3, 2024
@hvitved hvitved deleted the ruby/phi-barrier-guards branch April 3, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Ruby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants