Swift: add DataFlow::Content for arrays by rdmarsh2 · Pull Request #13741 · github/codeql

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.

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

+
+    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

+    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

+    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

+  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

          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

One small comment. We should probably also have a new DCA run to double check the impact of hiding InOutExprs from the main AST.

geoffw0

LGTM. Would be good to wait for a final 👍 from @MathiasVP as well.

MathiasVP · 2023-07-31T08:27:01Z

LGTM. Would be good to wait for a final 👍 from @MathiasVP as well.

Would really like to have #13741 (comment) addressed as well before we merge. And also another DCA run afterwards.

rdmarsh2 · 2023-07-31T20:12:45Z

DCA looks clean

geoffw0

I think this is ready to merge (I keep forgetting I don't have array content already on main...)

MathiasVP

LGTM!

Swift: add DataFlow::Content for arrays

42cc644

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

Robert Marsh 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

Comment thread swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll Outdated

Comment thread swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll Outdated

Comment thread swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll Outdated

Swift: only read ArrayContent from subscript keypaths

093c690

geoffw0 reviewed Jul 19, 2023

View reviewed changes

Robert Marsh 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

Comment thread swift/ql/lib/codeql/swift/dataflow/internal/DataFlowPrivate.qll Outdated

Robert Marsh 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

Merged

3 tasks

Robert Marsh added 3 commits July 27, 2023 19:18

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

c36ce78

Swift: update test expectations after hiding InOutExpr

7ed520a

Swift: autoformat

6039af0

MathiasVP reviewed Jul 28, 2023

View reviewed changes

Comment thread swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImpl.qll Outdated

Robert Marsh added 2 commits July 28, 2023 20:41

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

bb4fe20

Swift: accept more test changes from hiding InOutExpr

22ae430

geoffw0 approved these changes Jul 31, 2023

View reviewed changes

geoffw0 approved these changes Aug 2, 2023

View reviewed changes

MathiasVP approved these changes Aug 2, 2023

View reviewed changes

MathiasVP merged commit 89aa86a into github:main Aug 2, 2023

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

Conversation

rdmarsh2 commented Jul 13, 2023

Uh oh!

rdmarsh2 commented Jul 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MathiasVP commented Jul 18, 2023

Uh oh!

MathiasVP commented Jul 18, 2023

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MathiasVP Jul 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geoffw0 commented Jul 19, 2023

Uh oh!

geoffw0 commented Jul 20, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

MathiasVP commented Jul 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rdmarsh2 commented Jul 31, 2023

Uh oh!

geoffw0 left a comment

Choose a reason for hiding this comment

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MathiasVP Jul 22, 2023 •

edited

Loading

MathiasVP commented Jul 31, 2023 •

edited

Loading