[Feature branch] JS: Migrate to shared dataflow library #14412

asgerf · 2023-10-09T12:39:22Z

Targeting a feature branch

This PR is targeting a branch named js/shared-dataflow-branch, not main as we normally would.

Merging into main will happen at a later point, and will require that
- we have a good-looking DCA report (currently we have too many performance regressions), and
- possibly more evaluations on different suites and/or a QA trial run, and
- TODO comments have been resolved
The criteria for merging to js/shared-dataflow-branch should be that the code has been reviewed, and is considered good enough to go into main once the above criteria are met.
This means things like readability should be reviewed as if we were targeting main. We don't want to have to go back a re-review the code for readability later.

Review order

Commits have been organised into a reading order. The large number of commits is there to make it easier to the review.

Note that not every commit can compile in isolation. I split some commits into smaller chunks for easy of readability, but you may sometimes see odd artifacts like see a trailing and/or, an empty predicate body, or the occasional mention of a class that doesn't exist yet.

Otherwise the overarching order of commits are:

The first commit is from this PR, which should be reviewed in its own PR.
Core data flow changes: instantiating shared libraries and related work to enable that
Library modeling changes: mainly rewriting collection models as flow summaries
Query changes: porting to ConfigSig-style and updating tests
Library tests: port test configurations to ConfigSig style, generic test output changes
Add TODOs and further attempts at fixing some of the performance and precision issues.

Steps across function boundaries

The most difficult change for JS is that steps across function boundaries are no longer permitted, except for jump steps which can result in FPs and performance issues.

This means the following type of steps need to be rewritten:

Steps into or out of callbacks. This applies equally to value-preversing steps, taint steps, read steps, and store steps.
Store steps that target getALocalSource(), because that can cross function boundaries.
Steps that use the combined loadStoreStep feature, which is no longer supported.

The current status of this migration is:

Ported the core models for Array, Map, Set, Promise, and generators.
The above took care of all our existing uses of loadStoreStep.
String#replace has been converted, as it is the only string-related step that needed to be ported.
Value steps and taint steps from other models are implicitly converted to jump steps when they cross a function boundary.
Read/store steps from other models are just retained as they are for now, which in some case breaks the invariant by crossing a function boundary, as there is no combined jump-read or jump-store step we can map to.

Evaluation

The latest evaluation can be found here. As mentioned there are too many performance regressions at the moment, and alerts need to be triaged.

In preparation for migrating to the FlowSummary module in the qlpack, rename predicates to be consistent with the qlpack.

….qll

This is identical to the code in Ruby and seems to prevent a bad join ordering in a cached version of this predicate in DataFlowCommon

Only includes the changes that purely contain the new provenance columns

New library gets FN for spread arguments in a call to splice(), which was added to the old version in this PR: github#16739

These initially got messed up by a merge conflict where I couldn't rerun the tests due to breaking changes in the data flow library. I wanted the breaking-change updates to live in their own commits, not just eaten by a merge resolution commit, so the test output became broken for a while. The '#select' result set is unchanged in all of these, so they should be safe to accept.

asgerf · 2024-06-28T13:30:46Z

@erik-krogh after keeping up the CI changes and breaking changes in the data flow library, this PR is finally green again (apart from the QLDoc check which complains about shared libraries).

I'd like to merge it to the feature branch as it is now, so we can start working with smaller PRs. The first unreviewed commit so far is 102ca77.

erik-krogh

Looks ready for a feature branch 👍

erik-krogh · 2024-07-08T08:05:36Z

javascript/ql/lib/semmle/javascript/dataflow/internal/DataFlowPrivate.qll

+  int totalorder() {
+    result = TotalOrdering::astNodeId(this.asSourceCallable()).bitShiftLeft(1)
+    or
+    result = TotalOrdering::libraryCallableId(this.asLibraryCallable()).bitShiftLeft(1) + 1
+  }


Why the left-shift?

So the IDs for AST nodes and library callables can't clash, because the low bit is zero for AST nodes and 1 for library callables.

erik-krogh · 2024-07-08T08:36:35Z

javascript/ql/test/library-tests/Arrays/DataFlow.expected

@@ -1,11 +1,11 @@
 legacyDataFlowDifference
+| arrays.js:2:16:2:23 | "source" | arrays.js:39:8:39:24 | arr4_spread.pop() | only flow with OLD data flow library |


Are you planning to add this step to the new library?

I'm planning to fix this missing flow by adding general handling of the spread operator, so individual models don't have to specify how to handle spread operators. For example if a model specifies Argument[1] somewhere, and it applies to call that has a spread argument at position 0 or 1, then it should Just Work. The model shouldn't have to specify something like Argument[spread] which is what the legacy model currently does.

asgerf · 2024-08-02T10:48:13Z

Merging to the feature branch (not main)

github-actions bot added JS DataFlow Library ATM labels Oct 9, 2023

asgerf force-pushed the js/shared-dataflow branch from 9e3285e to e4bc193 Compare October 10, 2023 09:09

asgerf removed the ATM label Oct 10, 2023

asgerf force-pushed the js/shared-dataflow branch from e4bc193 to e29aa29 Compare October 10, 2023 12:41

github-actions bot added the ATM label Oct 10, 2023

asgerf force-pushed the js/shared-dataflow branch 2 times, most recently from 1457ea5 to a0202fd Compare October 11, 2023 08:18

github-actions bot added C# C++ documentation Java Python Go Ruby QL-for-QL Swift labels Oct 11, 2023

asgerf force-pushed the js/shared-dataflow branch 3 times, most recently from 0e5e149 to 70d675b Compare October 11, 2023 11:12

github-actions bot removed C# C++ documentation Java Python Go Ruby QL-for-QL Swift labels Oct 11, 2023

asgerf added 14 commits June 25, 2024 13:30

Rename predicates to be consistent with qlpack

6e32f27

In preparation for migrating to the FlowSummary module in the qlpack, rename predicates to be consistent with the qlpack.

Remove bindingsets

f0d7c3a

Instantiate shared FlowSummary library

dd7aff5

Migrate to shared FlowSummary library

6b35a76

Boilerplate implementation of default predicates from FlowSummaryImpl…

8c4e5e8

….qll

Port getMadStringFromContentSet -> encodeContent

5811a3c

Implement encodeReturn

b0ea812

Implement encodeWith/WithoutContent

6c0c67d

Handle AnyMemberDeep and ArrayElementDeep in encodeContent

3bebd70

Implement decodeUnknownArgument/ParameterPosition

e67e89d

Remove unused code

fc7c2c5

Avoid bad join in compatibleTypesCached

88edc06

This is identical to the code in Ruby and seems to prevent a bad join ordering in a cached version of this predicate in DataFlowCommon

JS: Update some tests with provenance columns

53efb58

Only includes the changes that purely contain the new provenance columns

Accept flow difference due to added test cases

af7b4e3

New library gets FN for spread arguments in a call to splice(), which was added to the old version in this PR: github#16739

asgerf force-pushed the js/shared-dataflow branch from 233b791 to af7b4e3 Compare June 26, 2024 11:53

asgerf added 11 commits June 27, 2024 09:06

JS: Benign test output changes

2473274

Fix unknown Parameter/Argument decoding

e53c0cd

JS: Provide RenderSummarizedCallable

c52a4b0

Ensure Member tokens from flow summaries are seen in PropertyName

df0488a

JS: Another provanance test output update

ee10702

JS: Benign update after fixing PropertyName charpred

90f0e07

JS: Fix compilation error in DataFlowImplConsistency.qll

1c730bc

Update DataFlowConsistency.expected

14fc790

JS: Another messy test update

e5924c1

JS: Fix missing qldoc

1d267ef

erik-krogh approved these changes Jul 8, 2024

View reviewed changes

asgerf merged commit 5c222f7 into github:js/shared-dataflow-branch Aug 2, 2024
12 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature branch] JS: Migrate to shared dataflow library #14412

[Feature branch] JS: Migrate to shared dataflow library #14412

asgerf commented Oct 9, 2023 •

edited

Loading

asgerf commented Jun 28, 2024

erik-krogh left a comment

erik-krogh Jul 8, 2024

asgerf Aug 1, 2024

erik-krogh Jul 8, 2024

asgerf Aug 1, 2024

asgerf commented Aug 2, 2024

		@@ -1,11 +1,11 @@
		legacyDataFlowDifference
		\| arrays.js:2:16:2:23 \| "source" \| arrays.js:39:8:39:24 \| arr4_spread.pop() \| only flow with OLD data flow library \|

[Feature branch] JS: Migrate to shared dataflow library #14412

[Feature branch] JS: Migrate to shared dataflow library #14412

Conversation

asgerf commented Oct 9, 2023 • edited Loading

Targeting a feature branch

Review order

Steps across function boundaries

Related reading:

Evaluation

asgerf commented Jun 28, 2024

erik-krogh left a comment

Choose a reason for hiding this comment

erik-krogh Jul 8, 2024

Choose a reason for hiding this comment

asgerf Aug 1, 2024

Choose a reason for hiding this comment

erik-krogh Jul 8, 2024

Choose a reason for hiding this comment

asgerf Aug 1, 2024

Choose a reason for hiding this comment

asgerf commented Aug 2, 2024

asgerf commented Oct 9, 2023 •

edited

Loading