Swift: add DataFlow::Content for arrays #13741

rdmarsh2 · 2023-07-13T20:19:02Z

No description provided.

rdmarsh2 · 2023-07-13T20:19:43Z

Some more checking to do about Array member functions before I'm ready to merge this, but it seems to work so far.

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

MathiasVP · 2023-07-18T15:56:19Z

Also this needs a DCA run. But probably not before the above fixes have been implemented.

MathiasVP · 2023-07-18T20:41:26Z

Oh, one more thing: We need to add ArrayContent to forceHighPrecision like Java does here: https://github.com/github/codeql/blob/main/java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll#L391C3-L391C28. This will prevent an access-path blowup on certain evil databases.

geoffw0

Sorry for not looking at this sooner.

This looks great. Thank you for including models-as-data support (ArrayElement), there are a number of models that should benefit from using this in the near future.

geoffw0 · 2023-07-19T10:06:24Z

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

+
+    var matrix = [[source()]]
+    sink(arg: matrix[0])
+    sink(arg: matrix[0][0]) // $ flow=645


Does assignment to a multi-dimensional array work as well?

var matrix2 = [[1]] matrix2[0][0] = source() sink(arg: matrix2[0][0])

No, it doesn't, and now that I look at the cases I based this off of I'm not sure we handle nested fields or tuples properly either...

Mmm. Then we should probably create an issue for this (in all content types) rather than trying to address it for just arrays here?

After adding some tests it seems to work for nested fields... I'm not sure why, though.

This is because of "reverse reads": if readStep(n2.(PostUpdateNode).getPreUpdateNode(), f, n1.(PostUpdateNode).getPreUpdateNode()) then the dataflow library generates storeStep(n1, f, n2). So if this doesn't work for array content then it's either because the post-update/pre-update isn't working, or because the readStep isn't working.

I think it's actually that there's a step through an InOutExpr between the store and read step:

# 650| getElement(11): [AssignExpr] ... = ... # 650| getDest(): [SubscriptExpr] ...[...] # 650| getBase(): [InOutExpr] &... # 650| getSubExpr(): [SubscriptExpr] ...[...] # 650| getBase(): [InOutExpr] &... # 650| getSubExpr(): [DeclRefExpr] matrix2

geoffw0 · 2023-07-19T10:08:23Z

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

+    var arr3 = [1]
+    var arr4 = arr2 + arr3
+    sink(arg: arr3[0])
+    sink(arg: arr4[0]) // $ MISSING: flow=642


Would a model of Array.+(_:_:) in ArraySummaries fix this case?

Yes, it should.

geoffw0 · 2023-07-19T10:09:28Z

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

+    sink(arg: arr4[0]) // $ MISSING: flow=642
+
+    var arr5 = Array(repeating: source(), count: 2)
+    sink(arg: arr5[0]) // $ MISSING: flow=654


Again it's possible a model of the initializer is all we need here. It can wait as follow-up.

geoffw0 · 2023-07-19T10:14:02Z

DCA shows a 6.6x analysis slowdown, which hopefully @MathiasVP s suggestion will fix. We can tolerate some slowdown for an improvement like this.

geoffw0 · 2023-07-20T09:32:02Z

New DCA run LGTM. Not a hint of performance issues. Good work!

MathiasVP · 2023-07-20T09:58:35Z

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

+  component.getName() = "ArrayElement" and
+  content instanceof Content::ArrayContent


Should this be pulled into its own predicate to follow the structure of parseField?

It's a considerably simpler case than parseField, I'd be happy either way TBH.

MathiasVP · 2023-07-26T07:09:09Z

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

@@ -111,7 +111,8 @@ private module Cached {
      hasExprNode(n,
        [
          any(Argument arg | modifiable(arg)).getExpr(), any(MemberRefExpr ref).getBase(),
-          any(ApplyExpr apply).getQualifier(), any(TupleElementExpr te).getSubExpr()
+          any(ApplyExpr apply).getQualifier(), any(TupleElementExpr te).getSubExpr(),
+          any(SubscriptExpr se).getBase(), any(InOutExpr ioe).getSubExpr()


Would a better fix maybe be to hide InOutExpr from the AST entirely?

Maybe - we might break some other things by doing that... I can try it though

MathiasVP · 2023-07-26T07:10:52Z

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

@@ -705,7 +709,7 @@ predicate storeStep(Node node1, ContentSet c, Node node2) {
    node1.asExpr() = assign.getSource() and
    node2.(PostUpdateNode).getPreUpdateNode().asExpr() = subscript.getBase() and
    subscript = assign.getDest() and
-    subscript.getBase().getType().(InOutType).getObjectType() instanceof ArrayType and
+//    subscript.getBase().getType().(InOutType).getObjectType() instanceof ArrayType and


Did you mean to comment this out? Based on 42cc644#r1267210752, I gathered that conjunct was important

Swift: add DataFlow::Content for arrays

42cc644

github-actions bot added the Swift label Jul 13, 2023

rdmarsh2 added 7 commits July 17, 2023 14:58

Swift: more ArrayContent tests

ef9376d

Swift: additional dataflow tests for arrays

0b35be2

Swift: support array keypath reads in dataflow

169326f

Swift: add ArrayContent summary support

d4b635d

Swift: fix tests for array concatenation

81a25b2

Swift: autoformat

dfa5e18

Swift: add qldoc for ArrayContent and Array models

1fac08e

rdmarsh2 marked this pull request as ready for review July 18, 2023 14:08

rdmarsh2 requested a review from a team as a code owner July 18, 2023 14:08

Swift: Change note for ArrayContent

bcc4565

github-actions bot added the documentation label Jul 18, 2023

MathiasVP reviewed Jul 18, 2023

View reviewed changes

Swift: only read ArrayContent from subscript keypaths

093c690

geoffw0 reviewed Jul 19, 2023

View reviewed changes

rdmarsh2 added 3 commits July 19, 2023 16:31

Swift: force high precision for arraycontent

883543f

Swift: add test for assignment to nested arrays

ec99977

Swift: fix cartesian product in array readStep

41ad1ed

MathiasVP reviewed Jul 20, 2023

View reviewed changes

Swift: reverse flow steps through InOutExpr

3587374

MathiasVP reviewed Jul 26, 2023

View reviewed changes

rdmarsh2 added 2 commits July 26, 2023 17:12

Swift: undo commented out line

e54341a

Swift: Hide InOutExpr

bf5ba37

geoffw0 mentioned this pull request Jul 27, 2023

Swift: Model withUnsafeBytes and similar closure methods #13827

Draft

3 tasks

Merge branch 'main' into rdmarsh2/swift/array-content-flow

c36ce78

Swift: add DataFlow::Content for arrays #13741

Swift: add DataFlow::Content for arrays #13741

rdmarsh2 commented Jul 13, 2023

rdmarsh2 commented Jul 13, 2023

MathiasVP commented Jul 18, 2023

MathiasVP commented Jul 18, 2023

geoffw0 left a comment

geoffw0 Jul 19, 2023

rdmarsh2 Jul 19, 2023

geoffw0 Jul 19, 2023

rdmarsh2 Jul 21, 2023

MathiasVP Jul 22, 2023 •

edited

rdmarsh2 Jul 25, 2023

geoffw0 Jul 19, 2023

rdmarsh2 Jul 21, 2023

geoffw0 Jul 19, 2023

rdmarsh2 Jul 21, 2023

geoffw0 commented Jul 19, 2023

geoffw0 commented Jul 20, 2023

MathiasVP Jul 20, 2023

geoffw0 Jul 21, 2023

MathiasVP Jul 26, 2023

rdmarsh2 Jul 26, 2023

MathiasVP Jul 26, 2023

rdmarsh2 Jul 26, 2023

		component.getName() = "ArrayElement" and
		content instanceof Content::ArrayContent

Swift: add DataFlow::Content for arrays #13741

Are you sure you want to change the base?

Swift: add DataFlow::Content for arrays #13741

Conversation

rdmarsh2 commented Jul 13, 2023

rdmarsh2 commented Jul 13, 2023

MathiasVP commented Jul 18, 2023

MathiasVP commented Jul 18, 2023

geoffw0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MathiasVP Jul 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoffw0 commented Jul 19, 2023

geoffw0 commented Jul 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MathiasVP Jul 22, 2023 •

edited