Skip to content

Commit 0a7eecf

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#13795 from geoffw0/enumcontent
Swift: Support EnumContent in models-as-data
2 parents 1f39ec3 + b8f67d7 commit 0a7eecf

File tree

11 files changed

+875
-606
lines changed

11 files changed

+875
-606
lines changed

‎swift/ql/lib/change-notes/2023-07-24-forced-unwrap.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
5+
* Flow through forced optional unwrapping (`!`) is modelled more accurately.

‎swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -476,9 +476,18 @@ private predicate parseField(AccessPathToken c, Content::FieldContent f) {
476476
)
477477
}
478478

479+
private predicate parseEnum(AccessPathToken c, Content::EnumContent f) {
480+
c.getName() = "EnumElement" and
481+
c.getAnArgument() = f.getSignature()
482+
or
483+
c.getName() = "OptionalSome" and
484+
f.getSignature() = "some:0"
485+
}
486+
479487
/** Holds if the specification component parses as a `Content`. */
480488
predicate parseContent(AccessPathToken component, Content content) {
481-
parseField(component, content)
489+
parseField(component, content) or
490+
parseEnum(component, content)
482491
}
483492

484493
cached

‎swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,10 @@ private module Cached {
170170
nodeFrom.asExpr() = nodeTo.asExpr().(AnyTryExpr).getSubExpr()
171171
or
172172
// flow through `!`
173+
// note: there's a case in `readStep` that handles when the source is the
174+
// `OptionalSomeContentSet` within the RHS. This case is for when the
175+
// `Optional` itself is tainted (which it usually shouldn't be, but
176+
// retaining this case increases robustness of flow).
173177
nodeFrom.asExpr() = nodeTo.asExpr().(ForceValueExpr).getSubExpr()
174178
or
175179
// flow through `?` and `?.`
@@ -725,6 +729,10 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
725729
)
726730
)
727731
or
732+
// read of an enum (`Optional.Some`) member via `!`
733+
node1.asExpr() = node2.asExpr().(ForceValueExpr).getSubExpr() and
734+
c instanceof OptionalSomeContentSet
735+
or
728736
// read of a tuple member via `case let (v1, v2)` pattern matching
729737
exists(TuplePattern tupPat, int idx, Pattern subPat |
730738
node1.asPattern() = tupPat and

‎swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -202,9 +202,22 @@ module Content {
202202
/** Gets the declaration of the enum parameter. */
203203
ParamDecl getParam() { result = p }
204204

205-
override string toString() {
205+
/**
206+
* Gets a string describing this enum content, of the form: `EnumElementName:N` where `EnumElementName`
207+
* is the name of the enum element declaration within the enum, and `N` is the 0-based index of the
208+
* parameter that this content is for. For example in the following code there is only one `EnumContent`
209+
* and it's signature is `myValue:0`:
210+
* ```
211+
* enum MyEnum {
212+
* case myValue(Int)
213+
* }
214+
* ```
215+
*/
216+
string getSignature() {
206217
exists(EnumElementDecl d, int pos | d.getParam(pos) = p | result = d.toString() + ":" + pos)
207218
}
219+
220+
override string toString() { result = this.getSignature() }
208221
}
209222
}
210223

‎swift/ql/test/library-tests/dataflow/dataflow/CONSISTENCY/CfgConsistency.expected

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
multipleSuccessors
2-
| test.swift:488:8:488:12 | let ...? | no-match | test.swift:488:27:488:27 | y |
3-
| test.swift:488:8:488:12 | let ...? | no-match | test.swift:493:9:493:9 | tuple1 |
2+
| test.swift:519:8:519:12 | let ...? | no-match | test.swift:519:27:519:27 | y |
3+
| test.swift:519:8:519:12 | let ...? | no-match | test.swift:524:9:524:9 | tuple1 |

‎swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected

Lines changed: 352 additions & 268 deletions
Large diffs are not rendered by default.

‎swift/ql/test/library-tests/dataflow/dataflow/FlowConfig.qll

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,14 @@ module TestConfiguration implements DataFlow::ConfigSig {
2121

2222
private class TestSummaries extends SummaryModelCsv {
2323
override predicate row(string row) {
24-
// model to allow data flow through `signum()` as though it were an identity function, for the benefit of testing flow through optional chaining (`x?.`).
25-
row = ";Int;true;signum();;;Argument[-1];ReturnValue;value"
24+
row =
25+
[
26+
// model to allow data flow through `signum()` as though it were an identity function, for the benefit of testing flow through optional chaining (`x?.`).
27+
";Int;true;signum();;;Argument[-1];ReturnValue;value",
28+
// test Enum content in MAD
29+
";;false;mkMyEnum2(_:);;;Argument[0];ReturnValue.EnumElement[mySingle:0];value",
30+
";;false;mkOptional2(_:);;;Argument[0];ReturnValue.OptionalSome;value"
31+
]
2632
}
2733
}
2834

‎swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected

Lines changed: 374 additions & 310 deletions
Large diffs are not rendered by default.

‎swift/ql/test/library-tests/dataflow/dataflow/test.swift

Lines changed: 53 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,11 @@ indirect enum MyEnum {
372372
case myCons(Int, MyEnum)
373373
}
374374

375+
func mkMyEnum1(_ v: Int) -> MyEnum { return MyEnum.mySingle(v) }
376+
func mkMyEnum2(_ v: Int) -> MyEnum { return MyEnum.myNone } // modelled flow
377+
func mkOptional1(_ v: Int) -> Int? { return Optional.some(v) }
378+
func mkOptional2(_ v: Int) -> Int? { return nil } // modelled flow
379+
375380
func testEnums() {
376381
var a : MyEnum = .myNone
377382

@@ -401,7 +406,7 @@ func testEnums() {
401406
case .myNone:
402407
()
403408
case .mySingle(let a):
404-
sink(arg: a) // $ flow=398
409+
sink(arg: a) // $ flow=403
405410
case .myPair(let a, let b):
406411
sink(arg: a)
407412
sink(arg: b)
@@ -410,7 +415,7 @@ func testEnums() {
410415
}
411416

412417
if case .mySingle(let x) = a {
413-
sink(arg: x) // $ flow=398
418+
sink(arg: x) // $ flow=403
414419
}
415420
if case .myPair(let x, let y) = a {
416421
sink(arg: x)
@@ -426,7 +431,7 @@ func testEnums() {
426431
sink(arg: a)
427432
case .myPair(let a, let b):
428433
sink(arg: a)
429-
sink(arg: b) // $ flow=420
434+
sink(arg: b) // $ flow=425
430435
case let .myCons(a, _):
431436
sink(arg: a)
432437
}
@@ -436,7 +441,7 @@ func testEnums() {
436441
}
437442
if case .myPair(let x, let y) = a {
438443
sink(arg: x)
439-
sink(arg: y) // $ flow=420
444+
sink(arg: y) // $ flow=425
440445
}
441446

442447
let b: MyEnum = .myCons(42, a)
@@ -452,7 +457,7 @@ func testEnums() {
452457
case let .myCons(a, .myPair(b, c)):
453458
sink(arg: a)
454459
sink(arg: b)
455-
sink(arg: c) // $ flow=420
460+
sink(arg: c) // $ flow=425
456461
case let .myCons(a, _):
457462
sink(arg: a)
458463
}
@@ -461,23 +466,49 @@ func testEnums() {
461466
sink(arg: x)
462467
}
463468
if case MyEnum.myPair(let x, let y) = .myPair(source(), 0) {
464-
sink(arg: x) // $ flow=463
469+
sink(arg: x) // $ flow=468
465470
sink(arg: y)
466471
}
467472
if case let .myCons(_, .myPair(_, c)) = b {
468-
sink(arg: c) // $ flow=420
473+
sink(arg: c) // $ flow=425
469474
}
470475

471476
switch (a, b) {
472477
case let (.myPair(a, b), .myCons(c, .myPair(d, e))):
473478
sink(arg: a)
474-
sink(arg: b) // $ flow=420
479+
sink(arg: b) // $ flow=425
475480
sink(arg: c)
476481
sink(arg: d)
477-
sink(arg: e) // $ flow=420
482+
sink(arg: e) // $ flow=425
478483
default:
479484
()
480485
}
486+
487+
let c1 = MyEnum.mySingle(0)
488+
let c2 = MyEnum.mySingle(source())
489+
let c3 = mkMyEnum1(0)
490+
let c4 = mkMyEnum1(source())
491+
let c5 = mkMyEnum2(0)
492+
let c6 = mkMyEnum2(source())
493+
if case MyEnum.mySingle(let d1) = c1 { sink(arg: d1) }
494+
if case MyEnum.mySingle(let d2) = c2 { sink(arg: d2) } // $ flow=488
495+
if case MyEnum.mySingle(let d3) = c3 { sink(arg: d3) }
496+
if case MyEnum.mySingle(let d4) = c4 { sink(arg: d4) } // $ flow=490
497+
if case MyEnum.mySingle(let d5) = c5 { sink(arg: d5) }
498+
if case MyEnum.mySingle(let d6) = c6 { sink(arg: d6) } // $ flow=492
499+
500+
let e1 = Optional.some(0)
501+
let e2 = Optional.some(source())
502+
let e3 = mkOptional1(0)
503+
let e4 = mkOptional1(source())
504+
let e5 = mkOptional2(0)
505+
let e6 = mkOptional2(source())
506+
sink(arg: e1!)
507+
sink(arg: e2!) // $ flow=501
508+
sink(arg: e3!)
509+
sink(arg: e4!) // $ flow=503
510+
sink(arg: e5!)
511+
sink(arg: e6!) // $ flow=505
481512
}
482513

483514
func source2() -> (Int, Int)? { return nil }
@@ -523,8 +554,8 @@ func testOptionalPropertyAccess(y: Int?) {
523554
}
524555

525556
func testIdentityArithmetic() {
526-
sink(arg: +source()) // $ flow=526
527-
sink(arg: (source())) // $ flow=527
557+
sink(arg: +source()) // $ flow=557
558+
sink(arg: (source())) // $ flow=558
528559
}
529560

530561
func sink(str: String) {}
@@ -541,13 +572,13 @@ class MyClass {
541572
extension MyClass {
542573
convenience init(contentsOfFile: String) {
543574
self.init(s: source3())
544-
sink(str: str) // $ flow=543
575+
sink(str: str) // $ flow=574
545576
}
546577
}
547578

548579
func extensionInits(path: String) {
549-
sink(str: MyClass(s: source3()).str) // $ flow=549
550-
sink(str: MyClass(contentsOfFile: path).str) // $ flow=543
580+
sink(str: MyClass(s: source3()).str) // $ flow=580
581+
sink(str: MyClass(contentsOfFile: path).str) // $ flow=574
551582
}
552583

553584
class InoutConstructorClass {
@@ -572,10 +603,10 @@ struct S {
572603
func testKeyPath() {
573604
let s = S(x: source())
574605
let f = \S.x
575-
sink(arg: s[keyPath: f]) // $ flow=573
606+
sink(arg: s[keyPath: f]) // $ flow=604
576607

577608
let inferred : KeyPath<S, Int> = \.x
578-
sink(arg: s[keyPath: inferred]) // $ flow=573
609+
sink(arg: s[keyPath: inferred]) // $ flow=604
579610
}
580611

581612
struct S2 {
@@ -590,13 +621,13 @@ func testNestedKeyPath() {
590621
let s = S(x: source())
591622
let s2 = S2(s: s)
592623
let f = \S2.s.x
593-
sink(arg: s2[keyPath: f]) // $ flow=590
624+
sink(arg: s2[keyPath: f]) // $ flow=621
594625
}
595626

596627
func testArrayKeyPath() {
597628
let array = [source()]
598629
let f = \[Int].[0]
599-
sink(arg: array[keyPath: f]) // $ MISSING: flow=597
630+
sink(arg: array[keyPath: f]) // $ MISSING: flow=628
600631
}
601632

602633
struct S2_Optional {
@@ -611,7 +642,7 @@ func testOptionalKeyPath() {
611642
let s = S(x: source())
612643
let s2 = S2_Optional(s: s)
613644
let f = \S2_Optional.s?.x
614-
sink(opt: s2[keyPath: f]) // $ MISSING: flow=611
645+
sink(opt: s2[keyPath: f]) // $ MISSING: flow=642
615646
}
616647

617648
func testSwap() {
@@ -623,11 +654,11 @@ func testSwap() {
623654
x = y
624655
y = t
625656
sink(arg: x)
626-
sink(arg: y) // $ flow=618
657+
sink(arg: y) // $ flow=649
627658

628659
x = source()
629660
y = 0
630661
swap(&x, &y)
631-
sink(arg: x) // $ SPURIOUS: flow=628
632-
sink(arg: y) // $ flow=628
662+
sink(arg: x) // $ SPURIOUS: flow=659
663+
sink(arg: y) // $ flow=659
633664
}

‎swift/ql/test/library-tests/dataflow/taint/core/Taint.expected

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,10 @@ edges
7272
| subscript.swift:14:15:14:23 | call to source2() | subscript.swift:14:15:14:26 | ...[...] |
7373
| try.swift:9:17:9:24 | call to source() | try.swift:9:13:9:24 | try ... |
7474
| try.swift:15:17:15:24 | call to source() | try.swift:15:12:15:24 | try! ... |
75+
| try.swift:18:13:18:25 | try? ... [some:0] | try.swift:18:12:18:27 | ...! |
7576
| try.swift:18:18:18:25 | call to source() | try.swift:18:12:18:27 | ...! |
77+
| try.swift:18:18:18:25 | call to source() | try.swift:18:18:18:25 | call to source() [some:0] |
78+
| try.swift:18:18:18:25 | call to source() [some:0] | try.swift:18:13:18:25 | try? ... [some:0] |
7679
nodes
7780
| file://:0:0:0:0 | .first | semmle.label | .first |
7881
| file://:0:0:0:0 | .second | semmle.label | .second |
@@ -185,7 +188,9 @@ nodes
185188
| try.swift:15:12:15:24 | try! ... | semmle.label | try! ... |
186189
| try.swift:15:17:15:24 | call to source() | semmle.label | call to source() |
187190
| try.swift:18:12:18:27 | ...! | semmle.label | ...! |
191+
| try.swift:18:13:18:25 | try? ... [some:0] | semmle.label | try? ... [some:0] |
188192
| try.swift:18:18:18:25 | call to source() | semmle.label | call to source() |
193+
| try.swift:18:18:18:25 | call to source() [some:0] | semmle.label | call to source() [some:0] |
189194
subpaths
190195
| stringinterpolation.swift:13:36:13:36 | pair [first] | stringinterpolation.swift:6:6:6:6 | self [first] | file://:0:0:0:0 | .first | stringinterpolation.swift:13:36:13:41 | .first |
191196
| stringinterpolation.swift:19:13:19:20 | call to source() | stringinterpolation.swift:6:6:6:6 | value | file://:0:0:0:0 | [post] self [first] | stringinterpolation.swift:19:2:19:2 | [post] p1 [first] |

0 commit comments

Comments
 (0)