Java: Add support for java.util.StringJoiner #10533

pwntester · 2022-09-22T11:59:54Z

No description provided.

github-actions · 2022-09-22T12:03:07Z

⚠️ 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,588,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8431,1524,129,6,10,107,33,1,86

Changes to framework-coverage-java.csv:

- java.util,44,,461,,,,,,,,,,,34,,,,,,5,2,,1,2,,,,,,,,,,,,,,36,425
+ java.util,44,,464,,,,,,,,,,,34,,,,,,5,2,,1,2,,,,,,,,,,,,,,39,425

Add tests

atorralba · 2022-09-22T17:54:17Z

Hey @pwntester, thanks for this! I added some more CSV rows for the fluent methods of StringJoiner, tests, and a change note.

github-actions · 2022-09-22T17:58:05Z

⚠️ 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,591,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8434,1524,129,6,10,107,33,1,86

Changes to framework-coverage-java.csv:

- java.util,44,,461,,,,,,,,,,,34,,,,,,5,2,,1,2,,,,,,,,,,,,,,36,425
+ java.util,44,,467,,,,,,,,,,,34,,,,,,5,2,,1,2,,,,,,,,,,,,,,39,428

github-actions · 2022-09-22T18:09: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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,592,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8435,1524,129,6,10,107,33,1,86

Changes to framework-coverage-java.csv:

- java.util,44,,461,,,,,,,,,,,34,,,,,,5,2,,1,2,,,,,,,,,,,,,,36,425
+ java.util,44,,468,,,,,,,,,,,34,,,,,,5,2,,1,2,,,,,,,,,,,,,,40,428

Marcono1234

Hopefully these comments are useful.
Feel free to consider them only as suggestions since I am not a project member.

Marcono1234 · 2022-09-22T22:24:14Z

java/ql/lib/semmle/code/java/frameworks/Strings.qll

+        "java.util;StringJoiner;true;merge;;;Argument[0];Argument[-1];taint;manual",
+        "java.util;StringJoiner;true;merge;;;Argument[-1];ReturnValue;value;manual",
+        "java.util;StringJoiner;true;setEmptyValue;;;Argument[-1];ReturnValue;value;manual",
+        "java.util;StringJoiner;true;toString;;;Argument[-1];ReturnValue;taint;manual"


Might be useful to add a trailing comma to avoid a diff for this line when new lines are added:

Suggested change

"java.util;StringJoiner;true;toString;;;Argument[-1];ReturnValue;taint;manual"

"java.util;StringJoiner;true;toString;;;Argument[-1];ReturnValue;taint;manual",

Marcono1234 · 2022-09-22T22:24:14Z

java/ql/lib/semmle/code/java/frameworks/Strings.qll

@@ -64,7 +64,14 @@ private class StringSummaryCsv extends SummaryModelCsv {
        "java.lang;StringBuilder;true;StringBuilder;;;Argument[0];Argument[-1];taint;manual",
        "java.lang;CharSequence;true;charAt;;;Argument[-1];ReturnValue;taint;manual",
        "java.lang;CharSequence;true;subSequence;;;Argument[-1];ReturnValue;taint;manual",
-        "java.lang;CharSequence;true;toString;;;Argument[-1];ReturnValue;taint;manual"
+        "java.lang;CharSequence;true;toString;;;Argument[-1];ReturnValue;taint;manual",
+        "java.util;StringJoiner;true;StringJoiner;;;Argument[0..2];Argument[-1];taint;manual",


Not sure if it matters, but I think these could all be false because StringJoiner is final.

Suggested change

"java.util;StringJoiner;true;StringJoiner;;;Argument[0..2];Argument[-1];taint;manual",

"java.util;StringJoiner;false;StringJoiner;;;Argument[0..2];Argument[-1];taint;manual",

@atorralba, not sure if this is relevant, but the tests still have comments with java.util;StringJoiner;true.

Well, it's better that we stay accurate 😄 Fixed, thanks!

Marcono1234 · 2022-09-22T22:24:14Z

java/ql/lib/semmle/code/java/frameworks/Strings.qll

+        "java.util;StringJoiner;true;add;;;Argument[-1];ReturnValue;value;manual",
+        "java.util;StringJoiner;true;merge;;;Argument[0];Argument[-1];taint;manual",
+        "java.util;StringJoiner;true;merge;;;Argument[-1];ReturnValue;value;manual",
+        "java.util;StringJoiner;true;setEmptyValue;;;Argument[-1];ReturnValue;value;manual",


Should there also be taint from the setEmptyValue argument to the StringJoiner instance?

github-actions · 2022-09-23T07:44:26Z

⚠️ 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,593,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8436,1524,129,6,10,107,33,1,86

Changes to framework-coverage-java.csv:

- java.util,44,,461,,,,,,,,,,,34,,,,,,5,2,,1,2,,,,,,,,,,,,,,36,425
+ java.util,44,,469,,,,,,,,,,,34,,,,,,5,2,,1,2,,,,,,,,,,,,,,41,428

atorralba · 2022-09-23T07:45:25Z

Thanks @Marcono1234, those are good suggestions. I had doubts about setEmptyValue and that's why I didn't add it in the first place, but even if taint would only get propagated in certain cases (when no other strings are joined) I guess it makes sense to have the model.

Applied in e849acb.

github-actions · 2022-09-27T07:44:46Z

⚠️ 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,585,130,28,,,7,,,10
+    Java Standard Library,``java.*``,3,593,130,28,,,7,,,10
-    Totals,,217,8428,1524,129,6,10,107,33,1,86
+    Totals,,217,8436,1524,129,6,10,107,33,1,86

Changes to framework-coverage-java.csv:

- java.util,44,,461,,,,,,,,,,,34,,,,,,5,2,,1,2,,,,,,,,,,,,,,36,425
+ java.util,44,,469,,,,,,,,,,,34,,,,,,5,2,,1,2,,,,,,,,,,,,,,41,428

Add support for java.util.StringJoiner

3c7fdc9

pwntester requested a review from a team as a code owner Sep 22, 2022

github-actions bot added the Java label Sep 22, 2022

atorralba added 2 commits Sep 22, 2022

Add fluent models

db4db02

Add tests

Add change note

0a1676b

github-actions bot added the documentation label Sep 22, 2022

atorralba changed the title ~~Add support for java.util.StringJoiner~~ Java: Add support for java.util.StringJoiner Sep 22, 2022

github deleted a comment from github-actions bot Sep 22, 2022

Add models for StringJoiner constructor

904ce25

Marcono1234 reviewed Sep 22, 2022

View changes

Add review suggestions

e849acb

Fix test comments

c9a3d61

Java: Add support for java.util.StringJoiner #10533

Java: Add support for java.util.StringJoiner #10533

pwntester commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

java

Generated file changes for java

atorralba commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

java

Generated file changes for java

github-actions bot commented Sep 22, 2022

java

Generated file changes for java

Marcono1234 left a comment

Marcono1234 Sep 22, 2022

Marcono1234 Sep 22, 2022

Marcono1234 Sep 23, 2022

atorralba Sep 27, 2022

Marcono1234 Sep 22, 2022

github-actions bot commented Sep 23, 2022

java

Generated file changes for java

atorralba commented Sep 23, 2022

github-actions bot commented Sep 27, 2022

java

Generated file changes for java

	"java.util;StringJoiner;true;toString;;;Argument[-1];ReturnValue;taint;manual"
	"java.util;StringJoiner;true;toString;;;Argument[-1];ReturnValue;taint;manual",

	"java.util;StringJoiner;true;StringJoiner;;;Argument[0..2];Argument[-1];taint;manual",
	"java.util;StringJoiner;false;StringJoiner;;;Argument[0..2];Argument[-1];taint;manual",

Java: Add support for java.util.StringJoiner #10533

Are you sure you want to change the base?

Java: Add support for java.util.StringJoiner #10533

Conversation

pwntester commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

java

Generated file changes for java

atorralba commented Sep 22, 2022

github-actions bot commented Sep 22, 2022

java

Generated file changes for java

github-actions bot commented Sep 22, 2022

java

Generated file changes for java

Marcono1234 left a comment

Choose a reason for hiding this comment

Marcono1234 Sep 22, 2022

Choose a reason for hiding this comment

Marcono1234 Sep 22, 2022

Choose a reason for hiding this comment

Marcono1234 Sep 23, 2022

Choose a reason for hiding this comment

atorralba Sep 27, 2022

Choose a reason for hiding this comment

Marcono1234 Sep 22, 2022

Choose a reason for hiding this comment

github-actions bot commented Sep 23, 2022

java

Generated file changes for java

atorralba commented Sep 23, 2022

github-actions bot commented Sep 27, 2022

java

Generated file changes for java