Add data flow debugging guide to CodeQL docs #6502

jf205 · 2021-08-18T10:40:48Z

This PR closes https://github.com/github/docs-content/issues/3585 by adding an article written by @aschackmull to the public CodeQL docs. I've made a few small changes to the text, but it's still pretty technical. I don't think that's a problem given the audience we are aiming this article at.

cc @RasmusWL

intrigus-lgtm

Having this documentation is very helpful and I now finally know why I should not use any() for debugging 🎉

intrigus-lgtm · 2021-08-18T12:22:04Z

docs/codeql/writing-codeql-queries/debugging-data-flow-queries-using-partial-flow.rst

+Checking sources and sinks
+--------------------------
+
+As a first step, make sure that the source and sink definitions contain what you expect. If one of these is empty then there can never be any data flow. The easiest way to verify this is using quick evaluation in CodeQL for VS Code: Select the text ``node instanceof MySource``, right-click, and choose "CodeQL: Quick Evaluation". This will evaluate the highlighted text, which in this case means the set of sources.


Is there a difference between selecting the text node instanceof MySource and selecting the text isSource?

Not really.

If you do a quick evaluation the isSource member-predicate, your output will contains 2 columns: this (which is the string that identifies the configuration), and node which has the relevant nodes that are sources. So you will end up with a bit more noise in the output 🤷

RasmusWL · 2021-08-18T12:50:07Z

A few points in regards to this that we should probably cover in the tutorial as well:

Since this article was written initially, hasPartialFlowRev has been added for debugging flow starting at a sink.

In regards to that, @aschackmull came with the following:

PSA re. using the partial flow features of the shared data flow library: It looks like the (somewhat recent) addition of being able to do reverse partial flow has the side effect that both forward partial flow and reverse partial flow is calculated whenever you request one of the two (they share the type PartialPathNode). So your best bet in terms of getting good performance is to set whichever one of isSource and isSink that's currently irrelevant to none().

hvitved · 2021-08-19T18:11:51Z

docs/codeql/writing-codeql-queries/codeql-queries.rst

@@ -25,3 +26,4 @@ CodeQL queries are used in code scanning analyses to find problems in source cod
 - :doc:`About data flow analysis <about-data-flow-analysis>`: Data flow analysis is used to compute the possible values that a variable can hold at various points in a program, determining how those values propagate through the program and where they are used. 
 - :doc:`Creating path queries <creating-path-queries>`: You can create path queries to visualize the flow of information through a codebase.
 - :doc:`Troubleshooting query performance <troubleshooting-query-performance>`: Improve the performance of your CodeQL queries by following a few simple guidelines.
+- :doc:`Debugging data-flow queries using partial flow <debugging-data-flow-queries-using-partial-flow>`: If a data-flow query unexpectedly produces no results, you can use partial flow to debug the problem.


Perhaps If a data-flow does not produce expected results instead of If a data-flow query unexpectedly produces no results, as it may also be relevant to debug if a query has results. (Same change in the other file).

hvitved · 2021-08-19T18:14:37Z

docs/codeql/writing-codeql-queries/debugging-data-flow-queries-using-partial-flow.rst

+    where config.hasPath(source, sink)
+    select sink, "Sink is reached from $@.", source.getNode(), "here"
+
+If a data-flow query that you have written does not produce any results when you expect it to, there may be a problem with your query.


does not produce any results when you expect it to -> does not produce expected results

hvitved · 2021-08-19T18:21:56Z

docs/codeql/writing-codeql-queries/debugging-data-flow-queries-using-partial-flow.rst

+
+A naive next step could be to try changing the sink definition to ``any()``. This would mean that we would get a lot of flow to all the places that are reachable from the sources. While this approach makes sense and can work, it can be hard to get an overview of the results as they can be quite plentiful, and it does come with a couple of additional caveats: Performance might degrade to uselessness and we might not even see all the partial flow paths. The latter point is somewhat subtle and deserves elaboration. Since the data-flow library tries very hard to prune impossible paths and since field stores and reads must be evenly matched along a path, then we will never see paths going through a store that fail to reach a corresponding read. This can make it hard to see where flow actually stops.
+
+Because of this, a ``Configuration`` comes with a mechanism for exploring partial flow that tries to deal with these caveats. This is the ``Configuration.hasPartialFlow`` predicate:


Perhaps add (there is also a `hasPartialFlowRev` predicate for exploring flow backwards from sinks).

intrigus-lgtm · 2021-08-23T11:09:27Z

docs/codeql/writing-codeql-queries/debugging-data-flow-queries-using-partial-flow.rst

@@ -56,9 +56,9 @@ If there are still no results and performance did not degrade to complete useles
 Partial flow
 ------------

-A naive next step could be to try changing the sink definition to ``any()``. This would mean that we would get a lot of flow to all the places that are reachable from the sources. While this approach makes sense and can work, it can be hard to get an overview of the results as they can be quite plentiful, and it does come with a couple of additional caveats: Performance might degrade to uselessness and we might not even see all the partial flow paths. The latter point is somewhat subtle and deserves elaboration. Since the data-flow library tries very hard to prune impossible paths and since field stores and reads must be evenly matched along a path, then we will never see paths going through a store that fail to reach a corresponding read. This can make it hard to see where flow actually stops.
+A naive next step could be to try changing the sink definition to ``any()``. This would mean that we would get a lot of flow to all the places that are reachable from the sources. While this approach makes sense and can work in some cases, you might find that it produces so many results that it's very hard to explore the findings, which can also dramtatically affect query performance. More importnatly, you might not even see all the partial flow paths. This is because the data-flow library tries very hard to prune impossible paths and, since field stores and reads must be evenly matched along a path, we will never see paths going through a store that fail to reach a corresponding read. This can make it hard to see where flow actually stops.


Typo: "importnatly"

Also typo: "dramtatically"

jf205 · 2021-08-24T13:05:55Z

Thanks for the reviews so far! I'm going to work on making the text a little easier to follow before asking for a review from the docs team.

jf205 · 2021-09-02T15:45:29Z

I've had a go at further polishing the text following the technical reviews. I think this is now ready for a review by the docs team. Note that this article is aimed at fairly advanced query writers, so i haven't simplified the text too much.

aschackmull · 2021-09-03T06:50:22Z

docs/codeql/writing-codeql-queries/debugging-data-flow-queries-using-partial-flow.rst

+Checking sources and sinks
+--------------------------
+
+Initially, you should make sure that the ``source`` and ``sink`` definitions contain what you expect. If either the ``source`` or ``sink`` is empty then there can never be any data flow. The easiest way to check this is using quick evaluation in CodeQL for VS Code. Select the text ``node instanceof MySource``, right-click, and choose "CodeQL: Quick Evaluation". This will evaluate the highlighted text, which in this case means the set of sources. For more information, see :ref:`Analyzing your projects <running-a-specific-part-of-a-query-or-library>` in the CodeQL for VS Code help.


The words source and sink are not referring directly to code, so I don't think they should be type-set specially.

This was changed throughout, and I think it should be reverted throughout.

aschackmull · 2021-09-03T06:51:45Z

docs/codeql/writing-codeql-queries/debugging-data-flow-queries-using-partial-flow.rst

+      )
+    }
+
+If you are focusing on a single ``source`` then the ``src`` column is meaningless. You may of course also add other columns of interest based on ``n``, but including the enclosing callable and the distance to the source at the very least is generally recommended, as they can be useful columns to sort on to better inspect the results.


This was rewritten from "superfluous" to "meaningless", which I don't think makes sense. It should be reverted to "superfluous".

mattpollard

👋🏻 @jf205, thanks for your patience here! Checked out the text and it looks good to me 🚢

The things that I mentioned in #6502 (comment) that never got into the document 😳

james added 3 commits August 18, 2021 11:26

add data flow debugging guide

babec9b

add new tutorial

ad2850d

tweak sojme text

429decd

jf205 requested a review from aschackmull August 18, 2021 10:41

github-actions bot added the documentation label Aug 18, 2021

james added 3 commits August 18, 2021 11:47

fix link

18b8244

correct article name

8443d34

add new article to toc

dcbf766

intrigus-lgtm reviewed Aug 18, 2021

View reviewed changes

jf205 requested review from hvitved and aschackmull and removed request for aschackmull August 19, 2021 08:00

hvitved reviewed Aug 19, 2021

View reviewed changes

james added 2 commits August 23, 2021 11:34

address review comments

dbf7487

address review comments

66bdbf4

intrigus-lgtm reviewed Aug 23, 2021

View reviewed changes

fix typos

1844071

hvitved previously approved these changes Aug 24, 2021

View reviewed changes

aschackmull previously approved these changes Aug 31, 2021

View reviewed changes

polish text

81a9ce2

jf205 dismissed stale reviews from aschackmull and hvitved via 81a9ce2 September 2, 2021 15:40

fix link

2e99583

jf205 added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Sep 2, 2021

jf205 closed this Sep 2, 2021

jf205 reopened this Sep 2, 2021

aschackmull reviewed Sep 3, 2021

View reviewed changes

revert a couple of changes

8c37e90

aschackmull approved these changes Sep 3, 2021

View reviewed changes

mattpollard approved these changes Sep 11, 2021

View reviewed changes

jf205 merged commit c86311e into main Sep 13, 2021

jf205 deleted the dataflow-tutorial branch September 13, 2021 09:25

shati-patel removed the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Sep 13, 2021

RasmusWL added a commit that referenced this pull request Feb 25, 2022

Docs: Mention hasPartialFlowRev and performance problem

49dbb8c

The things that I mentioned in #6502 (comment) that never got into the document 😳

RasmusWL mentioned this pull request Feb 25, 2022

Docs: Mention hasPartialFlowRev and performance problem #8252

Merged

hmac pushed a commit that referenced this pull request Mar 20, 2022

Docs: Mention hasPartialFlowRev and performance problem

57a48b8

The things that I mentioned in #6502 (comment) that never got into the document 😳

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add data flow debugging guide to CodeQL docs #6502

Add data flow debugging guide to CodeQL docs #6502

Uh oh!

jf205 commented Aug 18, 2021 •

edited by mattpollard

Loading

Uh oh!

intrigus-lgtm left a comment

Uh oh!

intrigus-lgtm Aug 18, 2021

Uh oh!

RasmusWL Aug 18, 2021

Uh oh!

RasmusWL commented Aug 18, 2021 •

edited

Loading

Uh oh!

hvitved Aug 19, 2021

Uh oh!

hvitved Aug 19, 2021

Uh oh!

hvitved Aug 19, 2021

Uh oh!

intrigus-lgtm Aug 23, 2021

Uh oh!

hvitved Aug 23, 2021

Uh oh!

jf205 commented Aug 24, 2021

Uh oh!

jf205 commented Sep 2, 2021

Uh oh!

aschackmull Sep 3, 2021

Uh oh!

aschackmull Sep 3, 2021

Uh oh!

aschackmull Sep 3, 2021

Uh oh!

mattpollard left a comment

Uh oh!

Uh oh!


		A naive next step could be to try changing the sink definition to ``any()``. This would mean that we would get a lot of flow to all the places that are reachable from the sources. While this approach makes sense and can work, it can be hard to get an overview of the results as they can be quite plentiful, and it does come with a couple of additional caveats: Performance might degrade to uselessness and we might not even see all the partial flow paths. The latter point is somewhat subtle and deserves elaboration. Since the data-flow library tries very hard to prune impossible paths and since field stores and reads must be evenly matched along a path, then we will never see paths going through a store that fail to reach a corresponding read. This can make it hard to see where flow actually stops.

		Because of this, a ``Configuration`` comes with a mechanism for exploring partial flow that tries to deal with these caveats. This is the ``Configuration.hasPartialFlow`` predicate:

Add data flow debugging guide to CodeQL docs #6502

Add data flow debugging guide to CodeQL docs #6502

Uh oh!

Conversation

jf205 commented Aug 18, 2021 • edited by mattpollard Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

intrigus-lgtm 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!

RasmusWL commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jf205 commented Aug 24, 2021

Uh oh!

jf205 commented Sep 2, 2021

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!

mattpollard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jf205 commented Aug 18, 2021 •

edited by mattpollard

Loading

RasmusWL commented Aug 18, 2021 •

edited

Loading