Skip to content

Commit 45af148

Browse files
RasmusWLRasmusWL
authored
Merge pull request #9215 from RasmusWL/ruby-mad-argument-self
Ruby: Fixes for `Argument[any,any-named]` in MaD
2 parents 0ba2a67 + 24750dc commit 45af148

File tree

9 files changed

+268
-113
lines changed

9 files changed

+268
-113
lines changed

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -780,6 +780,24 @@ module API {
780780
or
781781
pos.isBlock() and
782782
result = Label::blockParameter()
783+
or
784+
pos.isAny() and
785+
(
786+
result = Label::parameter(_)
787+
or
788+
result = Label::keywordParameter(_)
789+
or
790+
result = Label::blockParameter()
791+
// NOTE: `self` should NOT be included, as described in the QLDoc for `isAny()`
792+
)
793+
or
794+
pos.isAnyNamed() and
795+
result = Label::keywordParameter(_)
796+
//
797+
// Note: there is currently no API graph label for `self`.
798+
// It was omitted since in practice it means going back to where you came from.
799+
// For example, `base.getMethod("foo").getSelf()` would just be `base`.
800+
// However, it's possible we'll need it later, for identifying `self` parameters or post-update nodes.
783801
}
784802

785803
/** Gets the API graph label corresponding to the given parameter position. */
@@ -796,6 +814,24 @@ module API {
796814
or
797815
pos.isBlock() and
798816
result = Label::blockParameter()
817+
or
818+
pos.isAny() and
819+
(
820+
result = Label::parameter(_)
821+
or
822+
result = Label::keywordParameter(_)
823+
or
824+
result = Label::blockParameter()
825+
// NOTE: `self` should NOT be included, as described in the QLDoc for `isAny()`
826+
)
827+
or
828+
pos.isAnyNamed() and
829+
result = Label::keywordParameter(_)
830+
//
831+
// Note: there is currently no API graph label for `self`.
832+
// It was omitted since in practice it means going back to where you came from.
833+
// For example, `base.getMethod("foo").getSelf()` would just be `base`.
834+
// However, it's possible we'll need it later, for identifying `self` parameters or post-update nodes.
799835
}
800836
}
801837
}

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

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,9 @@ private module Cached {
260260
or
261261
FlowSummaryImplSpecific::ParsePositions::isParsedKeywordParameterPosition(_, name)
262262
} or
263-
THashSplatArgumentPosition()
263+
THashSplatArgumentPosition() or
264+
TAnyArgumentPosition() or
265+
TAnyKeywordArgumentPosition()
264266

265267
cached
266268
newtype TParameterPosition =
@@ -280,7 +282,8 @@ private module Cached {
280282
FlowSummaryImplSpecific::ParsePositions::isParsedKeywordArgumentPosition(_, name)
281283
} or
282284
THashSplatParameterPosition() or
283-
TAnyParameterPosition()
285+
TAnyParameterPosition() or
286+
TAnyKeywordParameterPosition()
284287
}
285288

286289
import Cached
@@ -482,11 +485,14 @@ class ParameterPosition extends TParameterPosition {
482485
predicate isHashSplat() { this = THashSplatParameterPosition() }
483486

484487
/**
485-
* Holds if this position represents any parameter. This includes both positional
486-
* and named parameters.
488+
* Holds if this position represents any parameter, except `self` parameters. This
489+
* includes both positional, named, and block parameters.
487490
*/
488491
predicate isAny() { this = TAnyParameterPosition() }
489492

493+
/** Holds if this position represents any positional parameter. */
494+
predicate isAnyNamed() { this = TAnyKeywordParameterPosition() }
495+
490496
/** Gets a textual representation of this position. */
491497
string toString() {
492498
this.isSelf() and result = "self"
@@ -502,6 +508,8 @@ class ParameterPosition extends TParameterPosition {
502508
this.isHashSplat() and result = "**"
503509
or
504510
this.isAny() and result = "any"
511+
or
512+
this.isAnyNamed() and result = "any-named"
505513
}
506514
}
507515

@@ -519,6 +527,15 @@ class ArgumentPosition extends TArgumentPosition {
519527
/** Holds if this position represents a keyword argument named `name`. */
520528
predicate isKeyword(string name) { this = TKeywordArgumentPosition(name) }
521529

530+
/**
531+
* Holds if this position represents any argument, except `self` arguments. This
532+
* includes both positional, named, and block arguments.
533+
*/
534+
predicate isAny() { this = TAnyArgumentPosition() }
535+
536+
/** Holds if this position represents any positional parameter. */
537+
predicate isAnyNamed() { this = TAnyKeywordArgumentPosition() }
538+
522539
/**
523540
* Holds if this position represents a synthesized argument containing all keyword
524541
* arguments wrapped in a hash.
@@ -535,12 +552,16 @@ class ArgumentPosition extends TArgumentPosition {
535552
or
536553
exists(string name | this.isKeyword(name) and result = "keyword " + name)
537554
or
555+
this.isAny() and result = "any"
556+
or
557+
this.isAnyNamed() and result = "any-named"
558+
or
538559
this.isHashSplat() and result = "**"
539560
}
540561
}
541562

542563
/** Holds if arguments at position `apos` match parameters at position `ppos`. */
543-
pragma[inline]
564+
pragma[nomagic]
544565
predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
545566
ppos.isSelf() and apos.isSelf()
546567
or
@@ -556,5 +577,11 @@ predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) {
556577
or
557578
ppos.isHashSplat() and apos.isHashSplat()
558579
or
559-
ppos.isAny() and exists(apos)
580+
ppos.isAny() and not apos.isSelf()
581+
or
582+
apos.isAny() and not ppos.isSelf()
583+
or
584+
ppos.isAnyNamed() and apos.isKeyword(_)
585+
or
586+
apos.isAnyNamed() and ppos.isKeyword(_)
560587
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,12 @@ ArgumentPosition parseParamBody(string s) {
293293
or
294294
s = "block" and
295295
result.isBlock()
296+
or
297+
s = "any" and
298+
result.isAny()
299+
or
300+
s = "any-named" and
301+
result.isAnyNamed()
296302
}
297303

298304
/** Gets the parameter position obtained by parsing `X` in `Argument[X]`. */
@@ -317,4 +323,10 @@ ParameterPosition parseArgBody(string s) {
317323
or
318324
s = "block" and
319325
result.isBlock()
326+
or
327+
s = "any" and
328+
result.isAny()
329+
or
330+
s = "any-named" and
331+
result.isAnyNamed()
320332
}

‎ruby/ql/lib/codeql/ruby/frameworks/core/Hash.qll‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -378,10 +378,10 @@ private class MergeSummary extends SimpleSummarizedCallable {
378378

379379
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
380380
(
381-
input = "Argument[any].WithElement[any]" and
381+
input = "Argument[self,any].WithElement[any]" and
382382
output = "ReturnValue"
383383
or
384-
input = "Argument[any].Element[any]" and
384+
input = "Argument[self,any].Element[any]" and
385385
output = "Argument[block].Parameter[1,2]"
386386
) and
387387
preservesValue = true
@@ -393,10 +393,10 @@ private class MergeBangSummary extends SimpleSummarizedCallable {
393393

394394
override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
395395
(
396-
input = "Argument[any].WithElement[any]" and
396+
input = "Argument[self,any].WithElement[any]" and
397397
output = ["ReturnValue", "Argument[self]"]
398398
or
399-
input = "Argument[any].Element[any]" and
399+
input = "Argument[self,any].Element[any]" and
400400
output = "Argument[block].Parameter[1,2]"
401401
) and
402402
preservesValue = true

‎ruby/ql/lib/codeql/ruby/frameworks/data/internal/ApiGraphModelsSpecific.qll‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ API::Node getExtraSuccessorFromNode(API::Node node, AccessPathToken token) {
115115
or
116116
token.getName() = "Parameter" and
117117
result =
118-
node.getASuccessor(API::Label::getLabelFromArgumentPosition(FlowSummaryImplSpecific::parseParamBody(token
118+
node.getASuccessor(API::Label::getLabelFromParameterPosition(FlowSummaryImplSpecific::parseArgBody(token
119119
.getAnArgument())))
120120
// Note: The "Element" token is not implemented yet, as it ultimately requires type-tracking and
121121
// API graphs to be aware of the steps involving Element contributed by the standard library model.
@@ -129,7 +129,7 @@ bindingset[token]
129129
API::Node getExtraSuccessorFromInvoke(InvokeNode node, AccessPathToken token) {
130130
token.getName() = "Argument" and
131131
result =
132-
node.getASuccessor(API::Label::getLabelFromParameterPosition(FlowSummaryImplSpecific::parseArgBody(token
132+
node.getASuccessor(API::Label::getLabelFromArgumentPosition(FlowSummaryImplSpecific::parseParamBody(token
133133
.getAnArgument())))
134134
}
135135

@@ -181,7 +181,7 @@ predicate isExtraValidTokenArgumentInIdentifyingAccessPath(string name, string a
181181
or
182182
name = ["Argument", "Parameter"] and
183183
(
184-
argument = ["self", "block"]
184+
argument = ["self", "block", "any", "any-named"]
185185
or
186186
argument.regexpMatch("\\w+:") // keyword argument
187187
)

‎ruby/ql/test/library-tests/dataflow/hash-flow/hash-flow.expected‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -248,9 +248,7 @@ edges
248248
| hash_flow.rb:414:11:414:14 | hash [element :f] : | hash_flow.rb:414:11:414:18 | ...[...] : |
249249
| hash_flow.rb:414:11:414:18 | ...[...] : | hash_flow.rb:414:10:414:19 | ( ... ) |
250250
| hash_flow.rb:421:15:421:25 | call to taint : | hash_flow.rb:430:12:430:16 | hash1 [element :a] : |
251-
| hash_flow.rb:421:15:421:25 | call to taint : | hash_flow.rb:442:11:442:15 | hash1 [element :a] : |
252251
| hash_flow.rb:423:15:423:25 | call to taint : | hash_flow.rb:430:12:430:16 | hash1 [element :c] : |
253-
| hash_flow.rb:423:15:423:25 | call to taint : | hash_flow.rb:444:11:444:15 | hash1 [element :c] : |
254252
| hash_flow.rb:426:15:426:25 | call to taint : | hash_flow.rb:430:25:430:29 | hash2 [element :d] : |
255253
| hash_flow.rb:428:15:428:25 | call to taint : | hash_flow.rb:430:25:430:29 | hash2 [element :f] : |
256254
| hash_flow.rb:430:12:430:16 | [post] hash1 [element :a] : | hash_flow.rb:442:11:442:15 | hash1 [element :a] : |
@@ -444,9 +442,7 @@ edges
444442
| hash_flow.rb:663:11:663:14 | hash [element] : | hash_flow.rb:663:11:663:18 | ...[...] : |
445443
| hash_flow.rb:663:11:663:18 | ...[...] : | hash_flow.rb:663:10:663:19 | ( ... ) |
446444
| hash_flow.rb:670:15:670:25 | call to taint : | hash_flow.rb:679:12:679:16 | hash1 [element :a] : |
447-
| hash_flow.rb:670:15:670:25 | call to taint : | hash_flow.rb:691:11:691:15 | hash1 [element :a] : |
448445
| hash_flow.rb:672:15:672:25 | call to taint : | hash_flow.rb:679:12:679:16 | hash1 [element :c] : |
449-
| hash_flow.rb:672:15:672:25 | call to taint : | hash_flow.rb:693:11:693:15 | hash1 [element :c] : |
450446
| hash_flow.rb:675:15:675:25 | call to taint : | hash_flow.rb:679:25:679:29 | hash2 [element :d] : |
451447
| hash_flow.rb:677:15:677:25 | call to taint : | hash_flow.rb:679:25:679:29 | hash2 [element :f] : |
452448
| hash_flow.rb:679:12:679:16 | [post] hash1 [element :a] : | hash_flow.rb:691:11:691:15 | hash1 [element :a] : |

0 commit comments

Comments
 (0)