Move `FlowSummaryImpl.qll` to `dataflow` pack by hvitved · Pull Request #14573 · github/codeql

hvitved · 2023-10-24T08:41:08Z

This PR turns FlowSummaryImpl.qll (and AccessPathSyntax.qll) into a proper parameterized module, and moves it to the shared dataflow pack.

Previously, the exposed SummarizedCallable class would look like

abstract class SummarizedCallable extends SummarizedCallableBase {
  predicate propagatesFlow(
    SummaryComponentStack input, SummaryComponentStack output, boolean preservesValue
  ) {
    none()
  }

  predicate propagatesFlowExt(
    string input, string output, boolean preservesValue
  ) {
    none()
  }
}

but after this PR it is simply

abstract class SummarizedCallable extends SummarizedCallableBase {
  abstract predicate propagatesFlow(
    string input, string output, boolean preservesValue
  );
}

That is, we now consider SummaryComponentStack an internal implementation detail, which is not exposed to the user (the user-facing aliases have been deprecated).

Commit-by-commit review is encouraged, and for each of the <LANG>: Use FlowSummaryImpl from dataflow pack commits, I recommend viewing the FlowSummaryImpl.qll in full, as the diff is not really meaningful (previously, it was the synced copy of share library, now it contains the inputs to the shared library).

+      /**
+       * A query predicate for outputting flow summaries in semi-colon separated format in QL tests.
+       * The syntax is: "namespace;type;overrides;name;signature;ext;inputspec;outputspec;kind;provenance",
+       * ext is hardcoded to empty.
+       */


+          sinkElement(_, spec, _, _)
+        }
+
+        private module AccessPath = AccessPathSyntax::AccessPath<sourceSinkSpec/1>;


+module SourceSinkInterpretationInput implements
+  Impl::Private::External::SourceSinkInterpretationInputSig
+{
+  private import csharp as Cs


erik-krogh

JS 👍

MathiasVP

Swift 👍

RasmusWL

Overall LGTM, but there are 2 things I would like to get fixed for Python 🙏

michaelnebel · 2023-12-12T13:54:34Z

+  )
+}
+
+private class SummarizedCallableAdapter extends SummarizedCallable {


Maybe a comment that this is where the MaD rows are being used to create SummarizedCallable's (the same for neutrals as well).

michaelnebel

C#: Looks good to me!
Thank you for doing this @hvitved !

RasmusWL

Python 👍 (thanks for the updates)

yoff

I can see this has already been approved by Python. I can also see that I have a pending comment, but I do not remember what it is, so I will just submit the partial review now, to see if it is still relevant.

aschackmull · 2023-12-13T13:27:26Z

I think I might see a bug related to the MaD provenance column. Could you verify that the presence of both a manual and a generated model for the same callable yields only the manual summary? I believe this used to work, but I think it might be broken now. Alright, I no longer think there's a bug, instead I think that most of the logic in SummarizedCallableExternal can be deleted as it appears to be useless (and that's what threw me off in the first place).

owen-mc · 2023-12-13T13:50:42Z

@aschackmull Do we have a test for what happens when we have a manual model and a generated model?

hvitved · 2023-12-13T13:55:23Z

Do we have a test for what happens when we have a manual model and a generated model?

There is at least https://github.com/github/codeql/blob/main/java/ql/src/Metrics/Summaries/GeneratedVsManualCoverage.ql.

michaelnebel · 2023-12-14T08:13:26Z

@aschackmull Do we have a test for what happens when we have a manual model and a generated model?

I don't think there exists such a test for Java.
But one can use the summary/1 predicate for creating such a test. (cc @owen-mc)

aschackmull

LGTM! Thanks for tackling this!

github-actions Bot added documentation Ruby DataFlow Library labels Oct 24, 2023

github-advanced-security AI found potential problems Oct 24, 2023

View reviewed changes

Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Fixed

Comment thread ruby/ql/lib/codeql/ruby/typetracking/TypeTrackerSpecific.qll Fixed

github-advanced-security AI found potential problems Oct 24, 2023

View reviewed changes

hvitved force-pushed the flow-summary-impl-param branch from 741725b to 45b59c1 Compare October 24, 2023 12:30

github-actions Bot added JS Python labels Oct 24, 2023

hvitved force-pushed the flow-summary-impl-param branch from 45b59c1 to 6185feb Compare October 24, 2023 18:54

github-actions Bot added the C# label Oct 24, 2023

github-advanced-security AI found potential problems Oct 24, 2023

View reviewed changes

hvitved force-pushed the flow-summary-impl-param branch from 6185feb to a047beb Compare October 30, 2023 08:33

github-advanced-security AI found potential problems Oct 30, 2023

View reviewed changes

hvitved force-pushed the flow-summary-impl-param branch 7 times, most recently from 94cefa4 to b2ee1f0 Compare November 27, 2023 11:45

github-actions Bot added the Go label Nov 27, 2023

github-advanced-security AI found potential problems Nov 27, 2023

View reviewed changes

Comment thread go/ql/lib/semmle/go/dataflow/ExternalFlow.qll Fixed

hvitved force-pushed the flow-summary-impl-param branch from b2ee1f0 to 5f79e7c Compare November 27, 2023 13:31

github-actions Bot added the Swift label Nov 27, 2023

github-advanced-security AI found potential problems Nov 27, 2023

View reviewed changes

Comment thread swift/ql/lib/codeql/swift/dataflow/ExternalFlow.qll Fixed

hvitved force-pushed the flow-summary-impl-param branch from 5f79e7c to c656fba Compare November 27, 2023 14:45

hvitved force-pushed the flow-summary-impl-param branch from c656fba to d73136b Compare December 7, 2023 10:43

github-actions Bot added the Java label Dec 7, 2023

hvitved force-pushed the flow-summary-impl-param branch 2 times, most recently from ae7e347 to fcb6c19 Compare December 7, 2023 12:59

hvitved added 2 commits December 10, 2023 11:25

Swift: Use FlowSummaryImpl from dataflow pack

2d3f96f

Java: Use FlowSummaryImpl from dataflow pack

f9dbf67

hvitved force-pushed the flow-summary-impl-param branch from fb1102c to f9dbf67 Compare December 10, 2023 10:26

erik-krogh previously approved these changes Dec 11, 2023

View reviewed changes

MathiasVP previously approved these changes Dec 11, 2023

View reviewed changes

Ruby: Cache more predicates

cdf59e1

hvitved dismissed stale reviews from MathiasVP and erik-krogh via cdf59e1 December 11, 2023 09:27

RasmusWL requested changes Dec 12, 2023

View reviewed changes

Comment thread python/ql/lib/semmle/python/dataflow/new/internal/DataFlowDispatch.qll Outdated

Comment thread python/ql/lib/semmle/python/frameworks/data/internal/ApiGraphModels.qll Outdated

Address review comments

a46964d

michaelnebel reviewed Dec 12, 2023

View reviewed changes

michaelnebel previously approved these changes Dec 12, 2023

View reviewed changes

RasmusWL previously approved these changes Dec 13, 2023

View reviewed changes

yoff reviewed Dec 13, 2023

View reviewed changes

Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Outdated

aschackmull reviewed Dec 13, 2023

View reviewed changes

Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Outdated

aschackmull reviewed Dec 13, 2023

View reviewed changes

Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Outdated

aschackmull reviewed Dec 13, 2023

View reviewed changes

Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll

aschackmull reviewed Dec 13, 2023

View reviewed changes

Comment thread shared/dataflow/codeql/dataflow/internal/FlowSummaryImpl.qll Outdated

aschackmull reviewed Dec 14, 2023

View reviewed changes

Comment thread shared/dataflow/codeql/dataflow/internal/AccessPathSyntax.qll Outdated

Address more review comments

098afb9

hvitved dismissed stale reviews from RasmusWL and michaelnebel via 098afb9 December 14, 2023 08:49

aschackmull approved these changes Dec 14, 2023

View reviewed changes

aibaars approved these changes Dec 14, 2023

View reviewed changes

hvitved merged commit c8b4a21 into github:main Dec 14, 2023

hvitved deleted the flow-summary-impl-param branch December 14, 2023 11:24

Conversation

hvitved commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Check warning

Uh oh!

Check warning

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erik-krogh left a comment

Choose a reason for hiding this comment

Uh oh!

MathiasVP left a comment

Choose a reason for hiding this comment

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michaelnebel Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aschackmull commented Dec 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owen-mc commented Dec 13, 2023

Uh oh!

hvitved commented Dec 13, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelnebel commented Dec 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aschackmull left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

hvitved commented Oct 24, 2023 •

edited

Loading

aschackmull commented Dec 13, 2023 •

edited

Loading

michaelnebel commented Dec 14, 2023 •

edited

Loading