Skip to content
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

Jump to bottom

Docs: Update data flow documentation to the new API. #13743

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Docs: Update data flow documentation to the new API. #13743

wants to merge 3 commits into from

Conversation

aschackmull
Copy link
Contributor

This updates the pieces of documentation that I could find to the new data flow API.
I identified the places that needed updates, by grepping for the following list of words:

::Configuration
isSanitizer
AdditionalTaintStep
hasFlow
isBarrierGuard
DataFlow::PathNode
DataFlow::PathGraph

@aschackmull aschackmull added the ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. label Jul 14, 2023
github-code-scanning[bot]
github-code-scanning bot found potential problems Jul 14, 2023
View reviewed changes
docs/codeql/ql-training/query-examples/java/global-data-flow-java-1.ql Fixed Show fixed Hide fixed
docs/codeql/ql-training/query-examples/java/global-data-flow-java-1.ql Fixed Show fixed Hide fixed
docs/codeql/ql-training/query-examples/cpp/global-data-flow-cpp-1.ql Dismissed
class TaintedFormatConfig extends TaintTracking::Configuration {
TaintedFormatConfig() { this = "TaintedFormatConfig" }
module TaintedFormatConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { /* TBD */ }

Check warning

Code scanning / CodeQL

Dead code Warning

This code is never used, and it's not publicly exported.
docs/codeql/ql-training/query-examples/cpp/global-data-flow-cpp-1.ql
override predicate isSource(DataFlow::Node source) { /* TBD */ }

override predicate isSink(DataFlow::Node sink) { /* TBD */ }
predicate isSink(DataFlow::Node sink) { /* TBD */ }

Check warning

Code scanning / CodeQL

Dead code Warning

This code is never used, and it's not publicly exported.
docs/codeql/ql-training/query-examples/java/global-data-flow-java-1.ql
class TaintedOGNLConfig extends TaintTracking::Configuration {
TaintedOGNLConfig() { this = "TaintedOGNLConfig" }
module TaintedOGNLConfig implements DataFlow::ConfigSig {
predicate isSource(DataFlow::Node source) { /* TBD */ }

Check warning

Code scanning / CodeQL

Dead code Warning

This code is never used, and it's not publicly exported.
docs/codeql/ql-training/query-examples/java/global-data-flow-java-1.ql Fixed Show fixed Hide fixed
github-code-scanning[bot]
github-code-scanning bot found potential problems Jul 14, 2023
View reviewed changes
docs/codeql/ql-training/query-examples/java/global-data-flow-java-1.ql
override predicate isSource(DataFlow::Node source) { /* TBD */ }

override predicate isSink(DataFlow::Node sink) { /* TBD */ }
predicate isSink(DataFlow::Node sink) { /* TBD */ }

Check warning

Code scanning / CodeQL

Dead code Warning

This code is never used, and it's not publicly exported.
@mattpollard
Copy link
Contributor

👋🏻 @felicitymay, I'll remove this from the Docs Content FR project as it's now in y'all's project for triage. Let me know if I should handle this differently.

subatoi
subatoi previously approved these changes Jul 17, 2023
View reviewed changes
Copy link
Contributor

@subatoi subatoi left a comment

Choose a reason for hiding this comment

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

Just had one comment/question from Docs, thank you!

docs/codeql/codeql-language-guides/analyzing-data-flow-in-cpp-new.rst Outdated
Similar to global data flow, the characteristic predicate ``MyTaintTrackingConfiguration()`` defines the unique name of the configuration, so ``"MyTaintTrackingConfiguration"`` should be replaced by the name of your class.

The taint tracking analysis is performed using the predicate ``hasFlow(DataFlow::Node source, DataFlow::Node sink)``.
The resulting module is completely similar to the one obtained from ``DataFlow::Global<ConfigSig>``.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an alternative way of expressing "completely similar", if that's not a technical term I'm unfamiliar with? Does this mean "exactly the same as" for example?

(There are six instances of "completely similar")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a technical term, so I'm open to suggestion for how to phrase this more clearly.
Details: When you apply either DataFlow::Global<..> or TaintTracking::Global<..> to your configuration then the result is a module containing a number of predicates, such as flow and flowPath. The key is that the predicates the resulting modules contain are the same in the two cases, in the sense that the output api is the same. So if you switch between DataFlow and TaintTracking then your subsequent references to the results need no changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation! How about(?):

Suggested change
The resulting module is completely similar to the one obtained from ``DataFlow::Global<ConfigSig>``.
The resulting module is equivalent to the one obtained from ``DataFlow::Global<ConfigSig>``.

I think this should imply that the modules are the same in the context in which they're intended to be used, but that the modules themselves are not necessarily identical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't sound right to me. To me "equivalent" is too strong. There's definitely a difference in the contents of the predicates in those modules as they are computing different flow graphs (taint tracking includes a lot more flow steps). It's just that the apis of the resulting modules are the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, perhaps this?

The resulting module has an identical signature to the one obtained from ``DataFlow::Global<ConfigSig>``.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds good!

Copy link
Contributor

Choose a reason for hiding this comment

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

How about identical interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That could work as well, although I prefer "signature" as that's the proper technical term (QL modules implement signatures).

geoffw0
geoffw0 previously approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@geoffw0 geoffw0 left a comment

Choose a reason for hiding this comment

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

LGTM (focussed on CPP, and I didn't check the examples compile).

Good to know we have partial flow support in new data flow.

@aschackmull aschackmull dismissed stale reviews from geoffw0 and subatoi via afc4657 July 19, 2023 07:14
@jketema
Copy link
Contributor

jketema commented Jul 20, 2023

One high-level comment: internally we standardised the names of our configurations to end in Config should we do the same here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rvermeulen rvermeulen rvermeulen left review comments

@subatoi subatoi subatoi approved these changes

@geoffw0 geoffw0 geoffw0 left review comments

Assignees
No one assigned
Labels
documentation ready-for-doc-review This PR requires and is ready for review from the GitHub docs team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aschackmull @mattpollard @jketema @rvermeulen @subatoi @geoffw0