On Mon, 31 May 2021 15:02:57 GMT, Weijun Wang <wei...@openjdk.org> wrote:

>> Please review this implementation of [JEP 
>> 411](https://openjdk.java.net/jeps/411).
>> 
>> The code change is divided into 3 commits. Please review them one by one.
>> 
>> 1. 
>> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>>  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. 
>> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>>  Manual changes to several files so that the next commit can be generated 
>> programatically.
>> 3. 
>> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>>  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 
>> https://github.com/openjdk/jdk/pull/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.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   default behavior reverted to allow

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.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4073

Reply via email to