Skip to content

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented May 9, 2022

This changes how type-tracking steps are generated through global access paths.

Previously type tracking steps through global access paths went through the globalRootPseudoNode:

graph LR;
   wp1[Write p1]
   wpx1[Write p1]
   wp2[Write p2]
   wp3[Write p3]
   rp1[Read p1]
   rpx1[Read p1]
   rp2[Read p2]
   rp3[Read p3]
   wp1-- store p1 -->globalRootPseudoNode;
   wpx1-- store p1 -->globalRootPseudoNode;
   wp2-- store p2 -->globalRootPseudoNode;
   wp3-- store p3 -->globalRootPseudoNode;
   globalRootPseudoNode -- load p1 --> rp1;
   globalRootPseudoNode -- load p1 --> rpx1;
   globalRootPseudoNode -- load p2 --> rp2;
   globalRootPseudoNode -- load p3 --> rp3;
Loading

This PR changes it to simply generate an edge for each corresponding write/read pair:

graph LR;
   wp1[Write p1]
   wpx1[Write p1]
   wp2[Write p2]
   wp3[Write p3]
   rp1[Read p1]
   rpx1[Read p1]
   rp2[Read p2]
   rp3[Read p3]
   wp1 --> rp1;
   wpx1 --> rp1;
   wp1 --> rpx1;
   wpx1 --> rpx1;
   wp2 --> rp2;
   wp3 --> rp3;
Loading

As hinted in the above diagram, this generates N^2 edges when there are N writes and N reads of the same property/access path, which is why we used the globalRootPseudoNode to begin with.

However, when type-tracking through the globalRootPseudoNode, the original layout is inefficient due to the huge number of infeasible edges leading out of that node. We end up scanning all outgoing edges and it's only when we join with SmallStep::append that the infeasiable edges are discarded. When there are many iterations of trackUseNode going through globalRootPseudoNode, the end up enumerating all these edge at every iteration.

We already imposed the restriction that all writes must occur in the same file, and I believe this prevents the N^2 edge problem from occurring in practice.

Evaluation looks good:

  • 30% improvement in closure-library and a minor win overall
  • We discover more flows since a type-tracker that's already inside a property can now track through global access paths
  • This results in about 1000 new call edges, 8 new sinks, and 2 new alerts.

@asgerf asgerf added the JS label May 9, 2022
@asgerf asgerf requested a review from a team as a code owner May 9, 2022 08:16
@asgerf asgerf added the no-change-note-required This PR does not need a change note label May 9, 2022
@erik-krogh erik-krogh self-assigned this May 9, 2022
Copy link
Contributor

@erik-krogh erik-krogh left a comment

Choose a reason for hiding this comment

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

🚀

Simple and brilliant 👍

@codeql-ci codeql-ci merged commit e099b94 into github:main May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JS no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants