Skip to content

Swift: Support EnumContent in models-as-data #13795

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
MathiasVP merged 8 commits into from
Jul 25, 2023
Merged

Swift: Support EnumContent in models-as-data #13795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1cfb9bb
Swift: Tests for enum content in MAD.
geoffw0 Jul 21, 2023
d9187c6
Swift: Parse MAD enum content.
geoffw0 Jul 21, 2023
89e7b75
Swift: Flow from optional content through "!".
geoffw0 Jul 21, 2023
1259f8d
Swift: Clearer syntax and a shortcut for Optional.some.
geoffw0 Jul 21, 2023
94780aa
Swift: Effect on the taint/core test.
geoffw0 Jul 21, 2023
dc89e0d
Swift: Change note.
geoffw0 Jul 24, 2023
b62c86d
Swift: Autoformat.
geoffw0 Jul 24, 2023
b8f67d7
Swift: Test changes (not affecting results).
geoffw0 Jul 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions swift/ql/lib/change-notes/2023-07-24-forced-unwrap.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
category: minorAnalysis
---

* Flow through forced optional unwrapping (`!`) is modelled more accurately.
11 changes: 10 additions & 1 deletion swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,9 +476,18 @@ private predicate parseField(AccessPathToken c, Content::FieldContent f) {
)
}

private predicate parseEnum(AccessPathToken c, Content::EnumContent f) {
c.getName() = "EnumElement" and
c.getAnArgument() = f.getSignature()
or
c.getName() = "OptionalSome" and
f.getSignature() = "some:0"
}

/** Holds if the specification component parses as a `Content`. */
predicate parseContent(AccessPathToken component, Content content) {
parseField(component, content)
parseField(component, content) or
parseEnum(component, content)
}

cached
Expand Down
8 changes: 8 additions & 0 deletions swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@ private module Cached {
nodeFrom.asExpr() = nodeTo.asExpr().(AnyTryExpr).getSubExpr()
or
// flow through `!`
// note: there's a case in `readStep` that handles when the source is the
// `OptionalSomeContentSet` within the RHS. This case is for when the
// `Optional` itself is tainted (which it usually shouldn't be, but
// retaining this case increases robustness of flow).
nodeFrom.asExpr() = nodeTo.asExpr().(ForceValueExpr).getSubExpr()
or
// flow through `?` and `?.`
Expand Down Expand Up @@ -725,6 +729,10 @@ predicate readStep(Node node1, ContentSet c, Node node2) {
)
)
or
// read of an enum (`Optional.Some`) member via `!`
node1.asExpr() = node2.asExpr().(ForceValueExpr).getSubExpr() and
c instanceof OptionalSomeContentSet
or
// read of a tuple member via `case let (v1, v2)` pattern matching
exists(TuplePattern tupPat, int idx, Pattern subPat |
node1.asPattern() = tupPat and
Expand Down
15 changes: 14 additions & 1 deletion swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPublic.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,9 +202,22 @@ module Content {
/** Gets the declaration of the enum parameter. */
ParamDecl getParam() { result = p }

override string toString() {
/**
* Gets a string describing this enum content, of the form: `EnumElementName:N` where `EnumElementName`
* is the name of the enum element declaration within the enum, and `N` is the 0-based index of the
* parameter that this content is for. For example in the following code there is only one `EnumContent`
* and it's signature is `myValue:0`:
* ```
* enum MyEnum {
* case myValue(Int)
* }
* ```
*/
string getSignature() {
exists(EnumElementDecl d, int pos | d.getParam(pos) = p | result = d.toString() + ":" + pos)
}

override string toString() { result = this.getSignature() }
}
}

Expand Down
4 changes: 2 additions & 2 deletions swift/ql/test/library-tests/dataflow/dataflow/CONSISTENCY/CfgConsistency.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
multipleSuccessors
| test.swift:488:8:488:12 | let ...? | no-match | test.swift:488:27:488:27 | y |
| test.swift:488:8:488:12 | let ...? | no-match | test.swift:493:9:493:9 | tuple1 |
| test.swift:519:8:519:12 | let ...? | no-match | test.swift:519:27:519:27 | y |
| test.swift:519:8:519:12 | let ...? | no-match | test.swift:524:9:524:9 | tuple1 |
620 changes: 352 additions & 268 deletions swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected
View file
Open in desktop

Large diffs are not rendered by default.

10 changes: 8 additions & 2 deletions swift/ql/test/library-tests/dataflow/dataflow/FlowConfig.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,14 @@ module TestConfiguration implements DataFlow::ConfigSig {

private class TestSummaries extends SummaryModelCsv {
override predicate row(string row) {
// model to allow data flow through `signum()` as though it were an identity function, for the benefit of testing flow through optional chaining (`x?.`).
row = ";Int;true;signum();;;Argument[-1];ReturnValue;value"
row =
[
// model to allow data flow through `signum()` as though it were an identity function, for the benefit of testing flow through optional chaining (`x?.`).
";Int;true;signum();;;Argument[-1];ReturnValue;value",
// test Enum content in MAD
";;false;mkMyEnum2(_:);;;Argument[0];ReturnValue.EnumElement[mySingle:0];value",
Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pondering whether the MAD syntax should contain the enum type as well, e.g. EnumElement[MyEnum,mySingle:0] here. In a way it would be more precise, but also more fiddly to specify. There isn't really much ambiguity anyway, because the type is effectively determined by the rest of the MAD signature - though QL performance may be somewhat affected by potential extra rows mid compute.

@MathiasVP do you have an opinion on this?

Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP Jul 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dataflow library actually does something like this internally for performance reasons. I'd prefer not including the type as part of the syntax since it increases the chances of us getting a row wrong when we write MaD models 😂

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're on the top of syntax here, do you think the name EnumElement[...] is best, or perhaps EnumField[...], EnumValue[...] or EnumContent[...]. The documentation appears to call them "Associated Values" (where "Enumeration Values" mean the element name itself).

Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not sure why we shouldn't just support this via the same syntax we use for fields. Is it really important whether it's a field or an enumeration value?

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fields have names, enum associated values do not. In a way they're perhaps closer to a tuple (which we don't have MaD syntax for as yet).

Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP Jul 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean. I was thinking of examples like:

enum Season {
  case spring, summer, autumn, winter
}

and considered whether we could just write .spring for this enum value. But we do indeed need special syntax to handle something like:

enum MyEnum { case mySingle(Int) }

My vote is on EnumElement since that matches what we're doing for ArrayContent.

";;false;mkOptional2(_:);;;Argument[0];ReturnValue.OptionalSome;value"
]
}
}

Expand Down
684 changes: 374 additions & 310 deletions swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected
View file
Open in desktop

Large diffs are not rendered by default.

75 changes: 53 additions & 22 deletions swift/ql/test/library-tests/dataflow/dataflow/test.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,11 @@ indirect enum MyEnum {
case myCons(Int, MyEnum)
}

func mkMyEnum1(_ v: Int) -> MyEnum { return MyEnum.mySingle(v) }
func mkMyEnum2(_ v: Int) -> MyEnum { return MyEnum.myNone } // modelled flow
func mkOptional1(_ v: Int) -> Int? { return Optional.some(v) }
func mkOptional2(_ v: Int) -> Int? { return nil } // modelled flow

func testEnums() {
var a : MyEnum = .myNone

Expand Down Expand Up @@ -401,7 +406,7 @@ func testEnums() {
case .myNone:
()
case .mySingle(let a):
sink(arg: a) // $ flow=398
sink(arg: a) // $ flow=403
case .myPair(let a, let b):
sink(arg: a)
sink(arg: b)
Expand All @@ -410,7 +415,7 @@ func testEnums() {
}

if case .mySingle(let x) = a {
sink(arg: x) // $ flow=398
sink(arg: x) // $ flow=403
}
if case .myPair(let x, let y) = a {
sink(arg: x)
Expand All @@ -426,7 +431,7 @@ func testEnums() {
sink(arg: a)
case .myPair(let a, let b):
sink(arg: a)
sink(arg: b) // $ flow=420
sink(arg: b) // $ flow=425
case let .myCons(a, _):
sink(arg: a)
}
Expand All @@ -436,7 +441,7 @@ func testEnums() {
}
if case .myPair(let x, let y) = a {
sink(arg: x)
sink(arg: y) // $ flow=420
sink(arg: y) // $ flow=425
}

let b: MyEnum = .myCons(42, a)
Expand All @@ -452,7 +457,7 @@ func testEnums() {
case let .myCons(a, .myPair(b, c)):
sink(arg: a)
sink(arg: b)
sink(arg: c) // $ flow=420
sink(arg: c) // $ flow=425
case let .myCons(a, _):
sink(arg: a)
}
Expand All @@ -461,23 +466,49 @@ func testEnums() {
sink(arg: x)
}
if case MyEnum.myPair(let x, let y) = .myPair(source(), 0) {
sink(arg: x) // $ flow=463
sink(arg: x) // $ flow=468
sink(arg: y)
}
if case let .myCons(_, .myPair(_, c)) = b {
sink(arg: c) // $ flow=420
sink(arg: c) // $ flow=425
}

switch (a, b) {
case let (.myPair(a, b), .myCons(c, .myPair(d, e))):
sink(arg: a)
sink(arg: b) // $ flow=420
sink(arg: b) // $ flow=425
sink(arg: c)
sink(arg: d)
sink(arg: e) // $ flow=420
sink(arg: e) // $ flow=425
default:
()
}

let c1 = MyEnum.mySingle(0)
let c2 = MyEnum.mySingle(source())
let c3 = mkMyEnum1(0)
let c4 = mkMyEnum1(source())
let c5 = mkMyEnum2(0)
let c6 = mkMyEnum2(source())
if case MyEnum.mySingle(let d1) = c1 { sink(arg: d1) }
if case MyEnum.mySingle(let d2) = c2 { sink(arg: d2) } // $ flow=488
if case MyEnum.mySingle(let d3) = c3 { sink(arg: d3) }
if case MyEnum.mySingle(let d4) = c4 { sink(arg: d4) } // $ flow=490
if case MyEnum.mySingle(let d5) = c5 { sink(arg: d5) }
if case MyEnum.mySingle(let d6) = c6 { sink(arg: d6) } // $ flow=492

let e1 = Optional.some(0)
let e2 = Optional.some(source())
let e3 = mkOptional1(0)
let e4 = mkOptional1(source())
let e5 = mkOptional2(0)
let e6 = mkOptional2(source())
sink(arg: e1!)
sink(arg: e2!) // $ flow=501
sink(arg: e3!)
sink(arg: e4!) // $ flow=503
sink(arg: e5!)
sink(arg: e6!) // $ flow=505
}

func source2() -> (Int, Int)? { return nil }
Expand Down Expand Up @@ -523,8 +554,8 @@ func testOptionalPropertyAccess(y: Int?) {
}

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

func sink(str: String) {}
Expand All @@ -541,13 +572,13 @@ class MyClass {
extension MyClass {
convenience init(contentsOfFile: String) {
self.init(s: source3())
sink(str: str) // $ flow=543
sink(str: str) // $ flow=574
}
}

func extensionInits(path: String) {
sink(str: MyClass(s: source3()).str) // $ flow=549
sink(str: MyClass(contentsOfFile: path).str) // $ flow=543
sink(str: MyClass(s: source3()).str) // $ flow=580
sink(str: MyClass(contentsOfFile: path).str) // $ flow=574
}

class InoutConstructorClass {
Expand All @@ -572,10 +603,10 @@ struct S {
func testKeyPath() {
let s = S(x: source())
let f = \S.x
sink(arg: s[keyPath: f]) // $ flow=573
sink(arg: s[keyPath: f]) // $ flow=604

let inferred : KeyPath<S, Int> = \.x
sink(arg: s[keyPath: inferred]) // $ flow=573
sink(arg: s[keyPath: inferred]) // $ flow=604
}

struct S2 {
Expand All @@ -590,13 +621,13 @@ func testNestedKeyPath() {
let s = S(x: source())
let s2 = S2(s: s)
let f = \S2.s.x
sink(arg: s2[keyPath: f]) // $ flow=590
sink(arg: s2[keyPath: f]) // $ flow=621
}

func testArrayKeyPath() {
let array = [source()]
let f = \[Int].[0]
sink(arg: array[keyPath: f]) // $ MISSING: flow=597
sink(arg: array[keyPath: f]) // $ MISSING: flow=628
}

struct S2_Optional {
Expand All @@ -611,7 +642,7 @@ func testOptionalKeyPath() {
let s = S(x: source())
let s2 = S2_Optional(s: s)
let f = \S2_Optional.s?.x
sink(opt: s2[keyPath: f]) // $ MISSING: flow=611
sink(opt: s2[keyPath: f]) // $ MISSING: flow=642
}

func testSwap() {
Expand All @@ -623,11 +654,11 @@ func testSwap() {
x = y
y = t
sink(arg: x)
sink(arg: y) // $ flow=618
sink(arg: y) // $ flow=649

x = source()
y = 0
swap(&x, &y)
sink(arg: x) // $ SPURIOUS: flow=628
sink(arg: y) // $ flow=628
sink(arg: x) // $ SPURIOUS: flow=659
sink(arg: y) // $ flow=659
}
5 changes: 5 additions & 0 deletions swift/ql/test/library-tests/dataflow/taint/core/Taint.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,10 @@ edges
| subscript.swift:14:15:14:23 | call to source2() | subscript.swift:14:15:14:26 | ...[...] |
| try.swift:9:17:9:24 | call to source() | try.swift:9:13:9:24 | try ... |
| try.swift:15:17:15:24 | call to source() | try.swift:15:12:15:24 | try! ... |
| try.swift:18:13:18:25 | try? ... [some:0] | try.swift:18:12:18:27 | ...! |
| try.swift:18:18:18:25 | call to source() | try.swift:18:12:18:27 | ...! |
| try.swift:18:18:18:25 | call to source() | try.swift:18:18:18:25 | call to source() [some:0] |
| try.swift:18:18:18:25 | call to source() [some:0] | try.swift:18:13:18:25 | try? ... [some:0] |
nodes
| file://:0:0:0:0 | .first | semmle.label | .first |
| file://:0:0:0:0 | .second | semmle.label | .second |
Expand Down Expand Up @@ -185,7 +188,9 @@ nodes
| try.swift:15:12:15:24 | try! ... | semmle.label | try! ... |
| try.swift:15:17:15:24 | call to source() | semmle.label | call to source() |
| try.swift:18:12:18:27 | ...! | semmle.label | ...! |
| try.swift:18:13:18:25 | try? ... [some:0] | semmle.label | try? ... [some:0] |
| try.swift:18:18:18:25 | call to source() | semmle.label | call to source() |
| try.swift:18:18:18:25 | call to source() [some:0] | semmle.label | call to source() [some:0] |
subpaths
| 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 |
| 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] |
Expand Down
Loading