JS: Precise data-flow for returns from async functions by erik-krogh · Pull Request #4019 · github/codeql

erik-krogh · 2020-08-06T13:40:32Z

Previously async function were modeled imprecisely, such that there was flow from foo to bar in the below.

async function f() {
  return foo;
}
let bar = f(); // f returns a promise that contains foo.

With this PR the precision is better.

The pseudo-properties from the Promise model is used the achieve this.

A new FunctionReturnNode is introduced, and this node has to be SourceNode because we need to store properties on it (the pseudo-properties).

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll

asgerf

A few comments so far. I gave up on commit-by-commit review after I noticed my comments becoming obsolete by later commits. Let me know when you want another round of reviews.

javascript/ql/src/semmle/javascript/dataflow/Nodes.qll

asgerf · 2020-08-07T11:52:51Z

javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll

+      function.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    }
+
+    override BasicBlock getBasicBlock() { result = function.(ExprOrStmt).getBasicBlock() }


This basic block belongs to the enclosing function. How about function.getExit().getBasicBlock()?

Getting the basic-block of the enclosing function is consistent with how ExceptionalFunctionReturnNode works.
If we change it, then we should change both.

I think the current behavior is fine.

Oof. I still think the current behavior is completely wrong, but I understand if you don't want to fix it in this PR. Opened https://github.com/github/codeql-javascript-team/issues/214.

javascript/ql/src/semmle/javascript/Promises.qll

javascript/ql/src/semmle/javascript/dataflow/Sources.qll

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll

javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

asgerf · 2020-08-07T13:25:02Z

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll

   */
  cached
  predicate returnStep(DataFlow::Node pred, DataFlow::Node succ) {
+    // Note: FlowSteps::CachedSteps::returnStep/2 has copy-paste children


This comment was quite confusing when doing commit-by-commit review since the function doesn't appear similar to this at all until later in history 😕

I think we can eliminate the duplication by introducing a shared predicate

pragma[inline] predicate returnStepWithAsyncFlag(DataFlow::Node pred, DataFlow::Node succ, boolean isAsync) { // ... bind isAsync to the async-ness of the function being returned from ... }

and then call that from returnStep/2:

returnStepWithAsyncFlag(pred, succ, false)

and from the Promises.qll library

returnStepWithAsyncFlag(pred, succ, true)

In the end the copy-paste was with the exception part of the predicate.
I've shared the implementation of that part.

There is still a slight but of almost copy-pasted code between the predicate and DataFlow::localFlowStep, but I think that is OK.

asgerf · 2020-08-07T13:26:50Z

javascript/ql/test/library-tests/TaintTracking/exceptions.js

      await throwAsync(source());
    } catch (e) {
-      sink(e); // NOT OK
+      sink(e); // NOT OK - but not flagged


Why do we miss this now? Are we missing a load step from the await operand to the catch?

I think it is because of the mix of calls and property store/read.

The flow is: flow into throwAsync() -> store in pseudo-property -> return from throwAsync() -> read pseudo-property -> local flow to e.

And the new code in loadStep/storeStep doesn't catch it, because they only handle ordinary returns and not exceptional returns.

erik-krogh · 2020-08-07T14:14:02Z

A few comments so far. I gave up on commit-by-commit review after I noticed my comments becoming obsolete by later commits.

I thought I had opened a draft PR. Maybe the PR got un-drafted when I accepted the review comments from intrigus-lgtm, or maybe I misclicked 🤷‍♂️.

I had meant to do a rebase before undrafting the PR, sorry about that.

Thanks for the comments.

erik-krogh · 2020-08-07T17:26:06Z

A quick smoke test looks good in terms of performance.

@asgerf ready for next round.
I've rebased the entire PR (and included your review comments).
Everything after update expected output is new.

erik-krogh · 2020-08-10T07:00:09Z

Evaluation on nightly and big-apps show good performance, except for gecko-dev that timed out on TargetBlank.ql.

I'll investigate the difference in results.

erik-krogh · 2020-08-10T07:37:50Z

I'll investigate the difference in results.

The new results look like TPs.

The missed result is for the same reason as this.

asgerf

Thanks, mostly LGTM now but we'll obviously need to fix gecko.

Also, do you see a way to recover the lost result? That was a really good result IMO and I'd be sad to lose it.

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll

asgerf · 2020-08-10T08:16:04Z

javascript/ql/src/semmle/javascript/dataflow/DataFlow.qll

+      function.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
+    }
+
+    override BasicBlock getBasicBlock() { result = function.(ExprOrStmt).getBasicBlock() }


Oof. I still think the current behavior is completely wrong, but I understand if you don't want to fix it in this PR. Opened https://github.com/github/codeql-javascript-team/issues/214.

max-schaefer · 2020-08-10T08:38:40Z

except for gecko-dev that timed out on TargetBlank.ql.

FWIW, gecko-dev has been hovering at the edge of timing out for me for a while now. In particular I've seen it time out on master on a few occasions (cf #3980).

erik-krogh · 2020-08-10T11:38:39Z

Also, do you see a way to recover the lost result? That was a really good result IMO and I'd be sad to lose it.

I managed to recover both the missed result from the evaluation and our test-case by adding a summary step for exceptions in immediately awaited async function calls.

erik-krogh · 2020-08-11T13:42:28Z

I managed to recover both the missed result from the evaluation and our test-case by adding a summary step for exceptions in immediately awaited async function calls.

An evaluation still looks good.

asgerf

Final nit, then LGTM

javascript/ql/src/semmle/javascript/dataflow/Configuration.qll

Co-authored-by: Asger F <asgerf@github.com>

erik-krogh · 2020-08-13T08:10:35Z

I did a final round of evaluations: nightly and big-apps.

gecko-dev took equally long to time out both with and without my changes.

@asgerf can I get a final approve?

asgerf

Sorry, this fell off my radar for a bit. LGTM 👍

erik-krogh added JS WIP This is a work-in-progress, do not merge yet! labels Aug 6, 2020

erik-krogh requested a review from a team as a code owner August 6, 2020 13:40

intrigus-lgtm reviewed Aug 6, 2020

View reviewed changes

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll Outdated Show resolved Hide resolved

javascript/ql/src/semmle/javascript/dataflow/internal/FlowSteps.qll Outdated Show resolved Hide resolved

erik-krogh added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Aug 7, 2020

asgerf reviewed Aug 7, 2020

View reviewed changes

erik-krogh added 8 commits August 7, 2020 17:32

introduce and use FunctionReturnNode

0004c28

remove imprecise return-flow from async functions

cc94c5e

add precise return-flow for async functions

26ef2f3

improve precision for load/store steps with async functions

0edb46c

update existing tests to work with FunctionReturnNode

b9a98f5

update expected output

8f06e96

correct copy-paste note after refactorings

94cf3a8

share implementation instead of copy-pasting

54fd7d9

erik-krogh force-pushed the asyncCalls branch from 13495e1 to 54fd7d9 Compare August 7, 2020 16:08

erik-krogh removed the WIP This is a work-in-progress, do not merge yet! label Aug 7, 2020

deduplicate some implementation in storeStep and loadStep

2680afc

erik-krogh force-pushed the asyncCalls branch from 5f7f406 to 2680afc Compare August 7, 2020 17:16

erik-krogh added 2 commits August 8, 2020 21:20

autoformat

244052f

update expected output

30dc77e

asgerf reviewed Aug 10, 2020

View reviewed changes

summarize exceptions thrown by immidiatly awaited function calls

9bcac10

erik-krogh force-pushed the asyncCalls branch from db628bc to 9bcac10 Compare August 10, 2020 11:31

fill in docstring

3477857

erik-krogh removed the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label Aug 11, 2020

asgerf reviewed Aug 11, 2020

View reviewed changes

javascript/ql/src/semmle/javascript/dataflow/Configuration.qll Outdated Show resolved Hide resolved

fix typo

e1ecc46

Co-authored-by: Asger F <asgerf@github.com>

adityasharad changed the base branch from master to main August 14, 2020 18:33

Merge branch 'main' into asyncCalls

65a1769

asgerf approved these changes Aug 24, 2020

View reviewed changes

codeql-ci merged commit 765c40e into github:main Aug 24, 2020

erik-krogh mentioned this pull request Aug 25, 2020

JS: Add data-flow from yield in generators. #4134

Merged

Conversation

erik-krogh commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

asgerf left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

erik-krogh commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erik-krogh commented Aug 7, 2020

Uh oh!

erik-krogh commented Aug 10, 2020

Uh oh!

erik-krogh commented Aug 10, 2020

Uh oh!

asgerf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-schaefer commented Aug 10, 2020

Uh oh!

erik-krogh commented Aug 10, 2020

Uh oh!

erik-krogh commented Aug 11, 2020

Uh oh!

asgerf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erik-krogh commented Aug 13, 2020

Uh oh!

asgerf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

erik-krogh commented Aug 6, 2020 •

edited

Loading

erik-krogh commented Aug 7, 2020 •

edited

Loading