Skip to content

Commit 02656b1

Browse files
asgerfasgerf
authored
Merge pull request github#10685 from asgerf/rb/splat-and-local-field-step
Ruby: summarize unary splat operators and add local field step
2 parents 01bc5f7 + b6e07c0 commit 02656b1

File tree

5 files changed

+99
-10
lines changed

5 files changed

+99
-10
lines changed

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

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -522,7 +522,8 @@ module API {
522522
or
523523
exists(TypeTrackerSpecific::TypeTrackerContent c |
524524
TypeTrackerSpecific::basicLoadStep(node, ref, c) and
525-
lbl = Label::content(c.getAStoreContent())
525+
lbl = Label::content(c.getAStoreContent()) and
526+
not c.isSingleton(any(DataFlow::Content::AttributeNameContent k))
526527
)
527528
// note: method calls are not handled here as there is no DataFlow::Node for the intermediate MkMethodAccessNode API node
528529
}
@@ -638,7 +639,10 @@ module API {
638639
isUse(src) and
639640
t.start()
640641
or
641-
exists(TypeTracker t2 | result = trackUseNode(src, t2).track(t2, t))
642+
exists(TypeTracker t2 |
643+
result = trackUseNode(src, t2).track(t2, t) and
644+
not result instanceof DataFlowPrivate::SelfParameterNode
645+
)
642646
}
643647

644648
/**
@@ -657,7 +661,11 @@ module API {
657661
isDef(rhs) and
658662
result = rhs.getALocalSource()
659663
or
660-
exists(TypeBackTracker t2 | result = trackDefNode(rhs, t2).backtrack(t2, t))
664+
exists(TypeBackTracker t2, DataFlow::LocalSourceNode mid |
665+
mid = trackDefNode(rhs, t2) and
666+
not mid instanceof DataFlowPrivate::SelfParameterNode and
667+
result = mid.backtrack(t2, t)
668+
)
661669
}
662670

663671
/** Gets a data flow node reaching the RHS of the given def node. */

‎ruby/ql/lib/codeql/ruby/frameworks/Core.qll‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ class SubshellHeredocExecution extends SystemCommandExecution::Range {
5858
private class SplatSummary extends SummarizedCallable {
5959
SplatSummary() { this = "*(splat)" }
6060

61-
override SplatExpr getACall() { any() }
61+
override SplatExpr getACallSimple() { any() }
6262

6363
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
6464
(

‎ruby/ql/lib/codeql/ruby/typetracking/TypeTrackerSpecific.qll‎

Lines changed: 75 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,67 @@ private predicate summarizedLocalStep(Node nodeFrom, Node nodeTo) {
102102
}
103103

104104
/** Holds if there is a level step from `nodeFrom` to `nodeTo`. */
105-
predicate levelStep(Node nodeFrom, Node nodeTo) { summarizedLocalStep(nodeFrom, nodeTo) }
105+
predicate levelStep(Node nodeFrom, Node nodeTo) {
106+
summarizedLocalStep(nodeFrom, nodeTo)
107+
or
108+
localFieldStep(nodeFrom, nodeTo)
109+
}
110+
111+
/**
112+
* Gets a method of `mod`, with `instance` indicating if this is an instance method.
113+
*
114+
* Does not take inheritance or the various forms of inclusion into account.
115+
*/
116+
pragma[nomagic]
117+
private MethodBase getAMethod(ModuleBase mod, boolean instance) {
118+
not mod instanceof SingletonClass and
119+
result = mod.getAMethod() and
120+
if result instanceof SingletonMethod then instance = false else instance = true
121+
or
122+
exists(SingletonClass cls |
123+
cls.getValue().(SelfVariableAccess).getCfgScope() = mod and
124+
result = cls.getAMethod().(Method) and
125+
instance = false
126+
)
127+
}
128+
129+
/**
130+
* Gets a value flowing into `field` in `mod`, with `instance` indicating if it's
131+
* a field on an instance of `mod` (as opposed to the module object itself).
132+
*/
133+
pragma[nomagic]
134+
private Node fieldPredecessor(ModuleBase mod, boolean instance, string field) {
135+
exists(InstanceVariableWriteAccess access, AssignExpr assign |
136+
access.getReceiver().getCfgScope() = getAMethod(mod, instance) and
137+
field = access.getVariable().getName() and
138+
assign.getLeftOperand() = access and
139+
result.asExpr().getExpr() = assign.getRightOperand()
140+
)
141+
}
142+
143+
/**
144+
* Gets a reference to `field` in `mod`, with `instance` indicating if it's
145+
* a field on an instance of `mod` (as opposed to the module object itself).
146+
*/
147+
pragma[nomagic]
148+
private Node fieldSuccessor(ModuleBase mod, boolean instance, string field) {
149+
exists(InstanceVariableReadAccess access |
150+
access.getReceiver().getCfgScope() = getAMethod(mod, instance) and
151+
result.asExpr().getExpr() = access and
152+
field = access.getVariable().getName()
153+
)
154+
}
155+
156+
/**
157+
* Holds if `pred -> succ` should be used a level step, from a field assignment to
158+
* a read within the same class.
159+
*/
160+
private predicate localFieldStep(Node pred, Node succ) {
161+
exists(ModuleBase mod, boolean instance, string field |
162+
pred = fieldPredecessor(mod, instance, field) and
163+
succ = fieldSuccessor(mod, instance, field)
164+
)
165+
}
106166

107167
pragma[noinline]
108168
private predicate argumentPositionMatch(
@@ -325,9 +385,18 @@ private predicate hasStoreSummary(
325385
SummarizedCallable callable, DataFlow::ContentSet contents, SummaryComponentStack input,
326386
SummaryComponentStack output
327387
) {
328-
callable.propagatesFlow(input, push(SummaryComponent::content(contents), output), true) and
329388
not isNonLocal(input.head()) and
330-
not isNonLocal(output.head())
389+
not isNonLocal(output.head()) and
390+
(
391+
callable.propagatesFlow(input, push(SummaryComponent::content(contents), output), true)
392+
or
393+
// Allow the input to start with an arbitrary WithoutContent[X].
394+
// Since type-tracking only tracks one content deep, and we're about to store into another content,
395+
// we're already preventing the input from being in a content.
396+
callable
397+
.propagatesFlow(push(SummaryComponent::withoutContent(_), input),
398+
push(SummaryComponent::content(contents), output), true)
399+
)
331400
}
332401

333402
pragma[nomagic]
@@ -460,6 +529,9 @@ private predicate dependsOnSummaryComponentStack(
460529
callable.propagatesFlow(stack, _, true)
461530
or
462531
callable.propagatesFlow(_, stack, true)
532+
or
533+
// include store summaries as they may skip an initial step at the input
534+
hasStoreSummary(callable, _, stack, _)
463535
)
464536
or
465537
dependsOnSummaryComponentStackCons(callable, _, stack)

‎ruby/ql/test/library-tests/dataflow/array-flow/type-tracking-array-flow.expected‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,3 @@
1-
| array_flow.rb:3:16:3:35 | # $ hasValueFlow=0.1 | Missing result:hasValueFlow=0.1 |
2-
| array_flow.rb:5:16:5:35 | # $ hasValueFlow=0.1 | Missing result:hasValueFlow=0.1 |
3-
| array_flow.rb:83:13:83:30 | # $ hasValueFlow=9 | Missing result:hasValueFlow=9 |
41
| array_flow.rb:107:10:107:13 | ...[...] | Unexpected result: hasValueFlow=11.2 |
52
| array_flow.rb:179:28:179:46 | # $ hasValueFlow=19 | Missing result:hasValueFlow=19 |
63
| array_flow.rb:180:28:180:46 | # $ hasValueFlow=19 | Missing result:hasValueFlow=19 |

‎ruby/ql/test/library-tests/dataflow/type-tracker/TypeTracker.expected‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,16 @@ track
1616
| type_tracker.rb:2:5:5:7 | self in field= | type tracker with call steps | type_tracker.rb:7:5:9:7 | self in field |
1717
| type_tracker.rb:2:5:5:7 | self in field= | type tracker without call steps | type_tracker.rb:2:5:5:7 | self in field= |
1818
| type_tracker.rb:2:16:2:18 | val | type tracker with call steps | type_tracker.rb:2:16:2:18 | val |
19+
| type_tracker.rb:2:16:2:18 | val | type tracker with call steps | type_tracker.rb:8:9:8:14 | @field |
1920
| type_tracker.rb:2:16:2:18 | val | type tracker without call steps | type_tracker.rb:2:5:5:7 | return return in field= |
2021
| type_tracker.rb:2:16:2:18 | val | type tracker without call steps | type_tracker.rb:2:16:2:18 | val |
2122
| type_tracker.rb:2:16:2:18 | val | type tracker without call steps | type_tracker.rb:2:16:2:18 | val |
2223
| type_tracker.rb:2:16:2:18 | val | type tracker without call steps | type_tracker.rb:2:16:2:18 | val |
24+
| type_tracker.rb:2:16:2:18 | val | type tracker without call steps | type_tracker.rb:3:14:3:23 | call to field |
25+
| type_tracker.rb:2:16:2:18 | val | type tracker without call steps | type_tracker.rb:7:5:9:7 | return return in field |
26+
| type_tracker.rb:2:16:2:18 | val | type tracker without call steps | type_tracker.rb:8:9:8:14 | @field |
2327
| type_tracker.rb:2:16:2:18 | val | type tracker without call steps | type_tracker.rb:14:5:14:13 | call to field= |
28+
| type_tracker.rb:2:16:2:18 | val | type tracker without call steps | type_tracker.rb:15:10:15:18 | call to field |
2429
| type_tracker.rb:3:9:3:23 | call to puts | type tracker without call steps | type_tracker.rb:3:9:3:23 | call to puts |
2530
| type_tracker.rb:3:14:3:23 | call to field | type tracker without call steps | type_tracker.rb:3:14:3:23 | call to field |
2631
| type_tracker.rb:4:9:4:14 | @field | type tracker without call steps | type_tracker.rb:4:9:4:14 | @field |
@@ -55,6 +60,7 @@ track
5560
| type_tracker.rb:14:5:14:13 | call to field= | type tracker without call steps | type_tracker.rb:14:5:14:13 | call to field= |
5661
| type_tracker.rb:14:17:14:23 | "hello" | type tracker with call steps | type_tracker.rb:2:16:2:18 | val |
5762
| type_tracker.rb:14:17:14:23 | "hello" | type tracker with call steps | type_tracker.rb:2:16:2:18 | val |
63+
| type_tracker.rb:14:17:14:23 | "hello" | type tracker with call steps | type_tracker.rb:8:9:8:14 | @field |
5864
| type_tracker.rb:14:17:14:23 | "hello" | type tracker with call steps with content attribute field | type_tracker.rb:7:5:9:7 | self (field) |
5965
| type_tracker.rb:14:17:14:23 | "hello" | type tracker with call steps with content attribute field | type_tracker.rb:7:5:9:7 | self in field |
6066
| type_tracker.rb:14:17:14:23 | "hello" | type tracker without call steps | type_tracker.rb:14:5:14:13 | call to field= |
@@ -368,11 +374,16 @@ trackEnd
368374
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:2:16:2:18 | val |
369375
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:2:16:2:18 | val |
370376
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:2:16:2:18 | val |
377+
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:3:14:3:23 | call to field |
371378
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:4:9:4:20 | ... = ... |
372379
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:4:9:4:20 | ... = ... |
373380
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:4:18:4:20 | val |
374381
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:4:18:4:20 | val |
382+
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:7:5:9:7 | return return in field |
383+
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:8:9:8:14 | @field |
384+
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:8:9:8:14 | @field |
375385
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:14:5:14:13 | call to field= |
386+
| type_tracker.rb:2:16:2:18 | val | type_tracker.rb:15:10:15:18 | call to field |
376387
| type_tracker.rb:3:9:3:23 | call to puts | type_tracker.rb:3:9:3:23 | call to puts |
377388
| type_tracker.rb:3:14:3:23 | call to field | type_tracker.rb:3:14:3:23 | call to field |
378389
| type_tracker.rb:4:9:4:14 | @field | type_tracker.rb:4:9:4:14 | @field |
@@ -424,6 +435,7 @@ trackEnd
424435
| type_tracker.rb:14:17:14:23 | "hello" | type_tracker.rb:2:16:2:18 | val |
425436
| type_tracker.rb:14:17:14:23 | "hello" | type_tracker.rb:4:9:4:20 | ... = ... |
426437
| type_tracker.rb:14:17:14:23 | "hello" | type_tracker.rb:4:18:4:20 | val |
438+
| type_tracker.rb:14:17:14:23 | "hello" | type_tracker.rb:8:9:8:14 | @field |
427439
| type_tracker.rb:14:17:14:23 | "hello" | type_tracker.rb:14:5:14:13 | __synth__0 |
428440
| type_tracker.rb:14:17:14:23 | "hello" | type_tracker.rb:14:5:14:13 | call to field= |
429441
| type_tracker.rb:14:17:14:23 | "hello" | type_tracker.rb:14:5:14:23 | ... |

0 commit comments

Comments
 (0)