Skip to content

Commit 32e0f42

Browse files
asgerfasgerf
committed
Ruby: refactor Return(x) to Method(x).return
1 parent 55b5f19 commit 32e0f42

File tree

3 files changed

+70
-43
lines changed

3 files changed

+70
-43
lines changed

‎ruby/ql/lib/codeql/ruby/ApiGraphs.qll

Lines changed: 52 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -98,9 +98,17 @@ module API {
9898
/**
9999
* Gets a node representing the result of calling a method on the receiver represented by this node.
100100
*/
101-
Node getReturn(string method) {
102-
result = this.getASubclass().getASuccessor(Label::return(method))
103-
}
101+
Node getMethod(string method) { result = this.getASubclass().getASuccessor(Label::method(method)) }
102+
103+
/**
104+
* Gets a node representing the result of this call.
105+
*/
106+
Node getReturn() { result = this.getASuccessor(Label::return()) }
107+
108+
/**
109+
* Gets a node representing the result of calling a method on the receiver represented by this node.
110+
*/
111+
Node getReturn(string method) { result = this.getMethod(method).getReturn() }
104112

105113
private predicate hasParameterIndex(int n) {
106114
exists(string str |
@@ -288,6 +296,8 @@ module API {
288296
newtype TApiNode =
289297
/** The root of the API graph. */
290298
MkRoot() or
299+
/** The method accessed at `call`, synthetically treated as a separate object. */
300+
MkMethodCall(DataFlow::CallNode call) { isUse(call) } or
291301
/** A use of an API member at the node `nd`. */
292302
MkUse(DataFlow::Node nd) { isUse(nd) } or
293303
/** A value that escapes into an API at the node `nd` */
@@ -332,14 +342,7 @@ module API {
332342
ref.asExpr() = c and
333343
read = c.getExpr()
334344
)
335-
or
336-
// Calling a method on a node that is a use of `base`
337-
exists(ExprNodes::MethodCallCfgNode call, string name |
338-
node.asExpr() = call.getReceiver() and
339-
name = call.getExpr().getMethodName() and
340-
lbl = Label::return(name) and
341-
ref.asExpr() = call
342-
)
345+
// note: method calls are not handled here as there is no DataFlow::Node for the intermediate MkMethodCall API node
343346
}
344347

345348
pragma[nomagic]
@@ -351,6 +354,8 @@ module API {
351354
useStep(_, node, nd)
352355
)
353356
or
357+
useCandFwd().flowsTo(nd.(DataFlow::CallNode).getReceiver())
358+
or
354359
parameterStep(_, defCand(), nd)
355360
}
356361

@@ -395,10 +400,8 @@ module API {
395400
}
396401

397402
private predicate isDef(DataFlow::Node rhs) {
398-
exists(DataFlow::Node use |
399-
useCandFwd().flowsTo(use) and
400-
argumentStep(_, use, rhs)
401-
)
403+
// If a call node is relevant as a use-node, treat its arguments as def-nodes
404+
argumentStep(_, useCandFwd(), rhs)
402405
}
403406

404407
/** Gets a data flow node that flows to the RHS of a def-node. */
@@ -504,6 +507,11 @@ module API {
504507
result = trackDefNode(rhs, TypeBackTracker::end())
505508
}
506509

510+
pragma[nomagic]
511+
private predicate useNodeReachesReceiver(DataFlow::Node use, DataFlow::CallNode call) {
512+
trackUseNode(use).flowsTo(call.getReceiver())
513+
}
514+
507515
/**
508516
* Holds if there is an edge from `pred` to `succ` in the API graph that is labeled with `lbl`.
509517
*/
@@ -519,6 +527,11 @@ module API {
519527
trackUseNode(src).flowsTo(node) and
520528
useStep(lbl, node, ref)
521529
)
530+
or
531+
exists(DataFlow::Node callback |
532+
pred = MkDef(callback) and
533+
parameterStep(lbl, trackDefNode(callback), ref)
534+
)
522535
)
523536
or
524537
// `pred` is a use of class A
@@ -532,15 +545,26 @@ module API {
532545
lbl = Label::subclass()
533546
)
534547
or
535-
exists(DataFlow::Node use, DataFlow::Node base, DataFlow::Node rhs |
536-
pred = MkUse(use) and
537-
trackUseNode(use).flowsTo(base) and
538-
argumentStep(lbl, base, rhs) and
539-
succ = MkDef(rhs)
548+
exists(DataFlow::CallNode call |
549+
// from receiver to method call node
550+
exists(DataFlow::Node receiver |
551+
pred = MkUse(receiver) and
552+
useNodeReachesReceiver(receiver, call) and
553+
lbl = Label::method(call.getMethodName()) and
554+
succ = MkMethodCall(call)
555+
)
540556
or
541-
pred = MkDef(rhs) and
542-
parameterStep(lbl, trackDefNode(rhs), use) and
543-
succ = MkUse(use)
557+
// from method call node to return and arguments
558+
pred = MkMethodCall(call) and
559+
(
560+
lbl = Label::return() and
561+
succ = MkUse(call)
562+
or
563+
exists(DataFlow::Node rhs |
564+
argumentStep(lbl, call, rhs) and
565+
succ = MkDef(rhs)
566+
)
567+
)
544568
)
545569
}
546570

@@ -564,10 +588,13 @@ private module Label {
564588
/** Gets the `member` edge label for the unknown member. */
565589
string unknownMember() { result = "getUnknownMember()" }
566590

567-
/** Gets the `return` edge label. */
591+
/** Gets the `method` edge label. */
568592
bindingset[m]
569593
bindingset[result]
570-
string return(string m) { result = "getReturn(\"" + m + "\")" }
594+
string method(string m) { result = "getMethod(\"" + m + "\")" }
595+
596+
/** Gets the `return` edge label. */
597+
string return() { result = "getReturn()" }
571598

572599
string subclass() { result = "getASubclass()" }
573600

‎ruby/ql/test/library-tests/dataflow/api-graphs/ApiGraphs.expected

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
classMethodCalls
2-
| test1.rb:58:1:58:8 | Use getMember("M1").getMember("C1").getReturn("m") |
3-
| test1.rb:59:1:59:8 | Use getMember("M2").getMember("C3").getReturn("m") |
2+
| test1.rb:58:1:58:8 | Use getMember("M1").getMember("C1").getMethod("m").getReturn() |
3+
| test1.rb:59:1:59:8 | Use getMember("M2").getMember("C3").getMethod("m").getReturn() |
44
instanceMethodCalls
5-
| test1.rb:61:1:61:12 | Use getMember("M1").getMember("C1").getReturn("new").getReturn("m") |
6-
| test1.rb:62:1:62:12 | Use getMember("M2").getMember("C3").getReturn("new").getReturn("m") |
5+
| test1.rb:61:1:61:12 | Use getMember("M1").getMember("C1").getMethod("new").getReturn().getMethod("m").getReturn() |
6+
| test1.rb:62:1:62:12 | Use getMember("M2").getMember("C3").getMethod("new").getReturn().getMethod("m").getReturn() |

‎ruby/ql/test/library-tests/dataflow/api-graphs/test1.rb

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
11
MyModule #$ use=getMember("MyModule")
2-
print MyModule.foo #$ use=getMember("MyModule").getReturn("foo")
3-
Kernel.print(e) #$ use=getMember("Kernel").getReturn("print")
2+
print MyModule.foo #$ use=getMember("MyModule").getMethod("foo").getReturn()
3+
Kernel.print(e) #$ use=getMember("Kernel").getMethod("print").getReturn()
44
Object::Kernel #$ use=getMember("Kernel")
5-
Object::Kernel.print(e) #$ use=getMember("Kernel").getReturn("print")
5+
Object::Kernel.print(e) #$ use=getMember("Kernel").getMethod("print").getReturn()
66
begin
7-
print MyModule.bar #$ use=getMember("MyModule").getReturn("bar")
7+
print MyModule.bar #$ use=getMember("MyModule").getMethod("bar").getReturn()
88
raise AttributeError #$ use=getMember("AttributeError")
99
rescue AttributeError => e #$ use=getMember("AttributeError")
10-
Kernel.print(e) #$ use=getMember("Kernel").getReturn("print")
10+
Kernel.print(e) #$ use=getMember("Kernel").getMethod("print").getReturn()
1111
end
12-
Unknown.new.run #$ use=getMember("Unknown").getReturn("new").getReturn("run")
12+
Unknown.new.run #$ use=getMember("Unknown").getMethod("new").getReturn().getMethod("run").getReturn()
1313
Foo::Bar::Baz #$ use=getMember("Foo").getMember("Bar").getMember("Baz")
1414

15-
Const = [1, 2, 3] #$ use=getMember("Array").getReturn("[]")
16-
Const.each do |c| #$ use=getMember("Const").getReturn("each")
17-
puts c #$ use=getMember("Const").getReturn("each").getBlock().getParameter(0)
15+
Const = [1, 2, 3] #$ use=getMember("Array").getMethod("[]").getReturn()
16+
Const.each do |c| #$ use=getMember("Const").getMethod("each").getReturn()
17+
puts c #$ use=getMember("Const").getMethod("each").getBlock().getParameter(0)
1818
end
1919

2020
foo = Foo #$ use=getMember("Foo")
@@ -28,7 +28,7 @@ module Inner
2828
end
2929
end
3030

31-
Outer::Inner.foo #$ use=getMember("Outer").getMember("Inner").getReturn("foo")
31+
Outer::Inner.foo #$ use=getMember("Outer").getMember("Inner").getMethod("foo").getReturn()
3232

3333
module M1
3434
class C1
@@ -55,8 +55,8 @@ class C4 < C2 #$ use=getMember("C2")
5555
M2::C3 #$ use=getMember("M2").getMember("C3") use=getMember("M1").getMember("C1").getASubclass()
5656
M2::C4 #$ use=getMember("M2").getMember("C4") use=getMember("C2").getASubclass() use=getMember("M1").getMember("C1").getASubclass().getASubclass()
5757

58-
M1::C1.m #$ use=getMember("M1").getMember("C1").getReturn("m")
59-
M2::C3.m #$ use=getMember("M2").getMember("C3").getReturn("m") use=getMember("M1").getMember("C1").getASubclass().getReturn("m")
58+
M1::C1.m #$ use=getMember("M1").getMember("C1").getMethod("m").getReturn()
59+
M2::C3.m #$ use=getMember("M2").getMember("C3").getMethod("m").getReturn() use=getMember("M1").getMember("C1").getASubclass().getMethod("m").getReturn()
6060

61-
M1::C1.new.m #$ use=getMember("M1").getMember("C1").getReturn("new").getReturn("m")
62-
M2::C3.new.m #$ use=getMember("M2").getMember("C3").getReturn("new").getReturn("m")
61+
M1::C1.new.m #$ use=getMember("M1").getMember("C1").getMethod("new").getReturn().getMethod("m").getReturn()
62+
M2::C3.new.m #$ use=getMember("M2").getMember("C3").getMethod("new").getReturn().getMethod("m").getReturn()

0 commit comments

Comments
 (0)