Skip to content

Commit b88b2f1

Browse files
asgerfasgerf
authored
Merge pull request github#10836 from asgerf/rb/fix-spurious-singleton-calls
Ruby: fix spurious singleton calls
2 parents ec3dbd8 + 8cb4f23 commit b88b2f1

File tree

8 files changed

+848
-602
lines changed

8 files changed

+848
-602
lines changed

‎ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowDispatch.qll

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -368,14 +368,24 @@ private module Cached {
368368
}
369369

370370
/**
371-
* Holds if a `self` access may be the receiver of `call` inside some method, where
371+
* Holds if a `self` access may be the receiver of `call` inside some singleton method, where
372372
* that method belongs to `m` or one of `m`'s transitive super classes.
373373
*/
374374
pragma[nomagic]
375-
private predicate selfInMethodFlowsToMethodCallReceiver(RelevantCall call, Module m, string method) {
376-
exists(SsaSelfDefinitionNode self |
375+
private predicate selfInSingletonMethodFlowsToMethodCallReceiver(
376+
RelevantCall call, Module m, string method
377+
) {
378+
exists(SsaSelfDefinitionNode self, Module target, MethodBase caller |
377379
flowsToMethodCallReceiver(call, self, method) and
378-
selfInMethod(self.getVariable(), _, m.getSuperClass*())
380+
target = m.getSuperClass*() and
381+
selfInMethod(self.getVariable(), caller, target) and
382+
singletonMethod(caller, _, _) and
383+
// Singleton methods declared in a block in the top-level may spuriously end up being seen as singleton
384+
// methods on Object, if the block is actually evaluated in the context of another class.
385+
// The 'self' inside such a singleton method could then be any class, leading to self-calls
386+
// being resolved to arbitrary singleton methods.
387+
// To remedy this, we do not allow following super-classes all the way to Object.
388+
not (m != target and target = TResolved("Object"))
379389
)
380390
}
381391

@@ -454,7 +464,7 @@ private module Cached {
454464
// end
455465
// end
456466
// ```
457-
selfInMethodFlowsToMethodCallReceiver(call, m, method)
467+
selfInSingletonMethodFlowsToMethodCallReceiver(call, m, method)
458468
)
459469
)
460470
or

‎ruby/ql/test/library-tests/modules/ancestors.expected

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,15 @@ calls.rb:
110110
# 538| ProtectedMethodsSub
111111
#-----| super -> ProtectedMethods
112112

113+
# 552| SingletonUpCall_Base
114+
#-----| super -> Object
115+
116+
# 556| SingletonUpCall_Sub
117+
#-----| super -> SingletonUpCall_Base
118+
119+
# 564| SingletonUpCall_SubSub
120+
#-----| super -> SingletonUpCall_Sub
121+
113122
hello.rb:
114123
# 1| EnglishWords
115124

@@ -204,9 +213,6 @@ modules_rec.rb:
204213
# 1| B::A
205214
#-----| super -> Object
206215

207-
# 4| A::B
208-
#-----| super -> Object
209-
210216
private.rb:
211217
# 1| E
212218
#-----| super -> Object
@@ -218,3 +224,9 @@ private.rb:
218224

219225
# 96| PrivateOverride2
220226
#-----| super -> PrivateOverride1
227+
228+
toplevel_self_singleton.rb:
229+
# 2| A::B
230+
#-----| super -> Object
231+
232+
# 24| Good

‎ruby/ql/test/library-tests/modules/callgraph.expected

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ getTarget
88
| calls.rb:17:1:17:8 | call to bar | calls.rb:13:1:15:3 | bar |
99
| calls.rb:19:1:19:8 | call to foo | calls.rb:1:1:3:3 | foo |
1010
| calls.rb:19:1:19:8 | call to foo | calls.rb:85:1:89:3 | foo |
11-
| calls.rb:23:9:23:19 | call to singleton_m | calls.rb:25:5:27:7 | singleton_m |
1211
| calls.rb:32:5:32:15 | call to singleton_m | calls.rb:25:5:27:7 | singleton_m |
1312
| calls.rb:33:5:33:20 | call to singleton_m | calls.rb:25:5:27:7 | singleton_m |
1413
| calls.rb:37:1:37:13 | call to singleton_m | calls.rb:25:5:27:7 | singleton_m |
@@ -77,7 +76,6 @@ getTarget
7776
| calls.rb:224:9:224:24 | call to singleton_g | calls.rb:236:1:238:3 | singleton_g |
7877
| calls.rb:224:9:224:24 | call to singleton_g | calls.rb:243:1:245:3 | singleton_g |
7978
| calls.rb:224:9:224:24 | call to singleton_g | calls.rb:251:5:253:7 | singleton_g |
80-
| calls.rb:224:9:224:24 | call to singleton_g | calls.rb:267:1:269:3 | singleton_g |
8179
| calls.rb:228:1:228:22 | call to singleton_a | calls.rb:191:5:194:7 | singleton_a |
8280
| calls.rb:229:1:229:22 | call to singleton_f | calls.rb:218:9:220:11 | singleton_f |
8381
| calls.rb:231:6:231:19 | call to new | calls.rb:117:5:117:16 | new |
@@ -215,6 +213,10 @@ getTarget
215213
| calls.rb:549:2:549:6 | call to new | calls.rb:117:5:117:16 | new |
216214
| calls.rb:549:20:549:24 | call to baz | calls.rb:51:5:57:7 | baz |
217215
| calls.rb:550:26:550:37 | call to capitalize | calls.rb:97:5:97:23 | capitalize |
216+
| calls.rb:557:5:557:13 | call to singleton | calls.rb:553:5:554:7 | singleton |
217+
| calls.rb:560:9:560:17 | call to singleton | calls.rb:553:5:554:7 | singleton |
218+
| calls.rb:561:9:561:18 | call to singleton2 | calls.rb:565:5:566:7 | singleton2 |
219+
| calls.rb:568:5:568:14 | call to mid_method | calls.rb:559:5:562:7 | mid_method |
218220
| hello.rb:12:5:12:24 | call to include | calls.rb:108:5:110:7 | include |
219221
| hello.rb:14:16:14:20 | call to hello | hello.rb:2:5:4:7 | hello |
220222
| hello.rb:20:16:20:20 | call to super | hello.rb:13:5:15:7 | message |
@@ -263,7 +265,10 @@ getTarget
263265
| private.rb:104:1:104:20 | call to new | calls.rb:117:5:117:16 | new |
264266
| private.rb:104:1:104:28 | call to call_m1 | private.rb:91:3:93:5 | call_m1 |
265267
| private.rb:105:1:105:20 | call to new | calls.rb:117:5:117:16 | new |
268+
| toplevel_self_singleton.rb:30:13:30:19 | call to call_me | toplevel_self_singleton.rb:26:9:27:11 | call_me |
269+
| toplevel_self_singleton.rb:31:13:31:20 | call to call_you | toplevel_self_singleton.rb:29:9:32:11 | call_you |
266270
unresolvedCall
271+
| calls.rb:23:9:23:19 | call to singleton_m |
267272
| calls.rb:26:9:26:18 | call to instance_m |
268273
| calls.rb:29:5:29:14 | call to instance_m |
269274
| calls.rb:30:5:30:19 | call to instance_m |
@@ -334,6 +339,7 @@ unresolvedCall
334339
| calls.rb:549:1:549:26 | call to each |
335340
| calls.rb:550:1:550:13 | call to [] |
336341
| calls.rb:550:1:550:39 | call to each |
342+
| calls.rb:558:5:558:14 | call to singleton2 |
337343
| hello.rb:20:16:20:26 | ... + ... |
338344
| hello.rb:20:16:20:34 | ... + ... |
339345
| hello.rb:20:16:20:40 | ... + ... |
@@ -347,6 +353,11 @@ unresolvedCall
347353
| private.rb:57:1:57:14 | call to private4 |
348354
| private.rb:100:7:100:29 | call to m1 |
349355
| private.rb:105:1:105:23 | call to m1 |
356+
| toplevel_self_singleton.rb:8:1:16:3 | call to do_something |
357+
| toplevel_self_singleton.rb:10:9:10:27 | call to ab_singleton_method |
358+
| toplevel_self_singleton.rb:14:9:14:27 | call to ab_singleton_method |
359+
| toplevel_self_singleton.rb:18:12:22:1 | call to new |
360+
| toplevel_self_singleton.rb:20:9:20:27 | call to ab_singleton_method |
350361
privateMethod
351362
| calls.rb:1:1:3:3 | foo |
352363
| calls.rb:39:1:41:3 | call_instance_m |
@@ -377,6 +388,7 @@ privateMethod
377388
| private.rb:83:11:85:5 | m1 |
378389
| private.rb:87:11:89:5 | m2 |
379390
| private.rb:97:11:101:5 | m1 |
391+
| toplevel_self_singleton.rb:9:5:11:7 | method_in_block |
380392
publicMethod
381393
| calls.rb:7:1:9:3 | bar |
382394
| calls.rb:13:1:15:3 | bar |
@@ -435,6 +447,9 @@ publicMethod
435447
| calls.rb:485:5:487:7 | singleton |
436448
| calls.rb:526:5:531:7 | baz |
437449
| calls.rb:539:5:542:7 | baz |
450+
| calls.rb:553:5:554:7 | singleton |
451+
| calls.rb:559:5:562:7 | mid_method |
452+
| calls.rb:565:5:566:7 | singleton2 |
438453
| hello.rb:2:5:4:7 | hello |
439454
| hello.rb:5:5:7:7 | world |
440455
| hello.rb:13:5:15:7 | message |
@@ -456,6 +471,11 @@ publicMethod
456471
| private.rb:38:3:39:5 | public3 |
457472
| private.rb:66:3:67:5 | public |
458473
| private.rb:91:3:93:5 | call_m1 |
474+
| toplevel_self_singleton.rb:3:9:4:11 | ab_singleton_method |
475+
| toplevel_self_singleton.rb:13:5:15:7 | method_in_block |
476+
| toplevel_self_singleton.rb:19:5:21:7 | method_in_struct |
477+
| toplevel_self_singleton.rb:26:9:27:11 | call_me |
478+
| toplevel_self_singleton.rb:29:9:32:11 | call_you |
459479
protectedMethod
460480
| calls.rb:514:15:516:7 | foo |
461481
| calls.rb:522:15:524:7 | bar |

‎ruby/ql/test/library-tests/modules/calls.rb

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -548,3 +548,22 @@ def baz
548548

549549
[C.new].each { |c| c.baz }
550550
["a","b","c"].each { |s| s.capitalize }
551+
552+
class SingletonUpCall_Base
553+
def self.singleton
554+
end
555+
end
556+
class SingletonUpCall_Sub < SingletonUpCall_Base
557+
singleton
558+
singleton2 # should not resolve
559+
def self.mid_method
560+
singleton
561+
singleton2 # should resolve
562+
end
563+
end
564+
class SingletonUpCall_SubSub < SingletonUpCall_Sub
565+
def self.singleton2
566+
end
567+
568+
mid_method
569+
end

‎ruby/ql/test/library-tests/modules/methods.expected

Lines changed: 59 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -390,6 +390,48 @@ lookupMethod
390390
| calls.rb:538:1:543:3 | ProtectedMethodsSub | private_on_main | calls.rb:185:1:186:3 | private_on_main |
391391
| calls.rb:538:1:543:3 | ProtectedMethodsSub | puts | calls.rb:102:5:102:30 | puts |
392392
| calls.rb:538:1:543:3 | ProtectedMethodsSub | to_s | calls.rb:172:5:173:7 | to_s |
393+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | add_singleton | calls.rb:367:1:371:3 | add_singleton |
394+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | call_block | calls.rb:81:1:83:3 | call_block |
395+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | call_instance_m | calls.rb:39:1:41:3 | call_instance_m |
396+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | create | calls.rb:278:1:286:3 | create |
397+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | foo | calls.rb:1:1:3:3 | foo |
398+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | foo | calls.rb:85:1:89:3 | foo |
399+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | funny | calls.rb:140:1:142:3 | funny |
400+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | indirect | calls.rb:158:1:160:3 | indirect |
401+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | new | calls.rb:117:5:117:16 | new |
402+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | optional_arg | calls.rb:76:1:79:3 | optional_arg |
403+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | pattern_dispatch | calls.rb:343:1:359:3 | pattern_dispatch |
404+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | private_on_main | calls.rb:185:1:186:3 | private_on_main |
405+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | puts | calls.rb:102:5:102:30 | puts |
406+
| calls.rb:552:1:555:3 | SingletonUpCall_Base | to_s | calls.rb:172:5:173:7 | to_s |
407+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | add_singleton | calls.rb:367:1:371:3 | add_singleton |
408+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | call_block | calls.rb:81:1:83:3 | call_block |
409+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | call_instance_m | calls.rb:39:1:41:3 | call_instance_m |
410+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | create | calls.rb:278:1:286:3 | create |
411+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | foo | calls.rb:1:1:3:3 | foo |
412+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | foo | calls.rb:85:1:89:3 | foo |
413+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | funny | calls.rb:140:1:142:3 | funny |
414+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | indirect | calls.rb:158:1:160:3 | indirect |
415+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | new | calls.rb:117:5:117:16 | new |
416+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | optional_arg | calls.rb:76:1:79:3 | optional_arg |
417+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | pattern_dispatch | calls.rb:343:1:359:3 | pattern_dispatch |
418+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | private_on_main | calls.rb:185:1:186:3 | private_on_main |
419+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | puts | calls.rb:102:5:102:30 | puts |
420+
| calls.rb:556:1:563:3 | SingletonUpCall_Sub | to_s | calls.rb:172:5:173:7 | to_s |
421+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | add_singleton | calls.rb:367:1:371:3 | add_singleton |
422+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | call_block | calls.rb:81:1:83:3 | call_block |
423+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | call_instance_m | calls.rb:39:1:41:3 | call_instance_m |
424+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | create | calls.rb:278:1:286:3 | create |
425+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | foo | calls.rb:1:1:3:3 | foo |
426+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | foo | calls.rb:85:1:89:3 | foo |
427+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | funny | calls.rb:140:1:142:3 | funny |
428+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | indirect | calls.rb:158:1:160:3 | indirect |
429+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | new | calls.rb:117:5:117:16 | new |
430+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | optional_arg | calls.rb:76:1:79:3 | optional_arg |
431+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | pattern_dispatch | calls.rb:343:1:359:3 | pattern_dispatch |
432+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | private_on_main | calls.rb:185:1:186:3 | private_on_main |
433+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | puts | calls.rb:102:5:102:30 | puts |
434+
| calls.rb:564:1:569:3 | SingletonUpCall_SubSub | to_s | calls.rb:172:5:173:7 | to_s |
393435
| file://:0:0:0:0 | Class | include | calls.rb:108:5:110:7 | include |
394436
| file://:0:0:0:0 | Class | module_eval | calls.rb:107:5:107:24 | module_eval |
395437
| file://:0:0:0:0 | Class | new | calls.rb:117:5:117:16 | new |
@@ -476,9 +518,6 @@ lookupMethod
476518
| modules_rec.rb:1:1:2:3 | B::A | new | calls.rb:117:5:117:16 | new |
477519
| modules_rec.rb:1:1:2:3 | B::A | puts | calls.rb:102:5:102:30 | puts |
478520
| modules_rec.rb:1:1:2:3 | B::A | to_s | calls.rb:172:5:173:7 | to_s |
479-
| modules_rec.rb:4:1:5:3 | A::B | new | calls.rb:117:5:117:16 | new |
480-
| modules_rec.rb:4:1:5:3 | A::B | puts | calls.rb:102:5:102:30 | puts |
481-
| modules_rec.rb:4:1:5:3 | A::B | to_s | calls.rb:172:5:173:7 | to_s |
482521
| private.rb:1:1:49:3 | E | new | calls.rb:117:5:117:16 | new |
483522
| private.rb:1:1:49:3 | E | private1 | private.rb:2:11:3:5 | private1 |
484523
| private.rb:1:1:49:3 | E | private2 | private.rb:8:3:9:5 | private2 |
@@ -511,6 +550,9 @@ lookupMethod
511550
| private.rb:96:1:102:3 | PrivateOverride2 | private_on_main | private.rb:51:1:52:3 | private_on_main |
512551
| private.rb:96:1:102:3 | PrivateOverride2 | puts | calls.rb:102:5:102:30 | puts |
513552
| private.rb:96:1:102:3 | PrivateOverride2 | to_s | calls.rb:172:5:173:7 | to_s |
553+
| toplevel_self_singleton.rb:2:5:5:7 | A::B | new | calls.rb:117:5:117:16 | new |
554+
| toplevel_self_singleton.rb:2:5:5:7 | A::B | puts | calls.rb:102:5:102:30 | puts |
555+
| toplevel_self_singleton.rb:2:5:5:7 | A::B | to_s | calls.rb:172:5:173:7 | to_s |
514556
enclosingMethod
515557
| calls.rb:2:5:2:14 | call to puts | calls.rb:1:1:3:3 | foo |
516558
| calls.rb:2:5:2:14 | self | calls.rb:1:1:3:3 | foo |
@@ -842,6 +884,10 @@ enclosingMethod
842884
| calls.rb:541:9:541:27 | ProtectedMethodsSub | calls.rb:539:5:542:7 | baz |
843885
| calls.rb:541:9:541:31 | call to new | calls.rb:539:5:542:7 | baz |
844886
| calls.rb:541:9:541:35 | call to foo | calls.rb:539:5:542:7 | baz |
887+
| calls.rb:560:9:560:17 | call to singleton | calls.rb:559:5:562:7 | mid_method |
888+
| calls.rb:560:9:560:17 | self | calls.rb:559:5:562:7 | mid_method |
889+
| calls.rb:561:9:561:18 | call to singleton2 | calls.rb:559:5:562:7 | mid_method |
890+
| calls.rb:561:9:561:18 | self | calls.rb:559:5:562:7 | mid_method |
845891
| hello.rb:3:9:3:22 | return | hello.rb:2:5:4:7 | hello |
846892
| hello.rb:3:16:3:22 | "hello" | hello.rb:2:5:4:7 | hello |
847893
| hello.rb:3:17:3:21 | hello | hello.rb:2:5:4:7 | hello |
@@ -897,3 +943,13 @@ enclosingMethod
897943
| private.rb:100:7:100:22 | PrivateOverride1 | private.rb:97:11:101:5 | m1 |
898944
| private.rb:100:7:100:26 | call to new | private.rb:97:11:101:5 | m1 |
899945
| private.rb:100:7:100:29 | call to m1 | private.rb:97:11:101:5 | m1 |
946+
| toplevel_self_singleton.rb:10:9:10:27 | call to ab_singleton_method | toplevel_self_singleton.rb:9:5:11:7 | method_in_block |
947+
| toplevel_self_singleton.rb:10:9:10:27 | self | toplevel_self_singleton.rb:9:5:11:7 | method_in_block |
948+
| toplevel_self_singleton.rb:14:9:14:27 | call to ab_singleton_method | toplevel_self_singleton.rb:13:5:15:7 | method_in_block |
949+
| toplevel_self_singleton.rb:14:9:14:27 | self | toplevel_self_singleton.rb:13:5:15:7 | method_in_block |
950+
| toplevel_self_singleton.rb:20:9:20:27 | call to ab_singleton_method | toplevel_self_singleton.rb:19:5:21:7 | method_in_struct |
951+
| toplevel_self_singleton.rb:20:9:20:27 | self | toplevel_self_singleton.rb:19:5:21:7 | method_in_struct |
952+
| toplevel_self_singleton.rb:30:13:30:19 | call to call_me | toplevel_self_singleton.rb:29:9:32:11 | call_you |
953+
| toplevel_self_singleton.rb:30:13:30:19 | self | toplevel_self_singleton.rb:29:9:32:11 | call_you |
954+
| toplevel_self_singleton.rb:31:13:31:20 | call to call_you | toplevel_self_singleton.rb:29:9:32:11 | call_you |
955+
| toplevel_self_singleton.rb:31:13:31:20 | self | toplevel_self_singleton.rb:29:9:32:11 | call_you |

0 commit comments

Comments
 (0)