Skip to content

JS: Add helper library for call graph exploration #2886

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
May 14, 2020

Conversation

asgerf
Copy link
Contributor

@asgerf asgerf commented Feb 20, 2020

Exploring the call graph leading to or from a given function should be easier out-of-the-box than it currently is.

This adds a library semmle.javascript.explore.CallGraph which can be imported to help define a path query that looks for specific call paths. For example:

/**
 * @kind path-problem
 */

import javascript
import semmle.javascript.explore.CallGraph
import DataFlow

from InvokeNode invoke, FunctionNode target
where callEdge*(invoke, target)
  and isStartOfCallPath(invoke)
  and target.getName() = "myFunction"
select invoke, invoke, target, "Call path to myFunction"

There are two other work items I'd like to do, but first I'd like hear what people think of the idea:

  • Move semmle.javascript.dataflow.ForwardExploration and semmle.javascript.dataflow.BackwardExploration into semmle.javascript.explore
  • Add a help page about exploratory queries, explaining how to use these libraries.

@asgerf asgerf added JS WIP This is a work-in-progress, do not merge yet! labels Feb 20, 2020
@asgerf asgerf requested a review from a team as a code owner February 20, 2020 11:57
*
* There are edges from calls to their callees,
* and from functions to their contained calls and in some cases
* their inner functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

That last bit is interesting; why do we want those edges?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To handle callbacks passed to forEach where we don't see the exact call site.

So in a sense, a call edge A -> B can be read as A causes B to be called.

Even when it's not technically called in the same stack frame (e.g. through addEventListener) I still found this to be useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that makes sense. Could you expand the documentation slightly to explain this?

Also, I wonder whether it would make sense to distinguish the two kinds of edges by giving them different labels?

Copy link
Contributor

@esbena esbena left a comment

Choose a reason for hiding this comment

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

LGTM.

The move-proposal also sounds good.

Other content for .explore (lets track this elsewhere when this PR gets merged):

  • the heuristics module
  • a similar one-liner for getting a graph for type/configuration tracking to/from a specific node
  • a one liner for seeing the most important Configuration member predicate nodes. I.e. an alert at each sink, source, sanitizer...

}

/**
* Holds if `invoke` should be used as the starting point of a call path.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Holds if `invoke` should be used as the starting point of a call path.
* Holds if `fun` should be used as the ending point of a call path.

@asgerf asgerf force-pushed the js/call-graph-exploration branch from 14de234 to 8ad451e Compare March 7, 2020 15:28
@asgerf asgerf force-pushed the js/call-graph-exploration branch from 8ad451e to c04ba91 Compare April 21, 2020 09:52
@asgerf asgerf removed the WIP This is a work-in-progress, do not merge yet! label Apr 21, 2020
@asgerf
Copy link
Contributor Author

asgerf commented Apr 21, 2020

Rebased and moved the change note to 1.25. I found myself needing this the other day so I'd like it if we can try and get it merged. Writing the help page turned out to be more time-consuming than expected, so I'd like to merge without that.

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.

Could we remove the aliases to forward/backwards exploration in seemle.javascript.dataflow?
I know it is a breaking change, but the files explicitly states that they were only meant for debugging.

The rest LGTM, so I'm approving, and leaving it to you if you want to keep the aliases.

(I tried out GitHub's online conflict resolution, that's how I made a merge commit on your branch).

@asgerf
Copy link
Contributor Author

asgerf commented May 14, 2020

Could we remove the aliases to forward/backwards exploration in seemle.javascript.dataflow

It's usually a good idea to have a grace period where both solutions are usable, so I'd like to have both for one release.

@asgerf
Copy link
Contributor Author

asgerf commented May 14, 2020

@esbena you're still requesting changes. Any more requests?

@semmle-qlci semmle-qlci merged commit 57f44c5 into github:master May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants