-
Notifications
You must be signed in to change notification settings - Fork 728
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
Separate tests that rely on SecurityManager (Java8andUp) #14442
Conversation
Added compilation excludes |
We don't want to actually exclude the tests for jdk19+ until it's determined the SecurityManager is removed in jdk19. I was just a guess. We want to be ready to do so quickly, so we aren't blocked from running tests for a month when the change happens. |
I think this PR is preparing us to continue running tests when the SecurityManger related classes are removed in JDK19 or a future release. If it doesn't happen at JDK19, the change required is to just put JDK19 back to the allowed Java levels A list of deprecated APIs for removal:
fyi @EricYangIBM |
I can agree with that, as long as we have an issue in the 0.34 milestone plan to ensure we re-enable the tests if necessary. Ideally referring to all the PRs / places to fix. I've created the 0.34 milestone. |
9604e13
to
d3a8032
Compare
<version>15</version> | ||
<version>16</version> | ||
<version>17</version> | ||
<version>18</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will also have to add to versions. (Is there a better way than listing all the versions individually?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we don't, we can have something like this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering if this[1] is the playlist version parsing in question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we have a plan to add something like [8,18]. But we also need to support it in the internal perl tkg as well.
c9df40e
to
9f49220
Compare
a2eb843
to
b9c1dd6
Compare
b9c1dd6
to
a2eb843
Compare
Are there more changes coming for following tests?
|
Is this related to security manager? |
Usually [1] https://docs.oracle.com/javase/8/docs/technotes/guides/security/PolicyFiles.html |
For the two |
Scratch these two then, on the other hand, it means
Runtime check is fine for this test. |
I added the runtime check for VmArgumentTests and removed the Note: we will have to change |
test/functional/Java8andUp/build.xml
Outdated
<exclude name="org/openj9/test/java/lang/invoke/Test_MethodHandleInfo_SM.java" /> | ||
<exclude name="org/openj9/test/java/lang/Test_ClassLoader_SM.java" /> | ||
<exclude name="org/openj9/test/java/lang/Test_J9VMInternals.java" /> | ||
<exclude name="org/openj9/test/java/lang/Test_J9VMInternalsImpl.java" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some sub-tests seem applicable to JDK levels w/o SecurityManager related classes such as TestNCDFE.runTest1()
within test_checkPackageAccess()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the comment TestNG parses the test file, which interferes with the classloading under test
I added the security manager test classes with the same two file pattern now
<version>15</version> | ||
<version>16</version> | ||
<version>17</version> | ||
<version>18</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only 11/17/18 are required.
-groups $(TEST_GROUP) \ | ||
-excludegroups $(DEFAULT_EXCLUDE); \ | ||
$(TEST_STATUS)</command> | ||
<platformRequirements>^vm.hrt</platformRequirements> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vm.hrt
discontinued long time ago, this platformRequirements
is not needed.
<version>17</version> | ||
<version>18</version> | ||
</versions> | ||
<aot>nonapplicable</aot> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this aot
limitation is not needed either.
-groups $(TEST_GROUP) \ | ||
-excludegroups $(DEFAULT_EXCLUDE); \ | ||
$(TEST_STATUS)</command> | ||
<platformRequirements>^vm.hrt</platformRequirements> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly not required.
<version>15</version> | ||
<version>16</version> | ||
<version>17</version> | ||
<version>18</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only need 11/17/18, assuming this is a SM version of JCL_Test_none_SCC
which is for 11+
.
<version>15</version> | ||
<version>16</version> | ||
<version>17</version> | ||
<version>18</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only 11/17/18 are needed.
-groups $(TEST_GROUP) \ | ||
-excludegroups $(DEFAULT_EXCLUDE); \ | ||
$(TEST_STATUS)</command> | ||
<platformRequirements>^vm.hrt</platformRequirements> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required.
<version>15</version> | ||
<version>16</version> | ||
<version>17</version> | ||
<version>18</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only 11/17/18 are required.
test/functional/Java8andUp/build.xml
Outdated
<src path="src_110_up" /> | ||
<src path="${src_version}" /> | ||
<src path="${src_90_jcl}" /> | ||
<src path="${src_access}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script reads better w/o large number of duplicated elements. Can you try something like
<path id="src.path">
<pathelement path="${src}/"/>
...
<pathelement path="${src_access}"/>
...
</path>
The src.path
can be shared among targets.
<javac ...>
<sourcepath refid="src.path"/>
...
</javac>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Managed to remove duplication of src and class path using a property and path filesets
cc33cd6
to
b572a35
Compare
test/functional/Java8andUp/build.xml
Outdated
<compilerarg line='${addExports}' /> | ||
<compilerarg line='${addExports_90_jcl}' /> | ||
<compilerarg line='${addExports_version}' /> | ||
<compilerarg line='--add-modules openj9.sharedclasses' /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to remove duplicated compilerarg
w/ something like
<property name="common-args" value="${addExports} ${addExports_90_jcl} ..." />
<compilerarg line="${common-args}" />
* SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 OR LicenseRef-GPL-2.0 WITH Assembly-exception | ||
*******************************************************************************/ | ||
|
||
import org.testng.annotations.AfterMethod; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not required in this class.
import org.testng.annotations.AfterMethod; | ||
import org.testng.annotations.Test; | ||
import org.testng.log4testng.Logger; | ||
import org.testng.annotations.BeforeMethod; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not required.
@Test | ||
public void test_getMethods_subtest2() { | ||
java.security.PrivilegedAction action = new java.security.PrivilegedAction() { | ||
public Object run() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these existing code are being moved to a new class, please help some cleanup.
public Object run() { | |
PrivilegedAction<Object> action = new PrivilegedAction<Object>() { | |
@Override | |
public Object run() { |
java.net.URL url = new java.net.URL("file:" + file.getPath()); | ||
ClassLoader loader = new java.net.URLClassLoader(new java.net.URL[]{url}, null); | ||
|
||
Class cls_E = Class.forName("org.openj9.resources.classinheritance.E", false, loader); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class cls_E = Class.forName("org.openj9.resources.classinheritance.E", false, loader); | |
Class<?> cls_E = Class.forName("org.openj9.resources.classinheritance.E", false, loader); |
Support_Resources.copyFile(resources, null, "openj9tr_general.jar"); | ||
File file = new File(resources.toString() + "/openj9tr_general.jar"); | ||
java.net.URL url = new java.net.URL("file:" + file.getPath()); | ||
ClassLoader loader = new java.net.URLClassLoader(new java.net.URL[]{url}, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClassLoader loader = new java.net.URLClassLoader(new java.net.URL[]{url}, null); | |
ClassLoader loader = new URLClassLoader(new java.net.URL[]{url}, null); |
return null; | ||
} | ||
}; | ||
java.security.AccessController.doPrivileged(action); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
java.security.AccessController.doPrivileged(action); | |
AccessController.doPrivileged(action); |
}; | ||
class MyCombiner implements java.security.DomainCombiner { | ||
boolean combine; | ||
public java.security.ProtectionDomain[] combine(java.security.ProtectionDomain[] executionDomains, java.security.ProtectionDomain[] parentDomains) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public java.security.ProtectionDomain[] combine(java.security.ProtectionDomain[] executionDomains, java.security.ProtectionDomain[] parentDomains) { | |
public ProtectionDomain[] combine(ProtectionDomain[] executionDomains, ProtectionDomain[] parentDomains) { |
*/ | ||
@Test | ||
public void test_getClasses2() { | ||
final java.security.Permission privCheckPermission = new java.security.BasicPermission("Privilege check") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final java.security.Permission privCheckPermission = new java.security.BasicPermission("Privilege check") { | |
final Permission privCheckPermission = new BasicPermission("Privilege check") { |
final MyCombiner combiner = new MyCombiner(); | ||
class SecurityManagerCheck extends SecurityManager { | ||
String reason; | ||
Class checkClass; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Class checkClass; | |
Class<?> checkClass; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More cleanup for new classes please.
test/functional/Java8andUp/build.xml
Outdated
<exclude name="**/Cmvc194280.java" /> | ||
<exclude name="**/resources/**" /> | ||
<!-- requires special compilation methods --> | ||
<exclude name="**/NoSuchMethod/**" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These duplicated exclude
can be shared via something like
<property name="commonExcludes" value="**/Cmvc194280.java,**/resources/**, ... " />
<javac srcdir="${src}" excludes="${commonExcludes}"
|
||
import org.openj9.test.support.resource.Support_Resources; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking : an extra line.
private static final Logger logger = Logger.getLogger(Test_Class_SM.class); | ||
|
||
@Test | ||
public void test_getMethods_subtest2() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking : pls add an indention to align w/ private static final Logger logger
.
public Object run() { | ||
try { | ||
File resources = Support_Resources.createTempFolder(); | ||
String[] expected_E = new String[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking: use a apace instead of the tab between String[]
and expected_E
.
Support_Resources.copyFile(resources, null, "openj9tr_general.jar"); | ||
File file = new File(resources.toString() + "/openj9tr_general.jar"); | ||
URL url = new URL("file:" + file.getPath()); | ||
ClassLoader loader = new URLClassLoader(new URL[]{url}, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ClassLoader loader = new URLClassLoader(new URL[]{url}, null); | |
ClassLoader loader = new URLClassLoader(new URL[] { url }, null); |
private Class<?> getClass( String className )throws ClassNotFoundException { | ||
String classFile = className.replace('.', '/') + ".class"; | ||
try { | ||
InputStream classStream = getClass().getClassLoader().getResourceAsStream( classFile ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking: additional spaces around classFile
String classFile = className.replace('.', '/') + ".class"; | ||
try { | ||
InputStream classStream = getClass().getClassLoader().getResourceAsStream( classFile ); | ||
if ( classStream == null ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking: additional spaces around classStream == null
try { | ||
InputStream classStream = getClass().getClassLoader().getResourceAsStream( classFile ); | ||
if ( classStream == null ) { | ||
throw new ClassNotFoundException( "Error loading class : " + classFile ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking: additional spaces around "Error loading class : " + classFile
Class<?> clazz = defineClass( className, classRep, 0, classRep.length ); | ||
return clazz; | ||
} catch ( IOException e ) { | ||
throw new ClassNotFoundException( e.getMessage() ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new ClassNotFoundException( e.getMessage() ); | |
DataInputStream in = new DataInputStream(classStream); | |
in.readFully(classRep); | |
in.close(); | |
Class<?> clazz = defineClass(className, classRep, 0, classRep.length); | |
return clazz; | |
} catch (IOException e) { | |
throw new ClassNotFoundException(e.getMessage()); |
nitpicking: remove additional spaces, also a few other places in this class.
|
||
expectedArgs = new String[initialExpectedArgs.length + 1]; | ||
System.arraycopy(initialExpectedArgs, 0, expectedArgs, 0, initialExpectedArgs.length); | ||
expectedArgs[initialExpectedArgs.length] = "-Djava.security.policy=/opt/IBM/WebSphere/AppServer80/profiles/AppSrv01/properties/server.policy"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls define a string constant to share -Djava.security.policy=/opt/IBM/WebSphere/AppServer80/profiles/AppSrv01/properties/server.policy
, probably -Djava.ext.dirs=/opt/IBM/WebSphere/AppServer80/tivoli/tam:/opt/IBM/WebSphere/AppServer80/java/jre/lib/ext
as well.
@renfeiw could you help to review this PR? Thanks |
I have two other smaller PRs for |
This isn't working on vmfarm, see /jobs_by_status.php?build_id=23133&status=FAILED
I didn't try jenkins (yet) but I assume it's been tested there. |
28848b9
to
383b38f
Compare
I think the problem is that it is looking for |
Sanity.functional passes for jdk8 and 11: https://hyc-runtimes-jenkins.swg-devops.com/view/OpenJ9%20-%20Personal/job/Pipeline-Build-Test-Personal/12062/ |
JavaNext(19) doesn't work.
|
A vmfarm build didn't work either (/build_info.php?build_id=23259), although the problem doesn't seem obviously related to these changes. I'm not sure yet what else it could be. |
|
The vmfarm build problem also occurs without these changes so something else has caused it. I'll try these changes again once the other problem is resolved. |
Am I correct that all test classes named in |
83c54fb
to
300454d
Compare
A new vmfarm build passed. |
Although that was before the latest changes. I'll run a new one. |
Latest changes also passed on vmfarm. |
jenkins test sanity xlinux jdk8,jdk11,jdk17,jdk18,jdknext |
lgtm, I can see all the new tests running on 11-18, and not on 19. JCL_Test_SM
JCL_TEST_Java-Security_SM
J9VMInternals_Test_SM
|
SecurityManager will be removed in a future jdk. Separate tests that rely on it so that they can be easily disabled in the future. Issue: eclipse-openj9#14412 Signed-off-by: Eric Yang <[email protected]>
690fd9b
to
8097a01
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @EricYangIBM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
SecurityManager will be removed in a future jdk. Separate tests that
rely on it so that they can be easily disabled in the future.
Issue: #14412
Signed-off-by: Eric Yang [email protected]