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

8266459: Implement JEP 411: Deprecate the Security Manager for Removal #4073

Closed
wants to merge 14 commits into from

Conversation

wangweij
Copy link
Contributor

@wangweij wangweij commented May 17, 2021

Please review this implementation of JEP 411.

The code change is divided into 3 commits. Please review them one by one.

  1. 576161d The essential change for this JEP, including the @Deprecate annotations and spec change. It also update the default value of the java.security.manager system property to "disallow", and necessary test change following this update.
  2. 26a54a8 Manual changes to several files so that the next commit can be generated programatically.
  3. eb6c566 Automatic changes to other source files to avoid javac warnings on deprecation for removal

The 1st and 2nd commits should be reviewed carefully. The 3rd one is generated programmatically, see the comment below for more details. If you are only interested in a portion of the 3rd commit and would like to review it as a separate file, please comment here and I'll generate an individual webrev.

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

Furthermore, since the default value of java.security.manager system property is now "disallow", most of the tests calling System.setSecurityManager() need to launched with -Djava.security.manager=allow. This is covered in a different PR at #4071.

Update: the deprecation annotations and javadoc tags, build, compiler, core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are reviewed. Rest are 2d, awt, beans, sound, and swing.


Progress

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

Issue

  • JDK-8266459: Implement JEP 411: Deprecate the Security Manager for Removal

Reviewers

Contributors

  • Sean Mullan <mullan@openjdk.org>
  • 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/4073/head:pull/4073
$ git checkout pull/4073

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4073

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

Using diff file

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

@bridgekeeper
Copy link

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.

@wangweij
Copy link
Contributor Author

wangweij commented May 17, 2021

The 3rd commit is the biggest one but it's all generated programmatically. All changes are simply adding @SupressWarnings("removal") to a declaration.

Precisely, of all the diff hunks (leading whitespaces ignored), there are 1607

+ @SuppressWarnings("removal")

There are also 7 cases where an existing annotation is updated

================= 2 ====================
- @SuppressWarnings("deprecation")
+ @SuppressWarnings({"removal","deprecation"})
================= 1 ====================
- @SuppressWarnings("Convert2Lambda")
+ @SuppressWarnings({"removal","Convert2Lambda"})
================= 1 ====================
- @SuppressWarnings({"unchecked", "CloneDeclaresCloneNotSupported"})
+ @SuppressWarnings({"removal","unchecked", "CloneDeclaresCloneNotSupported"})
================= 1 ====================
- @SuppressWarnings("deprecation") // Use of RMISecurityManager
+ @SuppressWarnings({"removal","deprecation"}) // Use of RMISecurityManager
================= 1 ====================
- @SuppressWarnings("unchecked")     /*To suppress warning from line 1374*/
+ @SuppressWarnings({"removal","unchecked"})     /*To suppress warning from line 1374*/
================= 1 ====================
- @SuppressWarnings("fallthrough")
+ @SuppressWarnings({"removal","fallthrough"})

All other cases are new annotation embedded inline:

================= 7 ====================
- AccessControlContext acc) {
+ @SuppressWarnings("removal") AccessControlContext acc) {
================= 4 ====================
- AccessControlContext acc,
+ @SuppressWarnings("removal") AccessControlContext acc,
================= 3 ====================
- AccessControlContext context,
+ @SuppressWarnings("removal") AccessControlContext context,
================= 3 ====================
- AccessControlContext acc)
+ @SuppressWarnings("removal") AccessControlContext acc)
================= 2 ====================
- try (InputStream stream = AccessController.doPrivileged(pa)) {
+ try (@SuppressWarnings("removal") InputStream stream = AccessController.doPrivileged(pa)) {
================= 2 ====================
- AccessControlContext context, Permission... perms) {
+ @SuppressWarnings("removal") AccessControlContext context, Permission... perms) {
================= 2 ====================
- } catch (java.security.AccessControlException e) {
+ } catch (@SuppressWarnings("removal") java.security.AccessControlException e) {
================= 2 ====================
- } catch (AccessControlException ace) {
+ } catch (@SuppressWarnings("removal") AccessControlException ace) {
================= 2 ====================
- AccessControlContext context)
+ @SuppressWarnings("removal") AccessControlContext context)
================= 2 ====================
- AccessControlContext acc) throws LoginException {
+ @SuppressWarnings("removal") AccessControlContext acc) throws LoginException {
================= 2 ====================
- } catch (AccessControlException e) {
+ } catch (@SuppressWarnings("removal") AccessControlException e) {
================= 1 ====================
- public static void addHook(AccessControlContext acc, PlatformEventType type, Runnable hook) {
+ public static void addHook(@SuppressWarnings("removal") AccessControlContext acc, PlatformEventType type, Runnable hook) {
================= 1 ====================
- DomainCombiner combiner,
+ @SuppressWarnings("removal") DomainCombiner combiner,
================= 1 ====================
- } catch (java.security.AccessControlException ace) {
+ } catch (@SuppressWarnings("removal") java.security.AccessControlException ace) {
================= 1 ====================
- private static File checkFile(File f, SecurityManager sm) {
+ private static File checkFile(File f, @SuppressWarnings("removal") SecurityManager sm) {
================= 1 ====================
- private static File checkFile(File file, SecurityManager sm) {
+ private static File checkFile(File file, @SuppressWarnings("removal") SecurityManager sm) {
================= 1 ====================
- private static void checkPackageAccessForPermittedSubclasses(SecurityManager sm,
+ private static void checkPackageAccessForPermittedSubclasses(@SuppressWarnings("removal") SecurityManager sm,
================= 1 ====================
- ProtectionDomain[] getProtectDomains(AccessControlContext context);
+ ProtectionDomain[] getProtectDomains(@SuppressWarnings("removal") AccessControlContext context);
================= 1 ====================
- SecureCallbackHandler(java.security.AccessControlContext acc,
+ SecureCallbackHandler(@SuppressWarnings("removal") java.security.AccessControlContext acc,
================= 1 ====================
- private static void addHookInternal(AccessControlContext acc, PlatformEventType type, Runnable hook) {
+ private static void addHookInternal(@SuppressWarnings("removal") AccessControlContext acc, PlatformEventType type, Runnable hook) {
================= 1 ====================
- private void checkMemberAccess(SecurityManager sm, int which,
+ private void checkMemberAccess(@SuppressWarnings("removal") SecurityManager sm, int which,
================= 1 ====================
- private static File[] checkFiles(Stream<File> filesStream, SecurityManager sm) {
+ private static File[] checkFiles(Stream<File> filesStream, @SuppressWarnings("removal") SecurityManager sm) {
================= 1 ====================
- Thread(Runnable target, AccessControlContext acc) {
+ Thread(Runnable target, @SuppressWarnings("removal") AccessControlContext acc) {
================= 1 ====================
- public ProtectionDomain[] getProtectDomains(AccessControlContext context) {
+ public ProtectionDomain[] getProtectDomains(@SuppressWarnings("removal") AccessControlContext context) {
================= 1 ====================
- AccessControlContext context);
+ @SuppressWarnings("removal") AccessControlContext context);
================= 1 ====================
- AccessControlContext stack,
- AccessControlContext context);
+ @SuppressWarnings("removal") AccessControlContext stack,
+ @SuppressWarnings("removal") AccessControlContext context);
================= 1 ====================
- AccessControlContext(ProtectionDomain caller, DomainCombiner combiner,
+ AccessControlContext(ProtectionDomain caller, @SuppressWarnings("removal") DomainCombiner combiner,
================= 1 ====================
- public URLClassPath(URL[] urls, AccessControlContext acc) {
+ public URLClassPath(URL[] urls, @SuppressWarnings("removal") AccessControlContext acc) {
================= 1 ====================
- URLClassLoader(URL[] urls, AccessControlContext acc) {
+ URLClassLoader(URL[] urls, @SuppressWarnings("removal") AccessControlContext acc) {
================= 1 ====================
- public static void setSecurityManager(SecurityManager sm) {
+ public static void setSecurityManager(@SuppressWarnings("removal") SecurityManager sm) {
================= 1 ====================
- try (DataInputStream dis = new DataInputStream(new InflaterInputStream(
+ try (@SuppressWarnings("removal") DataInputStream dis = new DataInputStream(new InflaterInputStream(
================= 1 ====================
- try (FileInputStream fis = AccessController.doPrivileged(
+ try (@SuppressWarnings("removal") FileInputStream fis = AccessController.doPrivileged(
================= 1 ====================
- FactoryURLClassLoader(URL[] urls, AccessControlContext acc) {
+ FactoryURLClassLoader(URL[] urls, @SuppressWarnings("removal") AccessControlContext acc) {
================= 1 ====================
- Thread newThreadWithAcc(Runnable target, AccessControlContext acc);
+ Thread newThreadWithAcc(Runnable target, @SuppressWarnings("removal") AccessControlContext acc);
================= 1 ====================
- SecureRecorderListener(AccessControlContext context, FlightRecorderListener changeListener) {
+ SecureRecorderListener(@SuppressWarnings("removal") AccessControlContext context, FlightRecorderListener changeListener) {
================= 1 ====================
- private PolicyDelegate(PolicySpi spi, Provider p,
+ private PolicyDelegate(@SuppressWarnings("removal") PolicySpi spi, Provider p,
================= 1 ====================
- DomainCombiner combiner) {
+ @SuppressWarnings("removal") DomainCombiner combiner) {
================= 1 ====================
- PrivilegedRunnable(Runnable r, AccessControlContext acc) {
+ PrivilegedRunnable(Runnable r, @SuppressWarnings("removal") AccessControlContext acc) {
================= 1 ====================
- private static File[] checkFiles(Stream<File> fs, SecurityManager sm) {
+ private static File[] checkFiles(Stream<File> fs, @SuppressWarnings("removal") SecurityManager sm) {
================= 1 ====================
- private void checkSpecifyHandler(SecurityManager sm) {
+ private void checkSpecifyHandler(@SuppressWarnings("removal") SecurityManager sm) {
================= 1 ====================
- String serverPrincipal, AccessControlContext acc)
+ String serverPrincipal, @SuppressWarnings("removal") AccessControlContext acc)
================= 1 ====================
- public Thread newThreadWithAcc(Runnable target, AccessControlContext acc) {
+ public Thread newThreadWithAcc(Runnable target, @SuppressWarnings("removal") AccessControlContext acc) {
================= 1 ====================
- try (InputStream is = AccessController.doPrivileged(new PrivilegedAction<InputStream>() {
+ try (@SuppressWarnings("removal") InputStream is = AccessController.doPrivileged(new PrivilegedAction<InputStream>() {
================= 1 ====================
- public EventFileStream(AccessControlContext acc, Path path) throws IOException {
+ public EventFileStream(@SuppressWarnings("removal") AccessControlContext acc, Path path) throws IOException {
================= 1 ====================
- AccessControlContext context, Permission... perms)
+ @SuppressWarnings("removal") AccessControlContext context, Permission... perms)
================= 1 ====================
- private static void privateCheckPackageAccess(SecurityManager s, Class<?> clazz) {
+ private static void privateCheckPackageAccess(@SuppressWarnings("removal") SecurityManager s, Class<?> clazz) {
================= 1 ====================
- AbstractEventStream(AccessControlContext acc, PlatformRecording recording, List<Configuration> configurations) throws IOException {
+ AbstractEventStream(@SuppressWarnings("removal") AccessControlContext acc, PlatformRecording recording, List<Configuration> configurations) throws IOException {
================= 1 ====================
- private void checkPackageAccess(SecurityManager sm, final ClassLoader ccl,
+ private void checkPackageAccess(@SuppressWarnings("removal") SecurityManager sm, final ClassLoader ccl,
================= 1 ====================
- AccessControlContext context) {
+ @SuppressWarnings("removal") AccessControlContext context) {
================= 1 ====================
- public PrivilegedExecutor(Executor executor, AccessControlContext acc) {
+ public PrivilegedExecutor(Executor executor, @SuppressWarnings("removal") AccessControlContext acc) {
================= 1 ====================
- private RequestHook(AccessControlContext acc, PlatformEventType eventType, Runnable hook) {
+ private RequestHook(@SuppressWarnings("removal") AccessControlContext acc, PlatformEventType eventType, Runnable hook) {
================= 1 ====================
- try (BufferedReader bufferedReader =
+ try (@SuppressWarnings("removal") BufferedReader bufferedReader =
================= 1 ====================
- private static void privateCheckProxyPackageAccess(SecurityManager s, Class<?> clazz) {
+ private static void privateCheckProxyPackageAccess(@SuppressWarnings("removal") SecurityManager s, Class<?> clazz) {

That's all.

@openjdk
Copy link

openjdk bot commented May 17, 2021

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

  • 2d
  • awt
  • beans
  • build
  • compiler
  • core-libs
  • hotspot
  • i18n
  • jmx
  • net
  • nio
  • security
  • serviceability
  • sound
  • 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 openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org sound client-libs-dev@openjdk.org i18n i18n-dev@openjdk.org security security-dev@openjdk.org 2d client-libs-dev@openjdk.org swing client-libs-dev@openjdk.org jmx jmx-dev@openjdk.org nio nio-dev@openjdk.org build build-dev@openjdk.org beans client-libs-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels May 17, 2021
@wangweij
Copy link
Contributor Author

wangweij commented May 17, 2021

/contributor add mullan, lancea, weijun

@openjdk openjdk bot added net net-dev@openjdk.org compiler compiler-dev@openjdk.org awt client-libs-dev@openjdk.org labels May 17, 2021
@openjdk
Copy link

openjdk bot commented May 17, 2021

@wangweij Could not parse mullan, 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 commented May 17, 2021

/contributor add mullan
/contributor add lancea
/contributor add weijun

@openjdk
Copy link

openjdk bot commented May 17, 2021

@wangweij
Contributor Sean Mullan <mullan@openjdk.org> successfully added.

@wangweij
Copy link
Contributor Author

wangweij commented May 17, 2021

/contributor add lancea

@wangweij
Copy link
Contributor Author

wangweij commented May 17, 2021

/contributor add weijun

@openjdk
Copy link

openjdk bot commented May 17, 2021

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

@openjdk
Copy link

openjdk bot commented May 17, 2021

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

@openjdk openjdk bot removed the ready Pull request is ready to be integrated label May 27, 2021
@openjdk openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels May 27, 2021
@wangweij
Copy link
Contributor Author

wangweij commented May 28, 2021

Two files were removed by JEP 403 and JEP 407, respectively. One method in XMLSchemaFactory.java got its own @SuppressWarnings and have to be merged with the one here. Another file ResourceBundle.java had a portion of a method extracted into a new method and the annotation must be moved there.

@wangweij
Copy link
Contributor Author

wangweij commented May 31, 2021

New commit pushed. The default behavior is now reverted to be equivalent to -Djava.security.manager=allow. No warning will be shown when the system property is set to "allow", "disallow", or not set. A new test is added to check these warning messages. Some tests are updated to match the new behavior.

See https://mail.openjdk.java.net/pipermail/jdk-dev/2021-May/005616.html.

@wangweij
Copy link
Contributor Author

wangweij commented May 31, 2021

/csr needed

@openjdk
Copy link

openjdk bot commented May 31, 2021

@wangweij the issue for this pull request, JDK-8266459, already has an approved CSR request: JDK-8266577

@AlanBateman
Copy link
Contributor

AlanBateman commented Jun 1, 2021

System.setSecurityManagerDirect looks a bit ugly now. Can this be renamed to implSetSecurityManager and avoid the line break in the middle of the declaration?

The usage of System.err usage in setSecurityManager also needs to be re-examined as this will run arbitrary code when System.err can be changed. To fix this will require capturing the stream at startup (as was done with the illegal access logger). It's okay to integrate with what you have for the first push and we can fix this issue with System.err when the warning message is changed to the intended message.

@openjdk openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Jun 1, 2021
@openjdk openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Jun 1, 2021
@wangweij
Copy link
Contributor Author

wangweij commented Jun 2, 2021

/integrate

@openjdk openjdk bot closed this Jun 2, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 2, 2021
@openjdk
Copy link

openjdk bot commented Jun 2, 2021

@wangweij Pushed as commit 6765f90.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2d client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org beans client-libs-dev@openjdk.org build build-dev@openjdk.org compiler compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org hotspot hotspot-dev@openjdk.org i18n i18n-dev@openjdk.org integrated Pull request has been integrated jmx jmx-dev@openjdk.org net net-dev@openjdk.org nio nio-dev@openjdk.org security security-dev@openjdk.org serviceability serviceability-dev@openjdk.org sound client-libs-dev@openjdk.org swing client-libs-dev@openjdk.org