Java: Support for With[out]Element for MaD. #13546

michaelnebel · 2023-06-23T12:44:06Z

In this PR we introduce WithoutElement and WithElement for Java MaD.

Assume that c is a callable and assume that A and B are access paths and assume we have a summary that defines that data can flow from A to B via c:

c : A -> B

In case A and B targets collections this summary also implies that there is flow from the elements in the source collection to the elements in the target collection.
That is

c : A -> B       =>       c : A.Element -> B.Element

However, in some cases we want to avoid exactly this.
One example could be List.clear().
Using WithoutElement it is now possible to define

clear : A.WithoutElement -> B

If the list itself is tainted then the taint is propagated via clear, but not the element taint.
That is,

clear : A.WithoutElement -> B      \equiv      clear : A -> B and not clear : A.Element -> B.Element

The dual WithElement is identical to an already existing construct

c : A.WithElement -> B      \equiv      c : A.Element -> B.Element

For completeness also note that

c : A -> B.      \equiv.      c : A.WithoutElement -> B and c : A.WithElement -> B

github-actions · 2023-06-23T12:46:36Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,682,184,76,,9,,,17
+    Java Standard Library,``java.*``,3,683,184,76,,9,,,17
-    Totals,,283,12766,2040,286,16,122,33,1,390
+    Totals,,283,12767,2040,286,16,122,33,1,390

Changes to framework-coverage-java.csv:

- java.util,44,,484,,,,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,44,440
+ java.util,44,,485,,,,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,44,441

github-actions · 2023-07-04T08:55:49Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,683,197,76,,9,,,17
+    Java Standard Library,``java.*``,3,684,197,76,,9,,,17
-    Totals,,283,13593,2059,286,16,122,33,1,390
+    Totals,,283,13594,2059,286,16,122,33,1,390

Changes to framework-coverage-java.csv:

- java.util,44,,484,,,,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,44,440
+ java.util,44,,485,,,,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,44,441

github-actions · 2023-07-04T09:47:24Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,683,197,76,,9,,,17
+    Java Standard Library,``java.*``,3,684,197,76,,9,,,17
-    Totals,,283,13593,2059,286,16,122,33,1,390
+    Totals,,283,13594,2059,286,16,122,33,1,390

Changes to framework-coverage-java.csv:

- java.util,44,,484,,,,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,44,440
+ java.util,44,,485,,,,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,44,441

github-actions · 2023-07-04T11:31:14Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,683,197,76,,9,,,17
+    Java Standard Library,``java.*``,3,684,197,76,,9,,,17
-    Totals,,283,13593,2059,286,16,122,33,1,390
+    Totals,,283,13594,2059,286,16,122,33,1,390

Changes to framework-coverage-java.csv:

- java.util,44,,484,,,,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,44,440
+ java.util,44,,485,,,,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,44,441

…ted in collections/B.java).

…lement.

github-actions · 2023-08-01T10:56:42Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,689,205,80,,9,,,18
+    Java Standard Library,``java.*``,3,690,205,80,,9,,,18
-    Totals,,283,18451,2142,290,16,122,33,1,391
+    Totals,,283,18452,2142,290,16,122,33,1,391

Changes to framework-coverage-java.csv:

- java.util,45,,485,,,1,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,45,440
+ java.util,45,,486,,,1,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,45,441

jcogs33

I added a few minor questions, but otherwise looks reasonable to me:

Should there potentially be a change note for this since data extensions for Java is in beta?
For my own understanding: What is the benefit of including the .WithElement option if .Element -> .Element is identical?

docs/codeql/codeql-language-guides/customizing-library-models-for-java.rst

jcogs33 · 2023-08-02T02:47:47Z

java/ql/lib/ext/java.util.model.yml

      # When `WithoutElement` is implemented, these should be changed to summary models of the form `Argument[this].WithoutElement -> Argument[this]`.
      - ["java.util", "Collection", "removeIf", "(Predicate)", "summary", "manual"]
      - ["java.util", "Iterator", "remove", "()", "summary", "manual"]
-      - ["java.util", "List", "clear", "()", "summary", "manual"]
      - ["java.util", "List", "remove", "(Object)", "summary", "manual"]
      - ["java.util", "Map", "clear", "()", "summary", "manual"]
      - ["java.util", "Set", "clear", "()", "summary", "manual"]


Minor request that you can ignore if you want 🙂: Would it make sense to update the rest of these models to use WithoutElement on this PR?

With the current implementation we can't use WithoutElement and WithElement for anything else than collection content (not array content or dictionary content).
Would it maybe make more sense to include these as well for Java?

WithoutElement should still apply as-is to Set.clear(), right?

Yes, right - I will introduce that summary and make a test.

A question about remove[If] neutrals. Since we are not tracking individual elements for collections in Java, we will not be able to re-write these using WithElement and WithoutElement. Should we move these neutrals to their proper location in the data extension files or should we have a summary instead (is it a better assumption that there is element flow via eg. List.remove as it only might remove a single element)?

java/ql/lib/ext/java.util.model.yml

michaelnebel · 2023-08-04T11:31:40Z

I added a few minor questions, but otherwise looks reasonable to me:

Should there potentially be a change note for this since data extensions for Java is in beta?

For my own understanding: What is the benefit of including the .WithElement option if .Element -> .Element is identical?

I don't think that it is strictly needed with a change note as it is a non breaking change and since we are in private beta, but let us add one anyway (it can't hurt to advertise it a bit more).

The only "minor" benefit is that it might perform a bit better than .Element -> .Element and to make it consistent with the other languages.

github-actions · 2023-08-04T11:40:11Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,689,205,80,,9,,,18
+    Java Standard Library,``java.*``,3,690,205,80,,9,,,18
-    Totals,,283,18451,2142,290,16,122,33,1,391
+    Totals,,283,18452,2142,290,16,122,33,1,391

Changes to framework-coverage-java.csv:

- java.util,45,,485,,,1,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,45,440
+ java.util,45,,486,,,1,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,45,441

aschackmull · 2023-08-04T11:50:18Z

The dual WithElement is identical to an already existing construct
c : A.WithElement -> B      \equiv      c : A.Element -> B.Element

That's not entirely correct. The WithElement version also states that the object identity is preserved while an Element to Element summary just states that a (possibly) new object with the same elements is returned. This makes a difference if the type that we're tracking for the input is stronger than the types at the input/output of the summary.

github-actions · 2023-08-07T08:11:55Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,689,205,80,,9,,,18
+    Java Standard Library,``java.*``,3,691,205,80,,9,,,18
-    Totals,,283,18451,2142,290,16,122,33,1,391
+    Totals,,283,18453,2142,290,16,122,33,1,391

Changes to framework-coverage-java.csv:

- java.util,45,,485,,,1,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,45,440
+ java.util,45,,487,,,1,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,45,442

michaelnebel · 2023-08-07T08:17:29Z

The dual WithElement is identical to an already existing construct
c : A.WithElement -> B      \equiv      c : A.Element -> B.Element
That's not entirely correct. The WithElement version also states that the object identity is preserved while an Element to Element summary just states that a (possibly) new object with the same elements is returned. This makes a difference if the type that we're tracking for the input is stronger than the types at the input/output of the summary.

Hah, so what you are saying is that we infact should not use WithElement for (shallow)copy/clone like methods as they do not preserve object identity for the collection container object? 😸 (this defeats my example above).

…oduce appropriate tests.

github-actions · 2023-08-08T09:13:59Z

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

java

Generated file changes for java

Changes to framework-coverage-java.rst:

-    Java Standard Library,``java.*``,3,689,201,76,,9,,,18
+    Java Standard Library,``java.*``,3,691,201,76,,9,,,18
-    Totals,,283,18451,2138,286,16,122,33,1,391
+    Totals,,283,18453,2138,286,16,122,33,1,391

Changes to framework-coverage-java.csv:

- java.util,45,,485,,,1,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,45,440
+ java.util,45,,487,,,1,,,,,,,,,,,34,,,,,,,5,2,,1,2,,,,,,,,,,,45,442

github-actions bot added the Java label Jun 23, 2023

michaelnebel force-pushed the java/withoutelement branch from e7cf024 to 500480b Compare July 4, 2023 08:53

michaelnebel force-pushed the java/withoutelement branch from 500480b to 37d7680 Compare July 4, 2023 09:44

github-actions bot added the documentation label Jul 4, 2023

michaelnebel added the no-change-note-required This PR does not need a change note label Jul 4, 2023

michaelnebel marked this pull request as ready for review July 5, 2023 07:24

michaelnebel requested a review from a team as a code owner July 5, 2023 07:24

michaelnebel added 7 commits August 1, 2023 12:03

Java: Add MaD support for With[out]Element.

21ec83a

Java: Add WithoutElement model for List.clear and add appropriate test.

0604a85

Java: Update telemetry test expected output.

e97a4a1

Java: Re-factor a model to use WithElement (this model is already tes…

99ac98b

…ted in collections/B.java).

Java: Update test comments to reflect the MaD syntax.

fc66b6e

Java: Update MaD internal documentation.

a8ccc8d

Java: Update MaD external documentation related to With and Without E…

ffd1456

…lement.

michaelnebel force-pushed the java/withoutelement branch from c0f7b18 to ffd1456 Compare August 1, 2023 10:54

jcogs33 reviewed Aug 2, 2023

View reviewed changes

Java: Add release note and address review comments.

d3eb9c1

Java: Make a flow summary for Set.clear using WithoutElement and intr…

0ed724e

…oduce appropriate tests.

michaelnebel force-pushed the java/withoutelement branch from 823be50 to 0ed724e Compare August 8, 2023 09:10

michaelnebel requested a review from aschackmull August 9, 2023 07:09

aschackmull approved these changes Aug 14, 2023

View reviewed changes

michaelnebel merged commit a95aad5 into github:main Aug 15, 2023

michaelnebel deleted the java/withoutelement branch August 15, 2023 08:03

Java: Support for With[out]Element for MaD. #13546

Java: Support for With[out]Element for MaD. #13546

Uh oh!

Conversation

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

Uh oh!

Uh oh!

github-actions bot commented Jun 23, 2023

java

Generated file changes for java

Uh oh!

github-actions bot commented Jul 4, 2023

java

Generated file changes for java

Uh oh!

github-actions bot commented Jul 4, 2023

java

Generated file changes for java

Uh oh!

github-actions bot commented Jul 4, 2023

java

Generated file changes for java

Uh oh!

github-actions bot commented Aug 1, 2023

java

Generated file changes for java

Uh oh!

jcogs33 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcogs33 Aug 2, 2023

Choose a reason for hiding this comment

Uh oh!

michaelnebel Aug 4, 2023

Choose a reason for hiding this comment

Uh oh!

aschackmull Aug 4, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelnebel commented Aug 4, 2023

Uh oh!

github-actions bot commented Aug 4, 2023

java

Generated file changes for java

Uh oh!

aschackmull commented Aug 4, 2023

Uh oh!

github-actions bot commented Aug 7, 2023

java

Generated file changes for java

Uh oh!

michaelnebel commented Aug 7, 2023

Uh oh!

github-actions bot commented Aug 8, 2023

java

Generated file changes for java

Uh oh!

Uh oh!

michaelnebel commented Jun 23, 2023 •

edited

Loading

michaelnebel Aug 7, 2023 •

edited

Loading