Skip to content

Commit ba4d0a8

Browse files
hmachmac
committed
Ruby: Simplify filter dataflow
This introduces some false flow (the `ThreeController` and `FourController` examples in `filter_flow.rb`) but is simpler and in line with how we model flow for normal method calls.
1 parent 0a02b45 commit ba4d0a8

File tree

4 files changed

+120
-116
lines changed

4 files changed

+120
-116
lines changed

‎ruby/ql/lib/codeql/ruby/frameworks/actioncontroller/Filters.qll

Lines changed: 19 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -394,46 +394,18 @@ module Filters {
394394
*/
395395
predicate next(Method pred, Method succ) { next(_, pred, succ) }
396396

397-
/**
398-
* Holds if `n` is the post-update node of a self-variable access in method
399-
* `m` which stores content in the self variable.
400-
* In the example below, `n` is the post-update node for `@x = 1`.
401-
* ```rb
402-
* def m
403-
* @x = 1
404-
* end
405-
* ```
406-
*/
407-
private predicate variableAccessPostUpdate(DataFlow::PostUpdateNode n, Method m) {
408-
n.getPreUpdateNode().asExpr() instanceof SelfVariableAccessCfgNode and
409-
m = n.getPreUpdateNode().asExpr().getExpr().getEnclosingCallable() and
410-
DataFlowPrivate::storeStep(_, _, n)
411-
}
412-
413-
/**
414-
* Holds if `n` is the syntactically last dataflow node in `m` that satisfies `variableAccessPostUpdate`.
415-
* In the example below, `n` is the post-update node for `@y = 2`.
416-
* ```rb
417-
* def m
418-
* @x = 1
419-
* @y = 2
420-
* end
421-
* ```
422-
*/
423-
private predicate lastVariableAccessPostUpdate(DataFlow::PostUpdateNode n, Method m) {
424-
variableAccessPostUpdate(n, m) and
425-
not exists(DataFlow::PostUpdateNode n2 |
426-
variableAccessPostUpdate(n2, m) and
427-
n.getPreUpdateNode().asExpr().getASuccessor+() = n2.getPreUpdateNode().asExpr()
428-
)
429-
}
430-
431397
/**
432398
* Holds if `n` is a post-update node for `self` in method `m`.
433399
*/
434400
predicate selfPostUpdate(DataFlow::PostUpdateNode n, Method m) {
435401
n.getPreUpdateNode().asExpr() instanceof SelfVariableAccessCfgNode and
436-
m = n.getPreUpdateNode().asExpr().getExpr().getEnclosingCallable()
402+
m = n.getPreUpdateNode().asExpr().getExpr().getEnclosingCallable() and
403+
n.getPreUpdateNode()
404+
.asExpr()
405+
.(SelfVariableAccessCfgNode)
406+
.getExpr()
407+
.getVariable()
408+
.getDeclaringScope() = m
437409
}
438410

439411
/**
@@ -454,25 +426,6 @@ module Filters {
454426
)
455427
}
456428

457-
/**
458-
* Holds if `a` is the syntactically last access of the self variable `v` in method `m`.
459-
*/
460-
private predicate lastMethodSelfVarAccess(SelfVariableAccessCfgNode a, SelfVariable v, Method m) {
461-
a.getExpr().getEnclosingMethod() = m and
462-
v = a.getExpr().getVariable() and
463-
not exists(SelfVariableAccessCfgNode a2 |
464-
a2.getExpr().getVariable() = v and
465-
a.getASuccessor+() = a2
466-
)
467-
}
468-
469-
/**
470-
* Holds if there is no access to `self` in method `m`.
471-
*/
472-
private predicate noSelfVarAccess(Method m) {
473-
not exists(SelfVariableAccess a | a.getEnclosingMethod() = m)
474-
}
475-
476429
/**
477430
* Holds if `n` is the self parameter of method `m`.
478431
*/
@@ -494,53 +447,28 @@ module Filters {
494447
exists(Method predMethod, Method succMethod |
495448
next(predMethod, succMethod) and
496449
(
497-
// Flow from an update of self (due to an instance variable write) in
498-
// `pred` to the self parameter of `succ`
450+
// Flow from a post-update node of self in `pred` to the self parameter of `succ`
499451
//
500452
// def a
501-
// @x = 1 ---+
502-
// end |
503-
// |
504-
// def b <----+
453+
// foo() ---------+
454+
// @x = 1 ---+ |
455+
// end | |
456+
// | |
457+
// def b <----+----+
505458
// ...
506459
//
507-
lastVariableAccessPostUpdate(pred, predMethod) and
460+
selfPostUpdate(pred, predMethod) and
508461
selfParameter(succ, succMethod)
509462
or
510-
// Flow from an update of self in `pred` to the self parameter of `succ`
511-
//
512-
// def a
513-
// @x = 1 ---+
514-
// end |
515-
// |
516-
// def b <----+
517-
// ...
518-
//
519-
lastSelfPostUpdate(pred, predMethod) and
520-
selfParameter(succ, succMethod)
521-
or
522-
// When there is no update of self in `pred`, flow from the last access to self to the self parameter of `succ`
523-
//
524-
// def a
525-
// foo() ---+
526-
// x = 1 |
527-
// end |
528-
// |
529-
// def b <---+
530-
// ...
531-
//
532-
not variableAccessPostUpdate(_, predMethod) and
533-
lastMethodSelfVarAccess(pred.asExpr(), _, predMethod) and
534-
selfParameter(succ, succMethod)
535-
or
536-
// When there is no access to self in `pred`, flow from the self parameter of `pred` to the self parameter of `succ`
463+
// Flow from the self parameter of `pred` to the self parameter of `succ`
537464
//
538465
// def a ---+
466+
// ... |
539467
// end |
540468
// |
541-
// def b <--+
542-
// ...
543-
noSelfVarAccess(predMethod) and
469+
// def b <-+
470+
// ...
471+
//
544472
selfParameter(pred, predMethod) and
545473
selfParameter(succ, succMethod)
546474
)

‎ruby/ql/test/library-tests/frameworks/action_controller/Filters.expected

Lines changed: 63 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
11
additionalFlowSteps
2+
| controllers/application_controller.rb:6:3:8:5 | self in set_user | controllers/comments_controller.rb:74:3:77:5 | self in ensure_user_can_edit_comments |
3+
| controllers/application_controller.rb:6:3:8:5 | self in set_user | controllers/comments_controller.rb:79:3:81:5 | self in set_comment |
4+
| controllers/application_controller.rb:6:3:8:5 | self in set_user | controllers/comments_controller.rb:99:3:100:5 | self in foo |
5+
| controllers/application_controller.rb:6:3:8:5 | self in set_user | controllers/posts_controller.rb:12:3:15:5 | self in index |
6+
| controllers/application_controller.rb:6:3:8:5 | self in set_user | controllers/posts_controller.rb:17:3:18:5 | self in show |
7+
| controllers/application_controller.rb:6:3:8:5 | self in set_user | controllers/posts_controller.rb:20:3:21:5 | self in upvote |
28
| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/comments_controller.rb:74:3:77:5 | self in ensure_user_can_edit_comments |
39
| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/comments_controller.rb:79:3:81:5 | self in set_comment |
410
| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
@@ -11,57 +17,99 @@ additionalFlowSteps
1117
| controllers/application_controller.rb:7:23:7:29 | [post] self | controllers/posts_controller.rb:12:3:15:5 | self in index |
1218
| controllers/application_controller.rb:7:23:7:29 | [post] self | controllers/posts_controller.rb:17:3:18:5 | self in show |
1319
| controllers/application_controller.rb:7:23:7:29 | [post] self | controllers/posts_controller.rb:20:3:21:5 | self in upvote |
20+
| controllers/application_controller.rb:10:3:12:5 | self in log_request | controllers/application_controller.rb:6:3:8:5 | self in set_user |
21+
| controllers/application_controller.rb:10:3:12:5 | self in log_request | controllers/photos_controller.rb:3:3:6:5 | self in show |
22+
| controllers/application_controller.rb:10:3:12:5 | self in log_request | controllers/posts_controller.rb:25:3:27:5 | self in set_post |
23+
| controllers/application_controller.rb:11:35:11:41 | [post] self | controllers/application_controller.rb:6:3:8:5 | self in set_user |
24+
| controllers/application_controller.rb:11:35:11:41 | [post] self | controllers/photos_controller.rb:3:3:6:5 | self in show |
25+
| controllers/application_controller.rb:11:35:11:41 | [post] self | controllers/posts_controller.rb:25:3:27:5 | self in set_post |
1426
| controllers/application_controller.rb:11:53:11:59 | [post] self | controllers/application_controller.rb:6:3:8:5 | self in set_user |
1527
| controllers/application_controller.rb:11:53:11:59 | [post] self | controllers/photos_controller.rb:3:3:6:5 | self in show |
1628
| controllers/application_controller.rb:11:53:11:59 | [post] self | controllers/posts_controller.rb:25:3:27:5 | self in set_post |
17-
| controllers/application_controller.rb:11:53:11:59 | self | controllers/application_controller.rb:6:3:8:5 | self in set_user |
18-
| controllers/application_controller.rb:11:53:11:59 | self | controllers/photos_controller.rb:3:3:6:5 | self in show |
19-
| controllers/application_controller.rb:11:53:11:59 | self | controllers/posts_controller.rb:25:3:27:5 | self in set_post |
29+
| controllers/comments_controller.rb:17:3:51:5 | self in index | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
30+
| controllers/comments_controller.rb:18:5:18:11 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
31+
| controllers/comments_controller.rb:19:5:19:11 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
32+
| controllers/comments_controller.rb:20:5:20:11 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
33+
| controllers/comments_controller.rb:21:5:21:11 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
34+
| controllers/comments_controller.rb:22:5:22:11 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
35+
| controllers/comments_controller.rb:23:5:23:11 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
36+
| controllers/comments_controller.rb:24:5:24:11 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
37+
| controllers/comments_controller.rb:26:5:26:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
38+
| controllers/comments_controller.rb:28:5:28:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
39+
| controllers/comments_controller.rb:30:5:30:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
40+
| controllers/comments_controller.rb:31:5:31:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
41+
| controllers/comments_controller.rb:32:5:32:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
42+
| controllers/comments_controller.rb:33:5:33:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
43+
| controllers/comments_controller.rb:34:5:34:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
44+
| controllers/comments_controller.rb:36:5:36:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
45+
| controllers/comments_controller.rb:38:5:38:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
46+
| controllers/comments_controller.rb:40:5:40:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
47+
| controllers/comments_controller.rb:41:5:41:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
48+
| controllers/comments_controller.rb:42:5:42:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
49+
| controllers/comments_controller.rb:43:5:43:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
50+
| controllers/comments_controller.rb:44:5:44:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
51+
| controllers/comments_controller.rb:45:5:45:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
52+
| controllers/comments_controller.rb:47:5:47:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
53+
| controllers/comments_controller.rb:48:5:48:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
54+
| controllers/comments_controller.rb:49:5:49:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
2055
| controllers/comments_controller.rb:50:5:50:12 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
21-
| controllers/comments_controller.rb:50:5:50:12 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
2256
| controllers/comments_controller.rb:53:3:54:5 | self in create | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
57+
| controllers/comments_controller.rb:56:3:62:5 | self in show | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
2358
| controllers/comments_controller.rb:57:5:61:7 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
24-
| controllers/comments_controller.rb:57:5:61:7 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
25-
| controllers/comments_controller.rb:58:33:58:48 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
26-
| controllers/comments_controller.rb:60:58:60:63 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
59+
| controllers/comments_controller.rb:64:3:66:5 | self in photo | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
60+
| controllers/comments_controller.rb:65:5:65:20 | [post] self | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
2761
| controllers/comments_controller.rb:65:15:65:20 | [post] self | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
28-
| controllers/comments_controller.rb:65:15:65:20 | self | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
62+
| controllers/comments_controller.rb:68:3:70:5 | self in destroy | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
2963
| controllers/comments_controller.rb:69:12:69:18 | [post] self | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
30-
| controllers/comments_controller.rb:69:12:69:18 | self | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
64+
| controllers/comments_controller.rb:74:3:77:5 | self in ensure_user_can_edit_comments | controllers/comments_controller.rb:79:3:81:5 | self in set_comment |
65+
| controllers/comments_controller.rb:74:3:77:5 | self in ensure_user_can_edit_comments | controllers/comments_controller.rb:99:3:100:5 | self in foo |
66+
| controllers/comments_controller.rb:75:15:75:19 | [post] self | controllers/comments_controller.rb:79:3:81:5 | self in set_comment |
67+
| controllers/comments_controller.rb:75:15:75:19 | [post] self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
3168
| controllers/comments_controller.rb:76:5:76:68 | [post] self | controllers/comments_controller.rb:79:3:81:5 | self in set_comment |
3269
| controllers/comments_controller.rb:76:5:76:68 | [post] self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
33-
| controllers/comments_controller.rb:76:5:76:68 | self | controllers/comments_controller.rb:79:3:81:5 | self in set_comment |
34-
| controllers/comments_controller.rb:76:5:76:68 | self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
70+
| controllers/comments_controller.rb:79:3:81:5 | self in set_comment | controllers/comments_controller.rb:99:3:100:5 | self in foo |
3571
| controllers/comments_controller.rb:80:5:80:12 | [post] self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
72+
| controllers/comments_controller.rb:80:16:80:20 | [post] self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
3673
| controllers/comments_controller.rb:80:36:80:41 | [post] self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
74+
| controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
75+
| controllers/comments_controller.rb:84:45:84:49 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
3776
| controllers/comments_controller.rb:84:61:84:68 | [post] self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
38-
| controllers/comments_controller.rb:84:61:84:68 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
77+
| controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags | controllers/comments_controller.rb:95:3:97:5 | self in this_must_run_last |
3978
| controllers/comments_controller.rb:88:5:88:28 | [post] self | controllers/comments_controller.rb:95:3:97:5 | self in this_must_run_last |
40-
| controllers/comments_controller.rb:88:5:88:28 | self | controllers/comments_controller.rb:95:3:97:5 | self in this_must_run_last |
4179
| controllers/comments_controller.rb:91:3:93:5 | self in this_must_run_first | controllers/application_controller.rb:10:3:12:5 | self in log_request |
4280
| controllers/comments_controller.rb:99:3:100:5 | self in foo | controllers/comments_controller.rb:102:3:103:5 | self in bar |
4381
| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:17:3:51:5 | self in index |
4482
| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:53:3:54:5 | self in create |
4583
| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:56:3:62:5 | self in show |
4684
| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:64:3:66:5 | self in photo |
4785
| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:68:3:70:5 | self in destroy |
86+
| controllers/photos_controller.rb:3:3:6:5 | self in show | controllers/photos_controller.rb:8:3:9:5 | self in foo |
87+
| controllers/photos_controller.rb:4:5:4:6 | [post] self | controllers/photos_controller.rb:8:3:9:5 | self in foo |
4888
| controllers/photos_controller.rb:5:5:5:6 | [post] self | controllers/photos_controller.rb:8:3:9:5 | self in foo |
4989
| controllers/posts_controller.rb:20:3:21:5 | self in upvote | controllers/posts_controller.rb:29:3:31:5 | self in log_upvote |
90+
| controllers/posts_controller.rb:25:3:27:5 | self in set_post | controllers/application_controller.rb:6:3:8:5 | self in set_user |
5091
| controllers/posts_controller.rb:26:5:26:9 | [post] self | controllers/application_controller.rb:6:3:8:5 | self in set_user |
5192
| controllers/posts_controller.rb:26:23:26:28 | [post] self | controllers/application_controller.rb:6:3:8:5 | self in set_user |
93+
| filter_flow.rb:13:3:15:5 | self in a | filter_flow.rb:17:3:18:5 | self in b |
5294
| filter_flow.rb:14:5:14:8 | [post] self | filter_flow.rb:17:3:18:5 | self in b |
5395
| filter_flow.rb:14:12:14:17 | [post] self | filter_flow.rb:17:3:18:5 | self in b |
5496
| filter_flow.rb:17:3:18:5 | self in b | filter_flow.rb:20:3:22:5 | self in c |
97+
| filter_flow.rb:29:3:31:5 | self in a | filter_flow.rb:33:3:35:5 | self in b |
5598
| filter_flow.rb:30:5:30:8 | [post] self | filter_flow.rb:33:3:35:5 | self in b |
5699
| filter_flow.rb:30:12:30:17 | [post] self | filter_flow.rb:33:3:35:5 | self in b |
100+
| filter_flow.rb:33:3:35:5 | self in b | filter_flow.rb:37:3:39:5 | self in c |
57101
| filter_flow.rb:34:5:34:8 | [post] self | filter_flow.rb:37:3:39:5 | self in c |
102+
| filter_flow.rb:46:3:49:5 | self in a | filter_flow.rb:51:3:52:5 | self in b |
103+
| filter_flow.rb:47:5:47:8 | [post] self | filter_flow.rb:51:3:52:5 | self in b |
104+
| filter_flow.rb:47:12:47:17 | [post] self | filter_flow.rb:51:3:52:5 | self in b |
58105
| filter_flow.rb:48:5:48:8 | [post] self | filter_flow.rb:51:3:52:5 | self in b |
59106
| filter_flow.rb:51:3:52:5 | self in b | filter_flow.rb:54:3:56:5 | self in c |
107+
| filter_flow.rb:63:3:65:5 | self in a | filter_flow.rb:67:3:68:5 | self in b |
108+
| filter_flow.rb:64:5:64:8 | [post] self | filter_flow.rb:67:3:68:5 | self in b |
60109
| filter_flow.rb:64:16:64:21 | [post] self | filter_flow.rb:67:3:68:5 | self in b |
61-
| filter_flow.rb:64:16:64:21 | self | filter_flow.rb:67:3:68:5 | self in b |
62110
| filter_flow.rb:67:3:68:5 | self in b | filter_flow.rb:70:3:72:5 | self in c |
111+
| filter_flow.rb:79:3:81:5 | self in a | filter_flow.rb:83:3:84:5 | self in b |
63112
| filter_flow.rb:80:5:80:8 | [post] self | filter_flow.rb:83:3:84:5 | self in b |
64-
| filter_flow.rb:80:5:80:8 | self | filter_flow.rb:83:3:84:5 | self in b |
65113
| filter_flow.rb:83:3:84:5 | self in b | filter_flow.rb:86:3:88:5 | self in c |
66114
filterChain
67115
| controllers/comments_controller.rb:17:3:51:5 | index | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:99:3:100:5 | foo |

0 commit comments

Comments
 (0)