Skip to content

Ruby: Implement localMustFlowStep #14303

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.

Sign up for GitHub

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
hvitved merged 2 commits into from
Jan 27, 2025
Merged

Ruby: Implement localMustFlowStep #14303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07910b0
Ruby: Add more callback flow tests
hvitved Sep 23, 2023
de0deab
Ruby: Implement `localMustFlowStep`
hvitved Sep 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1972,7 +1972,26 @@ private predicate mustHaveCollectionType(Node n, DataFlowType t) {
not n instanceof SynthSplatParameterNode
}

predicate localMustFlowStep(Node node1, Node node2) { none() }
predicate localMustFlowStep(Node node1, Node node2) {
node1 = SsaFlow::toParameterNodeImpl(node2)
or
exists(SsaImpl::Definition def |
def.(Ssa::WriteDefinition).assigns(node1.asExpr()) and
node2.(SsaDefinitionExtNode).getDefinitionExt() = def
or
def = node1.(SsaDefinitionExtNode).getDefinitionExt() and
node2.asExpr() = SsaImpl::getARead(def)
)
or
node1.asExpr() = node2.asExpr().(CfgNodes::ExprNodes::AssignExprCfgNode).getRhs()
Copy link
Contributor

@aibaars aibaars Jan 27, 2025

Choose a reason for hiding this comment

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

This looks good to me, I was just wondering if there was overlap between this line and def.(Ssa::WriteDefinition).assigns(node1.asExpr()) above. Both seem to be related to assignments. Similar question about SsaFlow::toParameterNodeImpl and the special handling for (ImplicitBlockArgumentNode).getParameterNode(_).

Copy link
Contributor Author

@hvitved hvitved Jan 27, 2025

Choose a reason for hiding this comment

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

This looks good to me, I was just wondering if there was overlap between this line and def.(Ssa::WriteDefinition).assigns(node1.asExpr()) above. Both seem to be related to assignments.

The first line is about flow from e to SSA def for x in x = e, and the second line is about flow from e to the whole assignment expression x = e.

Similar question about SsaFlow::toParameterNodeImpl and the special handling for (ImplicitBlockArgumentNode).getParameterNode(_).

This is because implicit block parameters and arguments are not covered by our SSA analysis, otherwise you are right it would be an instance of that.

Copy link
Contributor

@aibaars aibaars Jan 27, 2025

Choose a reason for hiding this comment

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

Thanks for clarifying.

or
node1.asExpr() = node2.asExpr().(CfgNodes::ExprNodes::BlockArgumentCfgNode).getValue()
or
node2.(ImplicitBlockArgumentNode).getParameterNode(_) = node1
or
FlowSummaryImpl::Private::Steps::summaryLocalMustFlowStep(node1.(FlowSummaryNode).getSummaryNode(),
node2.(FlowSummaryNode).getSummaryNode())
}

/** Gets the type of `n` used for type pruning. */
DataFlowType getNodeType(Node n) {
Expand Down
5 changes: 0 additions & 5 deletions ruby/ql/test/library-tests/dataflow/call-sensitivity/call-sensitivity.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,9 @@ edges
| call_sensitivity.rb:194:17:194:17 | x | call_sensitivity.rb:189:19:189:19 | x | provenance | |
| call_sensitivity.rb:194:23:194:23 | x | call_sensitivity.rb:195:11:195:11 | x | provenance | |
| call_sensitivity.rb:195:11:195:11 | x | call_sensitivity.rb:199:30:199:30 | x | provenance | |
| call_sensitivity.rb:195:11:195:11 | x | call_sensitivity.rb:203:26:203:26 | x | provenance | |
| call_sensitivity.rb:199:15:199:24 | ( ... ) | call_sensitivity.rb:193:19:193:19 | x | provenance | |
| call_sensitivity.rb:199:16:199:23 | call to taint | call_sensitivity.rb:199:15:199:24 | ( ... ) | provenance | |
| call_sensitivity.rb:199:30:199:30 | x | call_sensitivity.rb:200:8:200:8 | x | provenance | |
| call_sensitivity.rb:203:26:203:26 | x | call_sensitivity.rb:204:8:204:8 | x | provenance | |
| call_sensitivity.rb:207:16:207:16 | y | call_sensitivity.rb:209:9:209:9 | y | provenance | |
| call_sensitivity.rb:209:9:209:9 | y | call_sensitivity.rb:214:9:214:9 | x | provenance | |
| call_sensitivity.rb:214:9:214:9 | x | call_sensitivity.rb:215:10:215:10 | x | provenance | |
Expand Down Expand Up @@ -169,8 +167,6 @@ nodes
| call_sensitivity.rb:199:16:199:23 | call to taint | semmle.label | call to taint |
| call_sensitivity.rb:199:30:199:30 | x | semmle.label | x |
| call_sensitivity.rb:200:8:200:8 | x | semmle.label | x |
| call_sensitivity.rb:203:26:203:26 | x | semmle.label | x |
| call_sensitivity.rb:204:8:204:8 | x | semmle.label | x |
| call_sensitivity.rb:207:16:207:16 | y | semmle.label | y |
| call_sensitivity.rb:209:9:209:9 | y | semmle.label | y |
| call_sensitivity.rb:214:9:214:9 | x | semmle.label | x |
Expand Down Expand Up @@ -204,7 +200,6 @@ testFailures
| call_sensitivity.rb:105:10:105:10 | x | call_sensitivity.rb:178:11:178:19 | call to taint | call_sensitivity.rb:105:10:105:10 | x | $@ | call_sensitivity.rb:178:11:178:19 | call to taint | call to taint |
| call_sensitivity.rb:105:10:105:10 | x | call_sensitivity.rb:187:12:187:19 | call to taint | call_sensitivity.rb:105:10:105:10 | x | $@ | call_sensitivity.rb:187:12:187:19 | call to taint | call to taint |
| call_sensitivity.rb:200:8:200:8 | x | call_sensitivity.rb:199:16:199:23 | call to taint | call_sensitivity.rb:200:8:200:8 | x | $@ | call_sensitivity.rb:199:16:199:23 | call to taint | call to taint |
| call_sensitivity.rb:204:8:204:8 | x | call_sensitivity.rb:199:16:199:23 | call to taint | call_sensitivity.rb:204:8:204:8 | x | $@ | call_sensitivity.rb:199:16:199:23 | call to taint | call to taint |
| call_sensitivity.rb:215:10:215:10 | x | call_sensitivity.rb:222:16:222:23 | call to taint | call_sensitivity.rb:215:10:215:10 | x | $@ | call_sensitivity.rb:222:16:222:23 | call to taint | call to taint |
mayBenefitFromCallContext
| call_sensitivity.rb:6:5:6:21 | call to puts |
Expand Down
2 changes: 1 addition & 1 deletion ruby/ql/test/library-tests/dataflow/call-sensitivity/call_sensitivity.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ def invoke_block2 x
end

invoke_block2 "safe" do |x|
sink x # $ SPURIOUS hasValueFlow=37
sink x
end

def call_m (x, y)
Expand Down
28 changes: 28 additions & 0 deletions ruby/ql/test/library-tests/dataflow/global/Flow.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,18 @@ edges
| blocks.rb:18:11:18:11 | x | blocks.rb:24:18:24:18 | x | provenance | |
| blocks.rb:24:3:24:11 | call to source | blocks.rb:17:10:17:10 | x | provenance | |
| blocks.rb:24:18:24:18 | x | blocks.rb:25:8:25:8 | x | provenance | |
| callbacks.rb:9:15:9:15 | x | callbacks.rb:10:12:10:12 | x | provenance | |
| callbacks.rb:10:12:10:12 | x | callbacks.rb:17:15:17:15 | x | provenance | |
| callbacks.rb:13:20:13:20 | x | callbacks.rb:14:14:14:14 | x | provenance | |
| callbacks.rb:14:14:14:14 | x | callbacks.rb:9:15:9:15 | x | provenance | |
| callbacks.rb:17:15:17:15 | x | callbacks.rb:17:25:17:25 | x | provenance | |
| callbacks.rb:17:31:17:38 | call to taint | callbacks.rb:13:20:13:20 | x | provenance | |
| callbacks.rb:20:17:20:17 | x | callbacks.rb:21:11:21:11 | x | provenance | |
| callbacks.rb:21:11:21:11 | x | callbacks.rb:28:31:28:31 | x | provenance | |
| callbacks.rb:24:23:24:23 | x | callbacks.rb:25:17:25:17 | x | provenance | |
| callbacks.rb:25:17:25:17 | x | callbacks.rb:20:17:20:17 | x | provenance | |
| callbacks.rb:28:18:28:25 | call to taint | callbacks.rb:24:23:24:23 | x | provenance | |
| callbacks.rb:28:31:28:31 | x | callbacks.rb:28:39:28:39 | x | provenance | |
| captured_variables.rb:9:24:9:24 | x | captured_variables.rb:11:5:11:6 | fn : [lambda] [captured x] | provenance | |
| captured_variables.rb:11:5:11:6 | fn : [lambda] [captured x] | captured_variables.rb:10:20:10:20 | x | provenance | |
| captured_variables.rb:13:20:13:29 | call to taint | captured_variables.rb:9:24:9:24 | x | provenance | |
Expand Down Expand Up @@ -272,6 +284,20 @@ nodes
| blocks.rb:24:3:24:11 | call to source | semmle.label | call to source |
| blocks.rb:24:18:24:18 | x | semmle.label | x |
| blocks.rb:25:8:25:8 | x | semmle.label | x |
| callbacks.rb:9:15:9:15 | x | semmle.label | x |
| callbacks.rb:10:12:10:12 | x | semmle.label | x |
| callbacks.rb:13:20:13:20 | x | semmle.label | x |
| callbacks.rb:14:14:14:14 | x | semmle.label | x |
| callbacks.rb:17:15:17:15 | x | semmle.label | x |
| callbacks.rb:17:25:17:25 | x | semmle.label | x |
| callbacks.rb:17:31:17:38 | call to taint | semmle.label | call to taint |
| callbacks.rb:20:17:20:17 | x | semmle.label | x |
| callbacks.rb:21:11:21:11 | x | semmle.label | x |
| callbacks.rb:24:23:24:23 | x | semmle.label | x |
| callbacks.rb:25:17:25:17 | x | semmle.label | x |
| callbacks.rb:28:18:28:25 | call to taint | semmle.label | call to taint |
| callbacks.rb:28:31:28:31 | x | semmle.label | x |
| callbacks.rb:28:39:28:39 | x | semmle.label | x |
| captured_variables.rb:9:24:9:24 | x | semmle.label | x |
| captured_variables.rb:10:20:10:20 | x | semmle.label | x |
| captured_variables.rb:11:5:11:6 | fn : [lambda] [captured x] | semmle.label | fn : [lambda] [captured x] |
Expand Down Expand Up @@ -585,6 +611,8 @@ testFailures
#select
| blocks.rb:8:10:8:14 | yield ... | blocks.rb:14:12:14:20 | call to source | blocks.rb:8:10:8:14 | yield ... | $@ | blocks.rb:14:12:14:20 | call to source | call to source |
| blocks.rb:25:8:25:8 | x | blocks.rb:24:3:24:11 | call to source | blocks.rb:25:8:25:8 | x | $@ | blocks.rb:24:3:24:11 | call to source | call to source |
| callbacks.rb:17:25:17:25 | x | callbacks.rb:17:31:17:38 | call to taint | callbacks.rb:17:25:17:25 | x | $@ | callbacks.rb:17:31:17:38 | call to taint | call to taint |
| callbacks.rb:28:39:28:39 | x | callbacks.rb:28:18:28:25 | call to taint | callbacks.rb:28:39:28:39 | x | $@ | callbacks.rb:28:18:28:25 | call to taint | call to taint |
| captured_variables.rb:10:20:10:20 | x | captured_variables.rb:13:20:13:29 | call to taint | captured_variables.rb:10:20:10:20 | x | $@ | captured_variables.rb:13:20:13:29 | call to taint | call to taint |
| captured_variables.rb:17:14:17:14 | x | captured_variables.rb:20:25:20:34 | call to taint | captured_variables.rb:17:14:17:14 | x | $@ | captured_variables.rb:20:25:20:34 | call to taint | call to taint |
| captured_variables.rb:24:14:24:14 | x | captured_variables.rb:27:48:27:57 | call to taint | captured_variables.rb:24:14:24:14 | x | $@ | captured_variables.rb:27:48:27:57 | call to taint | call to taint |
Expand Down
2 changes: 2 additions & 0 deletions ruby/ql/test/library-tests/dataflow/global/TypeTrackingInlineTest.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
| blocks.rb:4:10:4:10 | r | Fixed missing result: hasValueFlow=1 |
| callbacks.rb:17:41:17:58 | # $ hasValueFlow=1 | Missing result: hasValueFlow=1 |
| callbacks.rb:29:37:29:37 | x | Unexpected result: hasValueFlow=2 |
| captured_variables.rb:50:10:50:10 | x | Fixed missing result: hasValueFlow=2 |
| captured_variables.rb:68:25:68:68 | # $ hasValueFlow=3 $ MISSING: hasValueFlow=4 | Missing result: hasValueFlow=3 |
| captured_variables.rb:72:21:72:66 | # $ hasValueFlow=4 $ SPURIOUS: hasValueFlow=3 | Fixed spurious result: hasValueFlow=3 |
Expand Down
29 changes: 29 additions & 0 deletions ruby/ql/test/library-tests/dataflow/global/callbacks.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
def taint x
x
end

def sink x
puts "SINK: #{x}"
end

def apply (f, x)
f.call(x)
end

def apply_wrap (f, x)
apply(f, x)
end

apply_wrap(->(x) { sink(x) }, taint(1)) # $ hasValueFlow=1
apply_wrap(->(x) { sink(x) }, "safe")

def apply_block x
yield x
end

def apply_block_wrap (x, &block)
apply_block(x, &block)
end

apply_block_wrap(taint(2)) { |x| sink(x) } # $ hasValueFlow=2
apply_block_wrap("safe") { |x| sink(x) }
Loading