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

[JENKINS-68070] Adapt generic-whitelist to Java standard library changes in Java 15+ #394

Merged
merged 4 commits into from
Mar 18, 2022

Conversation

dwnusbaum
Copy link
Member

@dwnusbaum dwnusbaum commented Mar 17, 2022

See JENKINS-68070.

Java 15 added a new CharSequence.isEmpty default method. This breaks StaticWhitelistTest when running against Java 15+ because String.isEmpty becomes an override. It would also cause RejectedAccessException to be thrown for any sandboxed code that currently calls String.isEmpty.

This PR preemptively adds CharSequence.isEmpty to generic-whitelist so that everything should regardless of what Java version is being used.

I have not actually tested this against Java 15+ myself, so marking the PR as a draft.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@jglick jglick added the bug label Mar 17, 2022
@jglick
Copy link
Member

jglick commented Mar 17, 2022

mvnd test -Dtest=org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.GenericWhitelistTest

still fails on Java 17 with this PR:

java.lang.AssertionError: method java.util.Random nextBoolean does not exist (or is an override)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.StaticWhitelistTest.sanity(StaticWhitelistTest.java:102)
	at org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.GenericWhitelistTest.sanity(GenericWhitelistTest.java:39)

@jglick
Copy link
Member

jglick commented Mar 17, 2022

Could switch

[ platform: "linux", jdk: "11" ]
to 17 to get coverage in CI.

@basil
Copy link
Member

basil commented Mar 17, 2022

java.lang.AssertionError: method java.util.Random nextBoolean does not exist (or is an override)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.StaticWhitelistTest.sanity(StaticWhitelistTest.java:102)
	at org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.GenericWhitelistTest.sanity(GenericWhitelistTest.java:39)

As Jesse points out, there are some additional cases like this in Random and RandomGenerator. References:

If you want to take care of this in this PR, that is fine with me; otherwise, I can file a new PR to cover those cases. I started down this path already and got the tests passing in Java 17 with this diff:

diff --git a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/generic-whitelist b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/generic-whitelist
index edc554a..5504e45 100644
--- a/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/generic-whitelist
+++ b/src/main/resources/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/generic-whitelist
@@ -774,6 +774,14 @@ staticField java.util.concurrent.TimeUnit HOURS
 staticField java.util.concurrent.TimeUnit MILLISECONDS
 staticField java.util.concurrent.TimeUnit MINUTES
 staticField java.util.concurrent.TimeUnit SECONDS
+method java.util.random.RandomGenerator nextBoolean
+method java.util.random.RandomGenerator nextBytes byte[]
+method java.util.random.RandomGenerator nextDouble
+method java.util.random.RandomGenerator nextFloat
+method java.util.random.RandomGenerator nextGaussian
+method java.util.random.RandomGenerator nextInt
+method java.util.random.RandomGenerator nextInt int
+method java.util.random.RandomGenerator nextLong
 method java.util.regex.MatchResult end
 method java.util.regex.MatchResult end int
 method java.util.regex.MatchResult group
diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelistTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelistTest.java
index ef31811..0f07191 100644
--- a/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelistTest.java
+++ b/src/test/java/org/jenkinsci/plugins/scriptsecurity/sandbox/whitelists/StaticWhitelistTest.java
@@ -37,6 +37,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Random;
 import java.util.Set;
 
 import org.jenkinsci.plugins.scriptsecurity.sandbox.whitelists.EnumeratingWhitelist.MethodSignature;
@@ -125,7 +126,24 @@ public class StaticWhitelistTest {
             // Overrides CharSequence.isEmpty in Java 15+.
             new MethodSignature(String.class, "isEmpty", new Class<?>[0]),
             // Does not exist until Java 15.
-            new MethodSignature(CharSequence.class, "isEmpty", new Class<?>[0])
+            new MethodSignature(CharSequence.class, "isEmpty", new Class<?>[0]),
+            // Override the corresponding RandomGenerator methods in Java 17+.
+            new MethodSignature(Random.class, "nextBoolean", new Class<?>[0]),
+            new MethodSignature(Random.class, "nextBytes", new Class<?>[] {byte[].class}),
+            new MethodSignature(Random.class, "nextDouble", new Class<?>[0]),
+            new MethodSignature(Random.class, "nextFloat", new Class<?>[0]),
+            new MethodSignature(Random.class, "nextGaussian", new Class<?>[0]),
+            new MethodSignature(Random.class, "nextInt", new Class<?>[0]),
+            new MethodSignature(Random.class, "nextInt", new Class<?>[] {int.class}),
+            new MethodSignature(Random.class, "nextLong", new Class<?>[0]),
+            // Do not exist until Java 17.
+            new MethodSignature("java.util.random.RandomGenerator", "nextBoolean", new String[0]),
+            new MethodSignature("java.util.random.RandomGenerator", "nextBytes", new String[] {"byte[]"}),
+            new MethodSignature("java.util.random.RandomGenerator", "nextDouble", new String[0]),
+            new MethodSignature("java.util.random.RandomGenerator", "nextFloat", new String[0]),
+            new MethodSignature("java.util.random.RandomGenerator", "nextGaussian", new String[0]),
+            new MethodSignature("java.util.random.RandomGenerator", "nextInt", new String[0]),
+            new MethodSignature("java.util.random.RandomGenerator", "nextInt", new String[] {"int"}),
+            new MethodSignature("java.util.random.RandomGenerator", "nextLong", new String[0])
     ));
 
     @Test public void sanity() throws Exception {

However they then started failing on Java 11 because the java.util.random.RandomGenerator cannot be loaded on Java 11 (since it was only introduced in Java 17).

@basil
Copy link
Member

basil commented Mar 17, 2022

Could switch

[ platform: "linux", jdk: "11" ]

to 17 to get coverage in CI.

Unfortunately that will not work so long as --add-opens arguments are needed to test on Java 17. I am working on adding first-class Java 11 and 17 support to the toolchain in a way that covers cases like this; stay tuned. 😎

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Thank you!

@dwnusbaum
Copy link
Member Author

@basil Thanks for the diff!

However they then started failing on Java 11 because the java.util.random.RandomGenerator cannot be loaded on Java 11 (since it was only introduced in Java 17).

Do you have a stack trace for this? I ran the tests locally on Java 11 and they all passed.

@basil
Copy link
Member

basil commented Mar 18, 2022

Do you have a stack trace for this? I ran the tests locally on Java 11 and they all passed.

I must have been in error, since your branch at commit e137c1f passes mvn clean verify for me with Java 11. It also passed with Java 17 and --add-opens java.base/java.lang=ALL-UNNAMED --add-opens java.base/java.io=ALL-UNNAMED --add-opens java.base/java.util=ALL-UNNAMED --add-opens java.base/java.util.concurrent=ALL-UNNAMED.

@dwnusbaum dwnusbaum marked this pull request as ready for review March 18, 2022 16:56
Copy link
Contributor

@car-roll car-roll left a comment

Choose a reason for hiding this comment

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

LGTM!

@dwnusbaum dwnusbaum changed the title [JENKINS-68070] Adapt to addition of CharSequence.isEmpty in Java 15 [JENKINS-68070] Adapt generic-whitelist to Java standard library changes in Java 15+ Mar 18, 2022
@dwnusbaum dwnusbaum merged commit bcf6cf6 into jenkinsci:master Mar 18, 2022
@dwnusbaum dwnusbaum deleted the JENKINS-68070 branch March 18, 2022 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants