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

8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager #4071

Closed
wants to merge 20 commits into from

Conversation

@wangweij
Copy link
Contributor

@wangweij wangweij commented May 17, 2021

Please review the test changes for JEP 411.

With JEP 411 and the default value of -Djava.security.manager becoming disallow, tests calling System.setSecurityManager() need -Djava.security.manager=allow when launched. This PR covers such changes for tier1 to tier3 (except for the JCK tests).

To make it easier to focus your review on the tests in your area, this PR is divided into multiple commits for different areas (serviceability, hotspot-compiler, i18n, rmi, javadoc, swing, 2d, security, hotspot-runtime, nio, xml, beans, core-libs, net, compiler, hotspot-gc). Mostly the rule is the same as how Skara adds labels, but there are several small tweaks:

  1. When a file is covered by multiple labels, only one is chosen. I make my best to avoid putting too many tests into core-libs. If a file is covered by core-libs and another label, I categorized it into the other label.
  2. If a file is in core-libs but contains /xml/ in its name, it's in the xml commit.
  3. If a file is in core-libs but contains /rmi/ in its name, it's in the rmi commit.
  4. One file not covered by any label -- test/jdk/com/sun/java/accessibility/util/8051626/Bug8051626.java -- is in the swing commit.

Due to the size of this PR, no attempt is made to update copyright years for all files to minimize unnecessary merge conflict.

Please note that this PR can be integrated before the source changes for JEP 411, as the possible values of this system property was already defined long time ago in JDK 9.

Most of the change in this PR is a simple adding of -Djava.security.manager=allow to the @run main/othervm line. Sometimes it was not othervm and we add one. Sometimes there's no @run at all and we add the line.

There are several tests that launch another Java process that needs to call the System.setSecurityManager() method, and the system property is added to ProcessBuilder, ProcessTools, or the java command line (if the test is a shell test).

3 langtools tests are added into problem list due to JDK-8265611.

2 SQL tests are moved because they need different options on the @run line but they are inside a directory that has a TEST.properties:

rename test/jdk/java/sql/{testng/test/sql/othervm => permissionTests}/DriverManagerPermissionsTests.java (93%)
rename test/jdk/javax/sql/{testng/test/rowset/spi => permissionTests}/SyncFactoryPermissionsTests.java (95%)

The source change for JEP 411 is at #4073.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager

Reviewers

Contributors

  • Lance Andersen <lancea@openjdk.org>
  • Weijun Wang <weijun@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071
$ git checkout pull/4071

Update a local copy of the PR:
$ git checkout pull/4071
$ git pull https://git.openjdk.java.net/jdk pull/4071/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4071

View PR using the GUI difftool:
$ git pr show -t 4071

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4071.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 17, 2021

👋 Welcome back weijun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented May 17, 2021

@wangweij The following labels will be automatically applied to this pull request:

  • 2d
  • awt
  • beans
  • compiler
  • core-libs
  • hotspot
  • i18n
  • javadoc
  • jmx
  • net
  • nio
  • security
  • serviceability
  • swing

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk
Copy link

@openjdk openjdk bot commented May 17, 2021

@wangweij Could not parse lancea, weijun as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented May 17, 2021

/contributor add lancea
/contributor add weijun

@openjdk
Copy link

@openjdk openjdk bot commented May 17, 2021

@wangweij
Contributor Lance Andersen <lancea@openjdk.org> successfully added.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented May 17, 2021

/contributor add weijun

@openjdk
Copy link

@openjdk openjdk bot commented May 17, 2021

@wangweij
Contributor Weijun Wang <weijun@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented May 17, 2021

@wangweij
Contributor Weijun Wang <weijun@openjdk.org> successfully added.

@wangweij wangweij marked this pull request as ready for review May 17, 2021
@openjdk openjdk bot added the rfr label May 17, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 17, 2021

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Changes to hotspot-* and serviceability look good.

Thanks,
David

@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2021

@wangweij This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager

Co-authored-by: Lance Andersen <lancea@openjdk.org>
Co-authored-by: Weijun Wang <weijun@openjdk.org>
Reviewed-by: dholmes, alanb, dfuchs, mchung, mullan, prr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 18, 2021
@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented May 18, 2021

The changes look okay but a bit inconsistent on where -Djava...=allow is inserted for tests that already set other system properties or other parameters. Not a correctness issue, just looks odd in several places, e.g.

test/jdk/java/lang/System/LoggerFinder/BaseLoggerFinderTest/BaseLoggerFinderTest.java - the tests sets the system properties after -Xbootclasspath/a: but the change means it sets properties before and after.

test/jdk/java/lang/Class/getResource/ResourcesTest.java - you've added it in the middle of the module and class path parameters.

For uses using ProcessTools then it seems to be very random.

dfuch
dfuch approved these changes May 18, 2021
Copy link
Member

@dfuch dfuch left a comment

I looked at serviceability, net, and corelibs. The changes look good to me - though I have one question about the NotificationEmissionTest. I believe that regardless of whether the new property is needed, test case 1 should be run in /othervm too.

@@ -32,10 +32,10 @@
* @run clean NotificationEmissionTest
* @run build NotificationEmissionTest
* @run main NotificationEmissionTest 1
Copy link
Member

@dfuch dfuch May 18, 2021

Choose a reason for hiding this comment

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

This test case (NotificationEmissionTest 1) calls System.setProperty("java.security.policy", policyFile); - even though it doesn't call System.setSecurityManager(); Should the @run command line for test case 1 be modified as well?

Copy link
Contributor Author

@wangweij wangweij May 18, 2021

Choose a reason for hiding this comment

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

Or maybe the policy setting line should only be called when a security manager is set? IIRC jtreg is able to restore system properties even in agentvm mode. So maybe this really doesn't matter. We just want to modify as little as possible in this PR.

@wangweij
Copy link
Contributor Author

@wangweij wangweij commented May 18, 2021

The changes look okay but a bit inconsistent on where -Djava...=allow is inserted for tests that already set other system properties or other parameters. Not a correctness issue, just looks odd in several places, e.g.

test/jdk/java/lang/System/LoggerFinder/BaseLoggerFinderTest/BaseLoggerFinderTest.java - the tests sets the system properties after -Xbootclasspath/a: but the change means it sets properties before and after.

test/jdk/java/lang/Class/getResource/ResourcesTest.java - you've added it in the middle of the module and class path parameters.

For uses using ProcessTools then it seems to be very random.

I've updated the 3 cases you mentioned in my local repo and will go through more. Yes it looks good to group system property settings together. Thanks.

Copy link
Member

@mlchung mlchung left a comment

I reviewed non-client areas. Looks okay.

Copy link
Member

@seanjmullan seanjmullan left a comment

The changes to the security tests look good.

prrace
prrace approved these changes May 21, 2021
Copy link
Contributor

@prrace prrace left a comment

The client changes are fine except for the one misplaced (c)

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2008, 2015, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
Copy link
Contributor

@prrace prrace May 21, 2021

Choose a reason for hiding this comment

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

Probably the (c) update was meant to be on the .sh file that is actually modified.

Copy link
Contributor Author

@wangweij wangweij May 21, 2021

Choose a reason for hiding this comment

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

Oops, I'll back it out. Thanks.

@wangweij wangweij changed the title 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager 8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager May 21, 2021
@wangweij
Copy link
Contributor Author

@wangweij wangweij commented May 24, 2021

/integrate

@openjdk openjdk bot closed this May 24, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels May 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 24, 2021

@wangweij Since your change was applied there have been 5 commits pushed to the master branch:

  • f04db5f: 8224158: assertion related to NPE at DynamicCallSiteDesc::withArgs should be reworded
  • 838a007: 8267584: The java.security.krb5.realm system property only needs to be defined once
  • f2d880c: 8266400: importkeystore fails to a password less pkcs12 keystore
  • f5562f1: 8258535: jvm.ClassReader should set the accessor to the corresponding record component
  • d8e6e28: 8267544: (test) rmi test NonLocalSkeleton fails if network has multiple adapters with the same address

Your commit was automatically rebased without conflicts.

Pushed as commit 640a2af.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@wangweij wangweij deleted the 8267184 branch May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment