Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v4]

[Weijun Wang]

On Thu, 28 Apr 2022 02:33:49 GMT, Mark Powers  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8285504
>> 
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for 
>> a single code review, so it was decided to split into smaller chunks. This 
>> is one such chunk: 
>> 
>> open/src/java.base/share/classes/java/net
>
> Mark Powers has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains eight additional 
> commits since the last revision:
> 
>  - Merge
>  - Max and Brad comments
>  - jaikiran comments
>  - Alan Bateman comments
>  - second iteration
>  - Merge
>  - Merge
>  - first iteration

I made some wrong suggestions earlier.

src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 70:

> 68: String type;
> 69: type = GetPropertyAction.privilegedGetProperty(
> 70: "ssl.KeyManagerFactory.algorithm");

So sorry I got it wrong here, this is a security property. 
`GetPropertyAction.privilegedGetProperty` is for system properties.

src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 92:

> 90: static String getSecurityProperty(final String name) {
> 91: return AccessController.doPrivileged((PrivilegedAction) 
> () -> {
> 92: return Security.getProperty(name);

I assume we still need to do the if-empty-then-null conversion?

src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 82:

> 80: String type;
> 81: type = GetPropertyAction.privilegedGetProperty(
> 82: "ssl.TrustManagerFactory.algorithm");

Sorry I got it wrong here, this is a security property.

-

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

Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

[Weijun Wang]

On Tue, 26 Apr 2022 20:40:50 GMT, Bradford Wetmore  wrote:

>> Wasn't there another bug to address this?
>
> Perhaps as part of 
> [JDK-6725221](https://bugs.openjdk.java.net/browse/JDK-6725221)?

No problem.

-

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

Re: RFR: JDK-8285504 Minor cleanup could be done in javax.net [v2]

[Weijun Wang]

On Tue, 26 Apr 2022 00:27:43 GMT, Mark Powers  wrote:

>> https://bugs.openjdk.java.net/browse/JDK-8285504
>> 
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for 
>> a single code review, so it was decided to split into smaller chunks. This 
>> is one such chunk: 
>> 
>> open/src/java.base/share/classes/java/net
>
> Mark Powers has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Alan Bateman comments

Looks fine. Some small comments.

src/java.base/share/classes/javax/net/ssl/KeyManagerFactory.java line 69:

> 67: String type;
> 68: type = AccessController.doPrivileged((PrivilegedAction) 
> () ->
> 69: Security.getProperty("ssl.KeyManagerFactory.algorithm"));

We can probably use 
`sun.security.action.GetPropertyAction::privilegedGetProperty`. This is inside 
`java.base` so that class is always available.

src/java.base/share/classes/javax/net/ssl/SSLSocketFactory.java line 96:

> 94: s = s.trim();
> 95: if (s.isEmpty()) {
> 96: s = null;

According to the implementation of `Security::getProperty`. The return value is 
already trimmed.

src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 81:

> 79: String type;
> 80: type = AccessController.doPrivileged((PrivilegedAction) 
> () ->
> 81: Security.getProperty( "ssl.TrustManagerFactory.algorithm"));

Another `GetPropertyAction::privilegedGetProperty` candidate.

-

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

Re: RFR: 8282293: Domain value for system property jdk.https.negotiate.cbt should be case-insensitive [v3]

[Weijun Wang]

On Tue, 22 Mar 2022 07:51:18 GMT, Sibabrata Sahoo  wrote:

>> Domain value for system property jdk.https.negotiate.cbt  is 
>> case-insensitive now. Included Test has been updated to address the change.
>
> Sibabrata Sahoo has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update AbstractDelegateHttpsURLConnection.java

Marked as reviewed by weijun (Reviewer).

-

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

Re: RFR: 8282293: Domain value for system property jdk.https.negotiate.cbt should be case-insensitive [v2]

[Weijun Wang]

On Thu, 10 Mar 2022 05:59:14 GMT, Sibabrata Sahoo  wrote:

>> Domain value for system property jdk.https.negotiate.cbt  is 
>> case-insensitive now. Included Test has been updated to address the change.
>
> Sibabrata Sahoo has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update HttpsCB.java

src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
 line 338:

> 336: return true;
> 337: }
> 338: String afterWildCard = domain.substring(1);

`domain` could be an empty string if the property value is "domain:a,,b". I 
know it's invalid but at least let's try our best to avoid a runtime exception. 
In fact, why is this variable necessary? It looks like `regionMatches` allows 
you to compare ...er... regions.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v5]

[Weijun Wang]

On Mon, 14 Mar 2022 13:26:34 GMT, Michael McMahon  wrote:

>> Hi,
>> 
>> Could I get the following change reviewed please, which is to disable the 
>> MD5 message digest algorithm by default in the HTTP Digest authentication 
>> mechanism? The algorithm can be opted into by setting a new system property 
>> "http.auth.digest.reEnabledAlgs" to include the value MD5. The change also 
>> updates the Digest authentication implementation to use some of the more 
>> secure features defined in RFC7616, such as username hashing and additional 
>> digest algorithms like SHA256 and SHA512-256.
>> 
>> - Michael
>
> Michael McMahon has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains 17 additional 
> commits since the last revision:
> 
>  - Merge branch 'master' into md5
>  - update after third review round
>  - removed swp file
>  - update after second review round
>  - update
>  - update after first review round
>  - fix whitespace
>  - update property name. add documentation
>  - fixed one more test
>  - fixed up existing tests using digest auth
>  - ... and 7 more: 
> https://git.openjdk.java.net/jdk/compare/4bef4cc9...c55fdd94

LGTM now. It will be even nicer if the known answer tests in RFC 7616 can be 
included.

-

Marked as reviewed by weijun (Reviewer).

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]

[Weijun Wang]

On Thu, 10 Mar 2022 16:50:05 GMT, Michael McMahon  wrote:

>> src/java.base/share/classes/java/net/doc-files/net-properties.html line 234:
>> 
>>> 232: in the {@code java.security} properties file and currently 
>>> comprises {@code MD5} and
>>> 233: {@code SHA-1}. If it is still required to use one of these 
>>> algorithms, then they can be
>>> 234: re-enabled by setting this property to a comma separated list 
>>> of the algorithm names.
>> 
>> Can we use "re-enabled" in the property name? To me, the name "enabled" 
>> sounds like all enabled algorithms are listed here.
>
> Okay, I'm suggesting "http.auth.digest.reEnabledAlgorithms" now. 
> Hopefully we can stick with that.

Property name not updated yet.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v3]

[Weijun Wang]

On Fri, 11 Mar 2022 17:37:44 GMT, Michael McMahon  wrote:

>> Hi,
>> 
>> Could I get the following change reviewed please, which is to disable the 
>> MD5 message digest algorithm by default in the HTTP Digest authentication 
>> mechanism? The algorithm can be opted into by setting a new system property 
>> "http.auth.digest.reEnabledAlgs" to include the value MD5. The change also 
>> updates the Digest authentication implementation to use some of the more 
>> secure features defined in RFC7616, such as username hashing and additional 
>> digest algorithms like SHA256 and SHA512-256.
>> 
>> - Michael
>
> Michael McMahon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   update after second review round

Please remove `src/java.base/share/classes/java/util/.Random.java.swp`.

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java 
line 430:

> 428: algorithm = "MD5";  // The default, accoriding to rfc2069
> 429: }
> 430: var oid = 
> KnownOIDs.findMatch(algorithm.toUpperCase(Locale.ROOT));

No need to call `toUpperCase`. `findMatch` already called that. On the other 
hand, we don't support the name `SHA-512-256`, and if you translate it to 
stdName here there is no need to do it again in `computeUserhash`.

-

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

Re: RFR: 8280400: JDK 19 L10n resource files update - msgdrop 10 [v2]

[Weijun Wang]

On Thu, 10 Mar 2022 17:55:44 GMT, Alisen Chung  wrote:

>> msg drop for jdk19, Mar 9, 2022
>
> Alisen Chung has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   moved CurrencyNames changes to jdk.localedata

The security related changes look fine, except for the year 2021.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]

[Weijun Wang]

On Wed, 9 Mar 2022 14:23:38 GMT, Michael McMahon  wrote:

>> Hi,
>> 
>> Could I get the following change reviewed please, which is to disable the 
>> MD5 message digest algorithm by default in the HTTP Digest authentication 
>> mechanism? The algorithm can be opted into by setting a new system property 
>> "http.auth.digest.reEnabledAlgs" to include the value MD5. The change also 
>> updates the Digest authentication implementation to use some of the more 
>> secure features defined in RFC7616, such as username hashing and additional 
>> digest algorithms like SHA256 and SHA512-256.
>> 
>> - Michael
>
> Michael McMahon has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - update
>  - update after first review round

src/java.base/share/classes/java/net/doc-files/net-properties.html line 234:

> 232: in the {@code java.security} properties file and currently 
> comprises {@code MD5} and
> 233: {@code SHA-1}. If it is still required to use one of these 
> algorithms, then they can be
> 234: re-enabled by setting this property to a comma separated list of 
> the algorithm names.

Can we use "re-enabled" in the property name? To me, the name "enabled" sounds 
like all enabled algorithms are listed here.

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java 
line 68:

> 66: 
> 67: private static final String secPropName =
> 68: "jdk.httpdigest.defaultDisabledAlgorithms";

How about a dot between "http" and "digest"?

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java 
line 76:

> 74: // used for proxy connections, or plain text http server connections
> 75: 
> 76: private static final Set defDisabledAlgs = getDefaultAlgs();

Can we move this field into a local variable in the static block near line 120? 
After all it's the `disabledAlgs` field finally gets used and the 
`defDisabledAlgs` is only used to calculate it?

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java 
line 78:

> 76: private static final Set defDisabledAlgs = getDefaultAlgs();
> 77: 
> 78: private static Set getDefaultAlgs() {

How about rename the method to include "disabled"?

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java 
line 693:

> 691: }
> 692: md.update(src.getBytes(charset));
> 693: if (passwd != null) {

[RFC7616 ](https://datatracker.ietf.org/doc/html/rfc7616#section-4) says the 
password should also support unicode. It's OK we use the old code for 
compatibility reason.

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java 
line 701:

> 699: }
> 700: byte[] digest = md.digest();
> 701: StringBuilder res = new StringBuilder(digest.length * 2);

Can we use `HexFormat` to encode the bytes?

src/java.base/share/conf/security/java.security line 711:

> 709: # separated list of algorithms to be allowed.
> 710: #
> 711: jdk.httpdigest.defaultDisabledAlgorithms = MD5, MD-5, SHA1, SHA-1

I haven't seen people using "MD-5". It's also not an alias of "MD5" in our own 
security providers. On the other hand, we support both "SHA1" and "SHA" (and 
its OID) as aliases of "SHA-1". So, either we list all these aliases here, or 
we only put the standard names here and "canonicalize" the name when we see 
one. `sun.security.util.KnownOIDs.findMatch("SHA-1").stdName()` can be used.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]

[Weijun Wang]

On Thu, 10 Mar 2022 10:48:09 GMT, Michael McMahon  wrote:

>> Maybe `String.trim()` should be called on each element after splitting 
>> instead: you want to remove spaces before and after commas, not necessarily 
>> spaces within a name. "MD 5, SHA-256" probably shouldn't be parsed as 
>> "MD5,SHA-256".
>
> Okay.

`s.n.w.p.h.HttpURLConnection::schemesListToSet` uses `list.split("\\s*,\\s*")` 
which ignores multiple consecutive spaces as well. Can we make it a helper 
method somewhere?

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default [v2]

[Weijun Wang]

On Wed, 9 Mar 2022 14:23:38 GMT, Michael McMahon  wrote:

>> Hi,
>> 
>> Could I get the following change reviewed please, which is to disable the 
>> MD5 message digest algorithm by default in the HTTP Digest authentication 
>> mechanism? The algorithm can be opted into by setting a new system property 
>> "http.auth.digest.reEnabledAlgs" to include the value MD5. The change also 
>> updates the Digest authentication implementation to use some of the more 
>> secure features defined in RFC7616, such as username hashing and additional 
>> digest algorithms like SHA256 and SHA512-256.
>> 
>> - Michael
>
> Michael McMahon has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - update
>  - update after first review round

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java 
line 99:

> 97: // A net property which overrides the disabled set above.
> 98: private static final String enabledAlgPropName =
> 99: "http.auth.digest.enabledAlgorithms";

I'm not familiar with the practice of overriding a security property with a net 
property. Just FYI, in security libs, we often override a security property 
with a system property and we have a dedicated method for this at 
https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/java.base/share/classes/sun/security/util/SecurityProperties.java#L46.

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java 
line 232:

> 230: ? StandardCharsets.UTF_8
> 231: : StandardCharsets.ISO_8859_1;
> 232: }

Do you want to reject other values? According to the RFC, `utf-8` is the only 
valid one.

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java 
line 426:

> 424: algorithm = "MD5";  // The default, accoriding to rfc2069
> 425: }
> 426: algorithm = algorithm.toUpperCase();

Please use `toUpperCase(Locale.ROOT)` or `toUpperCase(Locale.ENGLISH)`.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Weijun Wang]

On Mon, 7 Mar 2022 14:22:58 GMT, Weijun Wang  wrote:

>> Okay, I'll double check that. I haven't found any server implementations of 
>> this feature to test with yet,
>
> 2nd test of https://datatracker.ietf.org/doc/html/rfc7616#section-3.9 is on 
> this algorithm, but it requires UTF-8 charset support and a way to provide a 
> predefined cnonce. If it's not worth modifying our implementation to create a 
> regression test, I think at least we can temporarily hack our own JDK and try 
> on it. And I think it's most likely true that this algorithm is using a 
> different initialization vector as Bernd pointed out.

As https://www.rfc-editor.org/errata_search.php?rfc=7616&rec_status=0 shows, 
that 2nd test in rfc7616 was wrong in the initial version as it used a 
truncated version of SHA-512. The real SHA-512/256 algorithm should be used.

$ jshell
jshell> import java.security.MessageDigest

jshell> 
HexFormat.of().formatHex(MessageDigest.getInstance("SHA-512").digest("J\u00e4s\u00f8n
 Doe:a...@example.org".getBytes("UTF-8")))
$2 ==> 
"488869477bf257147b804c45308cd62ac4e25eb717b12b298c79e62dcea254ec5211a6631b181289b4dd8c14890f38f04bff8a388106dabb900c6984ba592b6a"

jshell> 
HexFormat.of().formatHex(MessageDigest.getInstance("SHA-512/256").digest("J\u00e4s\u00f8n
 Doe:a...@example.org".getBytes("UTF-8")))
$3 ==> "793263caabb707a56211940d90411ea4a575adeccb7e360aeb624ed06ece9b0b"

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Weijun Wang]

On Mon, 7 Mar 2022 11:01:16 GMT, Michael McMahon  wrote:

>> src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java
>>  line 670:
>> 
>>> 668: if (truncate256) {
>>> 669: assert digest.length >= 32;
>>> 670: start = digest.length - 32;
>> 
>> Does this mean the left half is truncated? My understanding is that the 
>> right half should be.
>
> Okay, I'll double check that. I haven't found any server implementations of 
> this feature to test with yet,

2nd test of https://datatracker.ietf.org/doc/html/rfc7616#section-3.9 is on 
this algorithm, but it requires UTF-8 charset support and a way to provide a 
predefined cnonce. If it's not worth modifying our implementation to create a 
regression test, I think at least we can temporarily hack our own JDK and try 
on it. And I think it's most likely true that this algorithm is using a 
different initialization vector as Bernd pointed out.

-

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

Re: RFR: 8281561: Disable http DIGEST mechanism with MD5 by default

[Weijun Wang]

On Fri, 4 Mar 2022 09:37:21 GMT, Michael McMahon  wrote:

> Hi,
> 
> Could I get the following change reviewed please, which is to disable the MD5 
> message digest algorithm by default in the HTTP Digest authentication 
> mechanism? The algorithm can be opted into by setting a new system property 
> "http.auth.digest.reEnabledAlgs" to include the value MD5. The change also 
> updates the Digest authentication implementation to use some of the more 
> secure features defined in RFC7616, such as username hashing and additional 
> digest algorithms like SHA256 and SHA512-256.
> 
> - Michael

Several comments added. Also, I am reading rfc7616 and it will be really nice 
if we can include the 2 examples at 
https://datatracker.ietf.org/doc/html/rfc7616#section-3.9 as a known answer 
test. Of course this means you need a way to provide a hardcoded `cnonce` and 
it's probably not easy.

src/java.base/share/classes/java/net/doc-files/net-properties.html line 232:

> 230: includes {@code MD5} but other algorithms may be added in 
> future. If it is still
> 231: required to use one of these algorithms, then they can be 
> re-enabled by setting
> 232: this property to a comma separated list of the algorithm 
> names.

Is it necessary to emphasize that no whitespace is allowed around the comma in 
the property value? Or is it better to modify the implementation below to allow 
whitespaces? I notice that whitespace is allowed in some of the other 
properties. For example: 
https://github.com/openjdk/jdk/blob/de3113b998550021bb502cd6f766036fb8351e7d/src/java.base/share/classes/sun/net/www/protocol/http/HttpURLConnection.java#L228

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java 
line 75:

> 73: // A net property which overrides the disabled set above.
> 74: private static final String enabledAlgPropName = "http.auth.digest." +
> 75: "reEnabledAlgs";

Why not put the string on one line?

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java 
line 658:

> 656: // truncate256 means only use the last 256 bits of the digest (32 
> bytes)
> 657: private String encode(String src, char[] passwd, MessageDigest md, 
> boolean truncate256) {
> 658: md.update(src.getBytes(ISO_8859_1.INSTANCE));

Maybe we can support the "charset" parameter as well. The only allowed value is 
"UTF-8".

src/java.base/share/classes/sun/net/www/protocol/http/DigestAuthentication.java 
line 670:

> 668: if (truncate256) {
> 669: assert digest.length >= 32;
> 670: start = digest.length - 32;

Does this mean the left half is truncated? My understanding is that the right 
half should be.

-

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]

[Weijun Wang]

On Wed, 26 Jan 2022 16:25:24 GMT, Daniel Fuchs  wrote:

>> Michael McMahon has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   removed ^M from test
>
> test/jdk/sun/security/krb5/auto/HttpsCB.java line 120:
> 
>> 118: 
>> 119: boolean expected1 = Boolean.parseBoolean(args[0]);
>> 120: boolean expected2 = Boolean.parseBoolean(args[1]);
> 
> It might be better for future maintainers and readability if these two 
> variables could have better names, and possibly a comment to explain their 
> purpose. AFAIU it's the expected result of running with/without CBT - where 
> `true` means that the operation should succeed and `false` that it's expected 
> to fail with some exception...

Maybe `expectedCbtUrlResult` and `expectedNormalUrlResult`.

-

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v10]

[Weijun Wang]

On Wed, 26 Jan 2022 16:27:29 GMT, Daniel Fuchs  wrote:

>> Michael McMahon has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   removed ^M from test
>
> test/jdk/sun/security/krb5/auto/HttpsCB.java line 201:
> 
>> 199: return reader.readLine().equals(CONTENT);
>> 200: } catch (Exception e) {
>> 201: return false;
> 
> Should we log that we have received the excepted exception here? Shouldn't 
> the catch clause only list the exceptions that we are expecting?

It's `java.net.SocketException: Unexpected end of file from server`. Does not 
include any CBT words so don't know if it's worth parsing.

-

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v7]

[Weijun Wang]

On Mon, 24 Jan 2022 22:11:51 GMT, Michael McMahon  wrote:

>> Hi,
>> 
>> This change adds Channel Binding Token (CBT) support to HTTPS 
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, 
>> Kerberos) authentication scheme. When enabled, the implementation 
>> preemptively includes a CBT with authentication requests over Kerberos. The 
>> feature is enabled as follows:
>> 
>> A system property "jdk.spnego.cbt" is defined which can have the values 
>> "never" (default), which means the feature is disabled, "always", which 
>> means the CBT is included for all https Negotiate authentications, or it can 
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of 
>> domains/hosts where the feature is enabled, and disabled everywhere else. In 
>> the given example, the CBT would be included in authentication requests for 
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
>> sub-domains.
>> 
>> A test will be added separately to the implementation.
>> 
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>> 
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   more updates

Looks good to me. Only several wording and style suggestions.

I know you are asking SQE to create a security infra test, but I'll see if I 
can contribute a regression test. Don't wait for me.

src/java.base/share/classes/java/net/doc-files/net-properties.html line 223:

> 221: 
> 222:   "never". This is also the default value if the property 
> is not set. In this case,
> 223:   CBT's are never sent.

Typo, "CBTs"?

src/java.base/share/classes/java/net/doc-files/net-properties.html line 224:

> 222:   "never". This is also the default value if the property 
> is not set. In this case,
> 223:   CBT's are never sent.
> 224:   "always". CBTs are sent for all Kerberos authentication 
> attempts over HTTPS.

Shall we remove "Kerberos"? Or we can use "Kerberos or Negotiate".

src/java.base/share/classes/sun/net/www/protocol/https/AbstractDelegateHttpsURLConnection.java
 line 1:

> 1: /**

This is not a doc comment.

src/java.security.jgss/share/classes/sun/net/www/protocol/http/spnego/NegotiatorImpl.java
 line 124:

> 122: try {
> 123: init(hci);
> 124: } catch (GSSException | ChannelBindingException  e) {

Two spaces before "e".

-

Marked as reviewed by weijun (Reviewer).

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]

[Weijun Wang]

On Mon, 24 Jan 2022 15:54:01 GMT, Michael McMahon  wrote:

>> src/java.base/share/classes/sun/security/util/TlsChannelBinding.java line 
>> 100:
>> 
>>> (failed to retrieve contents of file, check the PR for context)
>> I think this method should stay here. Suppose one day the CBT type is 
>> configurable for HTTPS we'll have to get it back. Of course we will need to 
>> update the message to avoid talking about LDAP.
>
> So, where should the two constant Strings go? It doesn't feel like they 
> belong in java.base since they are JNDI/SASL related, and we can't have a 
> method in `java.base` depending on code in other modules?

The 2 strings should be on the LDAP side. This method does not really depend on 
the strings except for mentioning one in the exception message. We can just 
rewrite it into `"Illegal channel binding type: " + cbType`.

-

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]

[Weijun Wang]

On Mon, 24 Jan 2022 13:54:12 GMT, Daniel Fuchs  wrote:

>> Michael McMahon has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains eight additional 
>> commits since the last revision:
>> 
>>  - fixed failing test issue and update for latest comments
>>  - Merge branch 'master' into spnego
>>  - added root cause to NamingException
>>  - more tidy-up
>>  - removed sasl module dependency and added SaslException cause
>>  - changes after first review round
>>  - cleanup but still no test. Will be added in closed repo
>>  - First version of fix. No test and feature enabled always.
>
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 260:
> 
>> 258:  * @throws ChannelBindingException
>> 259:  */
>> 260: private static TlsChannelBindingType parseType(String cbType) 
>> throws ChannelBindingException {
> 
> Maybe this method could throw NamingException directly now? That would avoid 
> wrapping CBE into NamingException?

My opinion is this method should be put back.

-

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v4]

[Weijun Wang]

On Fri, 21 Jan 2022 15:40:16 GMT, Daniel Fuchs  wrote:

>> Michael McMahon has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   more tidy-up
>
> src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 144:
> 
>> 142: } catch (ChannelBindingException e) {
>> 143: throw new NamingException(e.getMessage());
>> 144: }
> 
> Should we call initCause here and above? I see that we do call initCause in 
> NegotiatorImpl.java.
> On the one hand it's better for diagnostic. On the other hand it exposes a 
> module-internal exception class which is not great. Or maybe we should set 
> the cause of the CBE as the cause of NamingException.

As long as the spec has not dictated what the cause should be, I don't care if 
the exception type is internal or not.

-

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v6]

[Weijun Wang]

On Mon, 24 Jan 2022 13:36:47 GMT, Michael McMahon  wrote:

>> Hi,
>> 
>> This change adds Channel Binding Token (CBT) support to HTTPS 
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, 
>> Kerberos) authentication scheme. When enabled, the implementation 
>> preemptively includes a CBT with authentication requests over Kerberos. The 
>> feature is enabled as follows:
>> 
>> A system property "jdk.spnego.cbt" is defined which can have the values 
>> "never" (default), which means the feature is disabled, "always", which 
>> means the CBT is included for all https Negotiate authentications, or it can 
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of 
>> domains/hosts where the feature is enabled, and disabled everywhere else. In 
>> the given example, the CBT would be included in authentication requests for 
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
>> sub-domains.
>> 
>> A test will be added separately to the implementation.
>> 
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>> 
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request with a new target base due to a 
> merge or a rebase. The incremental webrev excludes the unrelated changes 
> brought in by the merge/rebase. The pull request contains eight additional 
> commits since the last revision:
> 
>  - fixed failing test issue and update for latest comments
>  - Merge branch 'master' into spnego
>  - added root cause to NamingException
>  - more tidy-up
>  - removed sasl module dependency and added SaslException cause
>  - changes after first review round
>  - cleanup but still no test. Will be added in closed repo
>  - First version of fix. No test and feature enabled always.

src/java.base/share/classes/sun/security/util/TlsChannelBinding.java line 100:

> (failed to retrieve contents of file, check the PR for context)
I think this method should stay here. Suppose one day the CBT type is 
configurable for HTTPS we'll have to get it back. Of course we will need to 
update the message to avoid talking about LDAP.

-

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v5]

[Weijun Wang]

On Fri, 21 Jan 2022 16:02:29 GMT, Michael McMahon  wrote:

>> Hi,
>> 
>> This change adds Channel Binding Token (CBT) support to HTTPS 
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, 
>> Kerberos) authentication scheme. When enabled, the implementation 
>> preemptively includes a CBT with authentication requests over Kerberos. The 
>> feature is enabled as follows:
>> 
>> A system property "jdk.spnego.cbt" is defined which can have the values 
>> "never" (default), which means the feature is disabled, "always", which 
>> means the CBT is included for all https Negotiate authentications, or it can 
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of 
>> domains/hosts where the feature is enabled, and disabled everywhere else. In 
>> the given example, the CBT would be included in authentication requests for 
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
>> sub-domains.
>> 
>> A test will be added separately to the implementation.
>> 
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>> 
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   added root cause to NamingException

Please move

// TLS channel binding type property
public static final String CHANNEL_BINDING_TYPE =
"com.sun.jndi.ldap.tls.cbtype";
// internal TLS channel binding property
public static final String CHANNEL_BINDING =
"jdk.internal.sasl.tlschannelbinding";

outside of the `TlsChannelBinding` class.

-

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v5]

[Weijun Wang]

On Fri, 21 Jan 2022 16:02:29 GMT, Michael McMahon  wrote:

>> Hi,
>> 
>> This change adds Channel Binding Token (CBT) support to HTTPS 
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, 
>> Kerberos) authentication scheme. When enabled, the implementation 
>> preemptively includes a CBT with authentication requests over Kerberos. The 
>> feature is enabled as follows:
>> 
>> A system property "jdk.spnego.cbt" is defined which can have the values 
>> "never" (default), which means the feature is disabled, "always", which 
>> means the CBT is included for all https Negotiate authentications, or it can 
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of 
>> domains/hosts where the feature is enabled, and disabled everywhere else. In 
>> the given example, the CBT would be included in authentication requests for 
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
>> sub-domains.
>> 
>> A test will be added separately to the implementation.
>> 
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>> 
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   added root cause to NamingException

src/java.base/share/classes/java/net/doc-files/net-properties.html line 220:

> 218:  This controls the generation and sending of TLS channel binding tokens 
> (CBT) when Kerberos 
> 219: or the Negotiate authentication scheme using Kerberos are 
> employed over HTTPS with 
> 220: {@code HttpsURLConnection}. There are three possible 
> settings:

You can probably mention here that the 'tls-server-end-point' Channel Binding 
Type defined in RFC 5929 is used.

-

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos [v2]

[Weijun Wang]

On Wed, 19 Jan 2022 22:20:47 GMT, Michael McMahon  wrote:

>> Hi,
>> 
>> This change adds Channel Binding Token (CBT) support to HTTPS 
>> (java.net.HttpsURLConnection) when used with the Negotiate (SPNEGO, 
>> Kerberos) authentication scheme. When enabled, the implementation 
>> preemptively includes a CBT with authentication requests over Kerberos. The 
>> feature is enabled as follows:
>> 
>> A system property "jdk.spnego.cbt" is defined which can have the values 
>> "never" (default), which means the feature is disabled, "always", which 
>> means the CBT is included for all https Negotiate authentications, or it can 
>> take the form "domain:a,b.c,*.d.com" which is a comma separated list of 
>> domains/hosts where the feature is enabled, and disabled everywhere else. In 
>> the given example, the CBT would be included in authentication requests for 
>> hosts "a", "b.c" and all hosts under the domain "d.com" and all of its 
>> sub-domains.
>> 
>> A test will be added separately to the implementation.
>> 
>> Bug report: https://bugs.openjdk.java.net/browse/JDK-8279842
>> 
>> Thanks,
>> Michael
>
> Michael McMahon has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   changes after first review round

src/java.naming/share/classes/com/sun/jndi/ldap/sasl/LdapSasl.java line 133:

> 131: 
> (String)env.get(TlsChannelBinding.CHANNEL_BINDING_TYPE));
> 132: } catch (ChannelBindingException e) {
> 133: throw new SaslException(e.getMessage());

How about setting `e` as cause of new exception? In `TlsChannelBinding.java` 
the when the original exception was thrown (the 2nd throws) there was a cause.

src/java.security.jgss/share/classes/module-info.java line 36:

> 34: module java.security.jgss {
> 35: requires java.naming;
> 36: requires java.security.sasl;

Can this be removed now?

-

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos

[Weijun Wang]

On Fri, 14 Jan 2022 10:18:50 GMT, Daniel Fuchs  wrote:

>> This is what was intended (equivalent)
>> 
>> `if (s ==null || (s!="always" && s!="never" && !s.startsWith("domain")))`
>
> Argh - you're right I missed the fact that the 3 expressions where included 
> in parenthesis. I read it as 
> 
> ! (s.equals("always")) || ...

Shall we log a message if the value is not one of the 3 forms?

-

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos

[Weijun Wang]

On Fri, 14 Jan 2022 18:40:41 GMT, Michael McMahon  wrote:

>> src/java.base/share/classes/sun/net/www/http/HttpClient.java line 152:
>> 
>>> 150:  * If enabled (for a particular destination) then SPNEGO 
>>> authentication requests will include
>>> 151:  * a channel binding token for the destination server. The default 
>>> behavior and setting for the
>>> 152:  * property is "never"
>> 
>> Maybe this description should be added to 
>> `src/java.base//share/classes/java/net/doc-files/net-properties.html` too?
>
> It's actually a purely system property rather than a Net property at the 
> moment (same as the other spnego ones). Maybe, I should convert them all to 
> net properties, so they can be documented/set in that file?

This system property should only be used for TLS, and the CBT can be used in 
both the SPNEGO mechanism and the Kerberos 5 mechanism. Therefore I suggest the 
name should probably contain "tls" (or maybe "https") and "negotiate".

BTW, will you reuse this system property if we decide to support CBT in NTLM as 
well?

-

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

Re: RFR: 8279842: HTTPS Channel Binding support for Java GSS/Kerberos

[Weijun Wang]

On Fri, 14 Jan 2022 18:42:08 GMT, Michael McMahon  wrote:

>> src/java.security.jgss/share/classes/module-info.java line 36:
>> 
>>> 34: module java.security.jgss {
>>> 35: requires java.naming;
>>> 36: requires java.security.sasl;
>> 
>> Someone from security-dev should probably review this and validate that this 
>> is OK. I'm also a bit uncomfortable that we require a class from 
>> `com.sun.jndi.ldap.sasl` even though `java.naming` is already required by 
>> `java.security.jgss` - so maybe this is OK.
>
> Yes. I would like the security team to validate this.

I suggest moving the `TlsChannelBinding` class into 
`java.base/sun.security.util` since it's not only used by LDAP anymore. You 
might need to modify the types of exceptions thrown in the class and move the 2 
final strings to some other class inside `java.security.sasl`.

-

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

Re: RFR: 8275079: Remove unnecessary conversion to String in java.net.http

[Weijun Wang]

On Sat, 2 Oct 2021 20:05:37 GMT, Andrey Turbanov  wrote:

> Cleanup unnecessary String.valueOf calls (and similar) when conversion will 
> happen implicitly anyway

Some small comments.

src/java.net.http/share/classes/jdk/internal/net/http/Http1AsyncReceiver.java 
line 738:

> 736: if (flowTag != null) {
> 737: dbgTag = tag = "Http1AsyncReceiver("+ flowTag + ")";
> 738: } else {

If the only use of `flowTag` is `flow` in string, it looks like `flow` is 
enough. i.e.


if (flow != null) {
dbgTag = tag = "Http1AsyncReceiver("+ flow + ")";
}

src/java.net.http/share/classes/jdk/internal/net/http/Http1Response.java line 
831:

> 829: @Override
> 830: public String toString() {
> 831: return super.toString() + "/parser=" + parser;

Can we use `super` instead of `super.toString()`?

-

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

Re: RFR: 8274949: Use String.contains() instead of String.indexOf() in java.base

[Weijun Wang]

On Fri, 17 Sep 2021 08:56:47 GMT, Andrey Turbanov 
 wrote:

> String.contains was introduced in Java 5.
> Some code in java.base still uses old approach with `String.indexOf` to check 
> if String contains specified substring.
> I propose to migrate such usages. Makes code shorter and easier to read.

security-related change looks fine.

-

Marked as reviewed by weijun (Reviewer).

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

Re: RFR: 8274237: Replace 'for' cycles with iterator with enhanced-for in java.base

[Weijun Wang]

On Thu, 23 Sep 2021 20:42:48 GMT, Andrey Turbanov 
 wrote:

> There are few places in code where manual `for` loop is used with Iterator to 
> iterate over Collection.
> Instead of manual `for` cycles it's preferred to use enhanced-for cycle 
> instead: it's less verbose, makes code easier to read and it's less 
> error-prone.
> Sometimes we even don't need cycle at all: we can just create one ArrayList 
> as a copy of another.
> It doesn't have any performance impact: java compiler generates similar code 
> when compiling enhanced-for cycle.
> This is continuation of 
> [JDK-8273261](https://bugs.openjdk.java.net/browse/JDK-8273261)

Changes in the 3 security-related files also look fine. Thanks.

I've also added a `noreg-cleanup` label to the JBS bug.

-

Marked as reviewed by weijun (Reviewer).

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

Re: RFR: 8274079: Cleanup unnecessary calls to Throwable.initCause() in java.base module

[Weijun Wang]

On Thu, 16 Sep 2021 19:03:26 GMT, Andrey Turbanov 
 wrote:

> Pass "cause" exception as constructor parameter is shorter and easier to read.

Looks fine. Thanks.

-

Marked as reviewed by weijun (Reviewer).

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

Re: RFR: 8272805: Avoid looking up standard charsets

[Weijun Wang]

On Sun, 22 Aug 2021 02:53:44 GMT, Sergey Bylokhov  wrote:

> This is the continuation of JDK-8233884, JDK-8271456, and JDK-8272120.
> 
> In many places standard charsets are looked up via their names, for example:
> absolutePath.getBytes("UTF-8");
> 
> This could be done more efficiently(up to x20 time faster) with use of 
> java.nio.charset.StandardCharsets:
> absolutePath.getBytes(StandardCharsets.UTF_8);
> 
> The later variant also makes the code cleaner, as it is known not to throw 
> UnsupportedEncodingException in contrary to the former variant.
> 
> This change includes:
>  * demo/utils
>  * jdk.xx packages
>  * Some places were missed in the previous changes. I have found it by 
> tracing the calls to the Charset.forName() by executing tier1,2,3 and desktop 
> tests.
> 
> Some performance discussion: https://github.com/openjdk/jdk/pull/5063
> 
> Code excluded in this fix: the Xerces library(should be fixed upstream), 
> J2DBench(should be compatible to 1.4), some code in the network(the change 
> there are not straightforward, will do it later).
> 
> Tested by the tier1/tier2/tier3 tests on Linux/Windows/macOS.

The security related change looks fine to me.

-

Marked as reviewed by weijun (Reviewer).

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

Integrated: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

[Weijun Wang]

On Mon, 28 Jun 2021 18:03:56 GMT, Weijun Wang  wrote:

> More refactoring to limit the scope of `@SuppressWarnings` annotations.
> 
> Sometimes I introduce new methods. Please feel free to suggest method names 
> you like to use.
> 
> Note: this is copied from https://github.com/openjdk/jdk17/pull/152.

This pull request has now been integrated.

Changeset: e9b2c058
Author:Weijun Wang 
URL:   
https://git.openjdk.java.net/jdk/commit/e9b2c058a4ed5de29b991360f78fc1c5263c9268
Stats: 293 lines in 21 files changed: 165 ins; 64 del; 64 mod

8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

Reviewed-by: lancea, naoto

-

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

RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

[Weijun Wang]

More refactoring to limit the scope of `@SuppressWarnings` annotations.

Sometimes I introduce new methods. Please feel free to suggest method names you 
like to use.

Note: this is copied from https://github.com/openjdk/jdk17/pull/152.

-

Commit messages:
 - copy all code change from jdk17

Changes: https://git.openjdk.java.net/jdk/pull/4615/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4615&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8269409
  Stats: 293 lines in 21 files changed: 165 ins; 64 del; 64 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4615.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4615/head:pull/4615

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

[jdk17] Withdrawn: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

[Weijun Wang]

On Fri, 25 Jun 2021 20:04:37 GMT, Weijun Wang  wrote:

> More refactoring to limit the scope of `@SuppressWarnings` annotations.
> 
> Sometimes I introduce new methods. Please feel free to suggest method names 
> you like to use.

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]

[Weijun Wang]

On Mon, 28 Jun 2021 12:20:38 GMT, Daniel Fuchs  wrote:

>> This cast is only to tell the compiler which overloaded method to call, and 
>> I don't think there will be a real cast at runtime. It might look a little 
>> ugly but extracting it into a variable declaration/definition plus a new 
>> `initStatic` method seems not worth doing, IMHO.
>
> Why not simply declare a local variable in the static initializer below?
> 
> 
> private static final long CURRENT_PID;
> private static final boolean ALLOW_ATTACH_SELF;
> static {
> PrivilegedAction pa = ProcessHandle::current;
> @SuppressWarnings("removal")
> long pid = AccessController.doPrivileged(pa).pid();
> CURRENT_PID = pid;
> String s = VM.getSavedProperty("jdk.attach.allowAttachSelf");
> ALLOW_ATTACH_SELF = "".equals(s) || Boolean.parseBoolean(s);
> }

I've just pushed a commit with a different fix:

private static final long CURRENT_PID = pid();

@SuppressWarnings("removal")
private static long pid() {
PrivilegedAction pa = () -> ProcessHandle.current();
return AccessController.doPrivileged(pa).pid();
}

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v3]

[Weijun Wang]

> More refactoring to limit the scope of `@SuppressWarnings` annotations.
> 
> Sometimes I introduce new methods. Please feel free to suggest method names 
> you like to use.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  one more refinement

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/152/files
  - new: https://git.openjdk.java.net/jdk17/pull/152/files/774eb9da..2e4a8ba7

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=152&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=152&range=01-02

  Stats: 6 lines in 1 file changed: 4 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/152.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/152/head:pull/152

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]

[Weijun Wang]

On Fri, 25 Jun 2021 23:40:27 GMT, Weijun Wang  wrote:

>> More refactoring to limit the scope of `@SuppressWarnings` annotations.
>> 
>> Sometimes I introduce new methods. Please feel free to suggest method names 
>> you like to use.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   one more

I'm going to move this to jdk18.

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]

[Weijun Wang]

On Sat, 26 Jun 2021 16:53:30 GMT, Alan Bateman  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   one more
>
> src/jdk.attach/share/classes/sun/tools/attach/HotSpotVirtualMachine.java line 
> 53:
> 
>> 51: private static final long CURRENT_PID = 
>> AccessController.doPrivileged(
>> 52: (PrivilegedAction) 
>> ProcessHandle::current).pid();
>> 53: 
> 
> The original code separated out the declaration of the PrivilegedAction to 
> avoid this cast. If you move the code from the original static initializer 
> into a static method that it called from initializer then it might provide 
> you with a cleaner way to refactor this. There are several other places in 
> this patch that could do with similar cleanup.

This cast is only to tell the compiler which overloaded method to call, and I 
don't think there will be a real cast at runtime. It might look a little ugly 
but extracting it into a variable declaration/definition plus a new 
`initStatic` method seems not worth doing, IMHO.

-

PR: https://git.openjdk.java.net/jdk17/pull/152

Re: [jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K [v2]

[Weijun Wang]

> More refactoring to limit the scope of `@SuppressWarnings` annotations.
> 
> Sometimes I introduce new methods. Please feel free to suggest method names 
> you like to use.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  one more

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/152/files
  - new: https://git.openjdk.java.net/jdk17/pull/152/files/d8b384df..774eb9da

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=152&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=152&range=00-01

  Stats: 2 lines in 1 file changed: 1 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/152.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/152/head:pull/152

PR: https://git.openjdk.java.net/jdk17/pull/152

[jdk17] RFR: 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

[Weijun Wang]

More refactoring to limit the scope of `@SuppressWarnings` annotations.

Sometimes I introduce new methods. Please feel free to suggest method names you 
like to use.

-

Commit messages:
 - 8269409: Post JEP 411 refactoring: core-libs with maximum covering > 10K

Changes: https://git.openjdk.java.net/jdk17/pull/152/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk17&pr=152&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8269409
  Stats: 289 lines in 21 files changed: 161 ins; 64 del; 64 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/152.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/152/head:pull/152

PR: https://git.openjdk.java.net/jdk17/pull/152

Withdrawn: 8268349: Provide more detail in JEP 411 warning messages

[Weijun Wang]

On Mon, 7 Jun 2021 20:42:53 GMT, Weijun Wang  wrote:

> More loudly and precise warning messages when a security manager is either 
> enabled at startup or installed at runtime.

This pull request has been closed without being integrated.

-

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

Re: RFR: 8268349: Provide more detail in JEP 411 warning messages

[Weijun Wang]

On Tue, 8 Jun 2021 17:15:29 GMT, Alan Bateman  wrote:

>> I thought about that but not sure of performance impact. Is the worst 
>> problem that more than one warnings will be printed for a single caller? 
>> It's not really harmless.
>> 
>> As for the frame, if the warning message only contain the caller class name 
>> and its code source, why is it worth using a key of multiple frames? The 
>> message will look the same.
>
>> I thought about that but not sure of performance impact. Is the worst 
>> problem that more than one warnings will be printed for a single caller? 
>> It's not really harmless.
>> 
>> As for the frame, if the warning message only contain the caller class name 
>> and its code source, why is it worth using a key of multiple frames? The 
>> message will look the same.
> 
> WeakHashMap access needs synchronization. Whether we need to cache to avoid 
> excessive warnings isn't clear. If the SM is enabled once and never 
> disabled/re-enabled then caching isn't interesting.  On the other hand if 
> there are programs that are enabling/disabling to execute subsets of code 
> then maybe it is. Maybe we should just drop this and see if there is any 
> feedback on the repeated warning?

Not sure what you meant by "WeakHashMap access synchronization", it's just a 
noun without any other parts. Do you think synchronization is necessary?

For the cache, I'm OK to drop it at the moment.

-

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

Re: RFR: 8268349: Provide more detail in JEP 411 warning messages

[Weijun Wang]

On Tue, 8 Jun 2021 06:11:17 GMT, Alan Bateman  wrote:

>> More loudly and precise warning messages when a security manager is either 
>> enabled at startup or installed at runtime.
>
> src/java.base/share/classes/java/lang/System.java line 331:
> 
>> 329: 
>> 330: // Remember original System.err. setSecurityManager() warning goes 
>> here
>> 331: private static PrintStream oldErrStream = null;
> 
> I assume this should needs to be volatile and @Stable. I think we need a 
> better name for it too.

Will add the modifiers. How about "originalErr"?

> src/java.base/share/classes/java/lang/System.java line 336:
> 
>> 334: // Remember callers of setSecurityManager() here so that warning
>> 335: // is only printed once for each different caller
>> 336: final static Map callersOfSSM = new 
>> WeakHashMap<>();
> 
> You can't use a WeakHashMap without synchronization but a big question here 
> is whether a single caller frame is sufficient. If I were doing this then I 
> think I would capture the hash of a number of stack frames to create a better 
> filter.

I thought about that but not sure of performance impact. Is the worst problem 
that more than one warnings will be printed for a single caller? It's not 
really harmless.

As for the frame, if the warning message only contain the caller class name and 
its code source, why is it worth using a key of multiple frames? The message 
will look the same.

> src/java.base/share/classes/java/lang/System.java line 2219:
> 
>> 2217: WARNING: java.lang.SecurityManager is 
>> deprecated and will be removed in a future release
>> 2218: WARNING: -Djava.security.manager=%s 
>> will have no effect when java.lang.SecurityManager is removed
>> 2219: """, smProp);
> 
> Raw strings may be useful here but means the lines length are inconsistent 
> and makes it too hard to look at side by side diffs now.

I understand what you mean when I switch to Split View.  While I can extract 
the lines to a method, I somehow think it's not worth doing because for each 
type of warning the method is only called once.

-

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

Re: RFR: 8268349: Provide more detail in JEP 411 warning messages

[Weijun Wang]

On Tue, 8 Jun 2021 06:41:11 GMT, Alan Bateman  wrote:

> > You might want to make your "WARNING" consistent with the VM's "Warning" so 
> > that OutputAnalyzer's logic to ignore warnings will automatically ignore 
> > these too.
> 
> The uppercase "WARNING" is intentional here, it was the same with illegal 
> reflective access warnings. I'm sure Max has or will run all tests to see if 
> there are any issues.

Will definitely run all from tier1-tier9. I ran them multiple times while 
implementing JEP 411.

I've seen warnings with "VM" word in the prefix and test methods that filter 
them out, but feel the warnings here are not related to VM. The new warnings do 
have impacts on some tests and I'll be very carefully not break them.

-

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

RFR: 8268349: Provide more detail in JEP 411 warning messages

[Weijun Wang]

More loudly and precise warning messages when either a security manager is 
enabled at startup or installed at runtime.

-

Commit messages:
 - 8268349: Provide more detail in JEP 411 warning messages

Changes: https://git.openjdk.java.net/jdk/pull/4400/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4400&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8268349
  Stats: 115 lines in 5 files changed: 73 ins; 21 del; 21 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4400.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4400/head:pull/4400

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

Integrated: 8267521: Post JEP 411 refactoring: maximum covering > 50K

[Weijun Wang]

On Fri, 21 May 2021 01:52:27 GMT, Weijun Wang  wrote:

> The code change refactors classes that have a `SuppressWarnings("removal")` 
> annotation that covers more than 50KB of code. The big annotation is often 
> quite faraway from the actual deprecated API usage it is suppressing, and 
> with the annotation covering too big a portion it's easy to call other 
> deprecated methods without being noticed.
> 
> The code change shows some common solutions to avoid such too wide 
> annotations:
> 
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local 
> variable and add annotation on the declaration, and then assign that value to 
> the original l-value if not void. The local variable will be called `tmp` if 
> later reassigned or `dummy` if it will be discarded.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
> 
> I'll add a copyright year update commit before integration.

This pull request has now been integrated.

Changeset: 508cec75
Author:Weijun Wang 
URL:   
https://git.openjdk.java.net/jdk/commit/508cec7535cd0ad015d566389bc9e5f53ce4103b
Stats: 245 lines in 18 files changed: 140 ins; 39 del; 66 mod

8267521: Post JEP 411 refactoring: maximum covering > 50K

Reviewed-by: dfuchs, prr

-

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v4]

[Weijun Wang]

> The code change refactors classes that have a `SuppressWarnings("removal")` 
> annotation that covers more than 50KB of code. The big annotation is often 
> quite faraway from the actual deprecated API usage it is suppressing, and 
> with the annotation covering too big a portion it's easy to call other 
> deprecated methods without being noticed.
> 
> The code change shows some common solutions to avoid such too wide 
> annotations:
> 
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local 
> variable and add annotation on the declaration, and then assign that value to 
> the original l-value if not void. The local variable will be called `tmp` if 
> later reassigned or `dummy` if it will be discarded.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.
> 
> I'll add a copyright year update commit before integration.

Weijun Wang has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains one commit:

  8267521: Post JEP 411 refactoring: maximum covering > 50K

-

Changes: https://git.openjdk.java.net/jdk/pull/4138/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=03
  Stats: 245 lines in 18 files changed: 140 ins; 39 del; 66 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138

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

Integrated: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal

[Weijun Wang]

On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang  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.

This pull request has now been integrated.

Changeset: 6765f902
Author:Weijun Wang 
URL:   
https://git.openjdk.java.net/jdk/commit/6765f902505fbdd02f25b599f942437cd805cad1
Stats: 2755 lines in 826 files changed: 1997 ins; 20 del; 738 mod

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

Co-authored-by: Sean Mullan 
Co-authored-by: Lance Andersen 
Co-authored-by: Weijun Wang 
Reviewed-by: erikj, darcy, chegar, naoto, joehw, alanb, mchung, kcr, prr, lancea

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v9]

[Weijun Wang]

> 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 with a new target base due to a merge 
or a rebase. The pull request now contains 14 commits:

 - copyright years
 - merge from master, resolve one conflict
 - Merge branch 'master'
 - merge from master
 - rename setSecurityManagerDirect to implSetSecurityManager
 - default behavior reverted to allow
 - move one annotation to new method
 - merge from master, two files removed, one needs merge
 - keep only one systemProperty tag
 - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
 - ... and 4 more: https://git.openjdk.java.net/jdk/compare/19450b99...331389b5

-

Changes: https://git.openjdk.java.net/jdk/pull/4073/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=08
  Stats: 2755 lines in 826 files changed: 1997 ins; 20 del; 738 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4073.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v8]

[Weijun Wang]

> 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 with a new target base due to a merge 
or a rebase. The pull request now contains 11 commits:

 - merge from master
 - rename setSecurityManagerDirect to implSetSecurityManager
 - default behavior reverted to allow
 - move one annotation to new method
 - merge from master, two files removed, one needs merge
 - keep only one systemProperty tag
 - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
 - feedback from Sean, Phil and Alan
 - add supresswarnings annotations automatically
 - manual change before automatic annotating
 - ... and 1 more: https://git.openjdk.java.net/jdk/compare/74b70a56...ea2c4b48

-

Changes: https://git.openjdk.java.net/jdk/pull/4073/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=07
  Stats: 2132 lines in 826 files changed: 1997 ins; 20 del; 115 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4073.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v7]

[Weijun Wang]

> 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:

  rename setSecurityManagerDirect to implSetSecurityManager

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4073/files
  - new: https://git.openjdk.java.net/jdk/pull/4073/files/8fd09c39..926e4b9a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=06
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=05-06

  Stats: 5 lines in 1 file changed: 0 ins; 1 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4073.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v6]

[Weijun Wang]

On Mon, 31 May 2021 15:02:57 GMT, Weijun Wang  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

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.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v6]

[Weijun Wang]

> 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

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4073/files
  - new: https://git.openjdk.java.net/jdk/pull/4073/files/01dc4c0d..8fd09c39

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=04-05

  Stats: 183 lines in 6 files changed: 127 ins; 23 del; 33 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4073.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]

[Weijun Wang]

On Fri, 28 May 2021 03:12:35 GMT, Phil Race  wrote:

>> There *is* a tiny refactoring here: a new variable `s2` is introduced so the 
>> 2nd `doPrivileged` call on line 636 is now also in a declaration statement 
>> (for `s2`) and therefore annotatable. Without this I cannot add the 
>> annotation on line 635.
>
> Ok. But I will quote you
> "This happens when a deprecated method is called inside a static block. The 
> annotation can only be added to a declaration and here it must be the whole 
> class"
> 
> So the way you explained this before made it sound like any time there was 
> any SM API usage in a static block, the entire class needed to be annotated.
> 
> Why has the explanation changed ?

I should have been more precise. An annotation can only be added on a 
declaration, whether it's a variable, a method, or a class. Static block is not 
a declaration and the only one covers it is the class. But then if it's on a 
local variable declaration inside a static block, we certainly can annotate on 
that variable.

-

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]

[Weijun Wang]

On Thu, 27 May 2021 17:42:56 GMT, Phil Race  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   update FtpClient.java
>
> src/java.desktop/share/classes/java/awt/Component.java line 630:
> 
>> 628: }
>> 629: 
>> 630: @SuppressWarnings("removal")
> 
> I'm confused. I thought the reason this wasn't done in the JEP implementation 
> PR is because of refactoring
> that was needed because of the usage in this static block and you could not 
> apply the annotation here.
> Yet it seems you are doing exactly what was supposed to be impossible with no 
> refactoring.
> Can you explain ?

There *is* a tiny refactoring here: a new variable `s2` is introduced so the 
2nd `doPrivileged` call on line 636 is now also in a declaration statement (for 
`s2`) and therefore annotatable. Without this I cannot add the annotation on 
line 635.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v5]

[Weijun Wang]

On Thu, 27 May 2021 20:16:25 GMT, Weijun Wang  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 with a new target base due to a 
> merge or a rebase. The pull request now contains eight commits:
> 
>  - move one annotation to new method
>  - merge from master, two files removed, one needs merge
>  - keep only one systemProperty tag
>  - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
>  - feedback from Sean, Phil and Alan
>  - add supresswarnings annotations automatically
>  - manual change before automatic annotating
>  - 8266459: Implement JEP 411: Deprecate the Security Manager for Removal

Two files were removed by JEP 403 and JEP 407, respectively. One method in 
`XMLSchemaFactory.java` got [its 
own](https://github.com/openjdk/jdk/commit/8c4719a58834dddcea39d69b199abf1aabf780e2#diff-593a224979eaff03e2a3df1863fcaf865364a31a2212cc0d1fe67a8458057857R429)
 `@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](https://github.com/openjdk/jdk/commit/a4c46e1e4f4f2f05c8002b2af683a390fc46b424#diff-59caf1a68085064b4b3eb4f6e33e440bb85ea93719f34660970e2d4eaf8ce469R3175)
 and the annotation must be moved there.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v5]

[Weijun Wang]

> 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 with a new target base due to a merge 
or a rebase. The pull request now contains eight commits:

 - move one annotation to new method
 - merge from master, two files removed, one needs merge
 - keep only one systemProperty tag
 - fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
 - feedback from Sean, Phil and Alan
 - add supresswarnings annotations automatically
 - manual change before automatic annotating
 - 8266459: Implement JEP 411: Deprecate the Security Manager for Removal

-

Changes: https://git.openjdk.java.net/jdk/pull/4073/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=04
  Stats: 2022 lines in 825 files changed: 1884 ins; 10 del; 128 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4073.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]

[Weijun Wang]

On Mon, 24 May 2021 09:00:07 GMT, Daniel Fuchs  wrote:

> Thanks for taking in my suggestions for FtpClient. I have also reviewed the 
> changes to java.util.logging and JMX. I wish there was a way to handle 
> doPrivileged returning void more nicely. Maybe component maintainers will be 
> able to deal with them on a case-by-case basis later on.

Assigning to a useless "dummy" variable looks ugly. Extracting the call to a 
method might make it faraway from its caller. In L114 of `FtpClient.java` I 
managed to invent a return value and I thought it was perfect. But you said 
it's "a bit strange". :-(

-

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

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

[Weijun Wang]

On Mon, 17 May 2021 17:51:36 GMT, Weijun Wang  wrote:

> Please review the test changes for [JEP 
> 411](https://openjdk.java.net/jeps/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](https://bugs.openjdk.java.net/browse/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 https://github.com/openjdk/jdk/pull/4073.

This pull request has now been integrated.

Changeset: 640a2afd
Author:Weijun Wang 
URL:   
https://git.openjdk.java.net/jdk/commit/640a2afda36857410b7abf398af81e35430a62e7
Stats: 1028 lines in 852 files changed: 252 ins; 9 del; 767 mod

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

Co-authored-by: Lance Andersen 
Co-authored-by: Weijun Wang 
Reviewed-by: dholmes, alanb, dfuchs, mchung, mullan, prr

-

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

Re: RFR: 8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v4]

[Weijun Wang]

> Please review the test changes for [JEP 
> 411](https://openjdk.java.net/jeps/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](https://bugs.openjdk.java.net/browse/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 https://github.com/openjdk/jdk/pull/4073.

Weijun Wang has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains 20 additional commits since the 
last revision:

 - Merge branch 'master' into 8267184
 - feedback from Phil
   
   reverted:
 - adjust order of VM options
 - test for awt
 - test for hotspot-gc
 - test for compiler
 - test for net
 - test for core-libs
 - test for beans
 - test for xml
 - ... and 10 more: https://git.openjdk.java.net/jdk/compare/37f74de7...412264a0

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4071/files
  - new: https://git.openjdk.java.net/jdk/pull/4071/files/9a3ec578..412264a0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=02-03

  Stats: 12227 lines in 453 files changed: 6978 ins; 3721 del; 1528 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v4]

[Weijun Wang]

On Sun, 23 May 2021 16:35:43 GMT, Sean Mullan  wrote:

>> src/java.base/share/classes/java/lang/SecurityManager.java line 104:
>> 
>>> 102:  * method will throw an {@code UnsupportedOperationException}). If the
>>> 103:  * {@systemProperty java.security.manager} system property is set to 
>>> the
>>> 104:  * special token "{@code allow}", then a security manager will not be 
>>> set at
>> 
>> Can/should the `{@systemProperty ...}` tag be used more than once for a 
>> given system property? I thought it should be used only once, at the place 
>> where the system property is defined. Maybe @jonathan-gibbons can offer some 
>> more guidance on this.
>
> Good point. I would remove the extra @systemProperty tags on lines 103, 106, 
> and 113. Also, in `System.setSecurityManager` there are 3 @systemProperty 
> java.security.manager tags, so we should remove those too.

New commit pushed. There is only one `@systemProperty` tag now.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v4]

[Weijun Wang]

> 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:

  keep only one systemProperty tag

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4073/files
  - new: https://git.openjdk.java.net/jdk/pull/4073/files/c4221b5f..1f6ff6c4

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=02-03

  Stats: 8 lines in 2 files changed: 0 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4073.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Weijun Wang]

On Fri, 21 May 2021 20:43:05 GMT, Phil Race  wrote:

> I haven't seen any response to this comment I made a couple of days ago and I 
> suspect it got missed
> 
> > Weijun Wang has updated the pull request incrementally with one additional 
> > commit since the last revision:
> > fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
> 
> test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
> 
> > 57: ProcessCommunicator
> > 58: .executeChildProcess(Consumer.class, new 
> > String[0]);
> > 59: if (!"Hello".equals(processResults.getStdOut())) {
> 
> Who or what prompted this change ?

I replied right in the thread but unfortunately GitHub does not display it at 
the end of page.

This is because the process is now launched with 
`-Djava.security.manager=allow` (because of another change in 
https://github.com/openjdk/jdk/pull/4071), and a new warning is displayed in 
stderr. Therefore I switched to stdout.

-

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v3]

[Weijun Wang]

> The code change refactors classes that have a `SuppressWarnings("removal")` 
> annotation that covers more than 50KB of code. The big annotation is often 
> quite faraway from the actual deprecated API usage it is suppressing, and 
> with the annotation covering too big a portion it's easy to call other 
> deprecated methods without being noticed.
> 
> The code change shows some common solutions to avoid such too wide 
> annotations:
> 
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local 
> variable and add annotation on the declaration, and then assign that value to 
> the original l-value.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  update FtpClient.java

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4138/files
  - new: https://git.openjdk.java.net/jdk/pull/4138/files/d460efb8..60d97a4c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=01-02

  Stats: 23 lines in 1 file changed: 0 ins; 12 del; 11 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]

[Weijun Wang]

On Fri, 21 May 2021 15:37:48 GMT, Daniel Fuchs  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   typo on windows
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 550:
> 
>> 548:  * @throws IOException if the connection was unsuccessful.
>> 549:  */
>> 550: @SuppressWarnings("removal")
> 
> Could the scope of the annotation be further reduced by applying it to the 
> two places where `doPrivileged` is called in this method?

I'll probably need to assign the doPriv result on L631 to a tmp variable and 
then assign it to `s`. Are you OK with this ugliness? Update: Ah, I see you 
already have similar suggestion in the next comment.

-

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]

[Weijun Wang]

On Fri, 21 May 2021 15:35:05 GMT, Daniel Fuchs  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   typo on windows
>
> src/java.base/share/classes/sun/net/ftp/impl/FtpClient.java line 120:
> 
>> 118: vals[1] = 
>> Integer.getInteger("sun.net.client.defaultConnectTimeout", 
>> 300_000).intValue();
>> 119: return System.getProperty("file.encoding", 
>> "ISO8859_1");
>> 120: }
> 
> It is a bit strange that "file.encoding" seem to get a special treatment - 
> but I guess that's OK.

You might say we thus avoid wasting the return value, as much as possible.

-

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

Re: RFR: 8267184: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v3]

[Weijun Wang]

> Please review the test changes for [JEP 
> 411](https://openjdk.java.net/jeps/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](https://bugs.openjdk.java.net/browse/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 https://github.com/openjdk/jdk/pull/4073.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  feedback from Phil
  
  reverted:

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4071/files
  - new: https://git.openjdk.java.net/jdk/pull/4071/files/bfa955ad..9a3ec578

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=01-02

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071

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

Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]

[Weijun Wang]

On Fri, 21 May 2021 18:26:48 GMT, Phil Race  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   adjust order of VM options
>
> test/jdk/java/awt/FontClass/CreateFont/fileaccess/FontFile.java line 3:
> 
>> 1: /*
>> 2:  * Copyright (c) 2008, 2021, Oracle and/or its affiliates. All rights 
>> reserved.
>> 3:  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> 
> Probably the (c) update was meant to be on the .sh file that is actually 
> modified.

Oops, I'll back it out. Thanks.

-

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

Re: RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v2]

[Weijun Wang]

> The code change refactors classes that have a `SuppressWarnings("removal")` 
> annotation that covers more than 50KB of code. The big annotation is often 
> quite faraway from the actual deprecated API usage it is suppressing, and 
> with the annotation covering too big a portion it's easy to call other 
> deprecated methods without being noticed.
> 
> The code change shows some common solutions to avoid such too wide 
> annotations:
> 
> 1. Extract calls into a method and add annotation on that method
> 2. Assign the return value of a deprecated method call to a new local 
> variable and add annotation on the declaration, and then assign that value to 
> the original l-value.
> 3. Put declaration and assignment into a single statement if possible.
> 4. Rewrite code to achieve #3 above.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  typo on windows

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4138/files
  - new: https://git.openjdk.java.net/jdk/pull/4138/files/e3f0405a..d460efb8

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138

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

RFR: 8267521: Post JEP 411 refactoring: maximum covering > 50K

[Weijun Wang]

The code change refactors classes that have a `SuppressWarnings("removal")` 
annotation that covers more than 50KB of code. The big annotation is often 
quite faraway from the actual deprecated API usage it is suppressing, and with 
the annotation covering too big a portion it's easy to call other deprecated 
methods without being noticed.

-

Depends on: https://git.openjdk.java.net/jdk/pull/4073

Commit messages:
 - 8267521: Post JEP 411 refactoring: maximum covering > 50K

Changes: https://git.openjdk.java.net/jdk/pull/4138/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4138&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267521
  Stats: 226 lines in 18 files changed: 142 ins; 29 del; 55 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4138.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4138/head:pull/4138

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Weijun Wang]

On Thu, 20 May 2021 02:06:46 GMT, Weijun Wang  wrote:

>> Well .. as an enhancement (P3 or otherwise) I can see it being dropped and 
>> definitely if it misses the fork,
>> and I don't get why you can't do it here. And if it isn't done the JEP isn't 
>> really ready.
>> I already pasted the patch for Component.java and it took about 60 seconds 
>> to do that.
>> Then yes, you would have to deal with the fact that now you need to reapply 
>> your automated tool to the file but this is just work you'd have to have 
>> done anyway if it was already refactored.
>> 
>> I only *noticed* Component and Container. And stopped there to raise the 
>> question. How many more cases are there ?
>
> I can make it a bug.
> 
> I don't want to do it here is because it involves indefinite amount of manual 
> work and we will see everyone having their preferences. The more time we 
> spend on this PR the more likely there will be merge conflict. There are just 
> too many files here.
> 
> And no matter if we include that change in this PR or after it, it will be 
> after the automatic conversion. In fact, the data about which cases are more 
> worth fixing come from the automatic conversion itself. Also, as the manual 
> work will be done part by part, if the automatic conversion is after it, 
> there will be rounds and rounds of history rewriting and force push. This is 
> quite unfriendly to the reviewers.
> 
> Altogether, there are 117 class-level annotations. Unfortunately, 
> `java.awt.Component` is the one with the biggest class -- estimated number of 
> bytes that the annotation covers is over 380K. In the client area, the 2nd 
> place is `java.awt.Container`, and then we have `sun.font.SunFontManager`, 
> `java.awt.Window`, and `java.awt.Toolkit` which are over 100KB, and other 25 
> classes over 10KB, and other 11 classes smaller than 10KB.

By converting JDK-8267432 to a bug, there is an extra benefit that we can fix 
it after RDP. So I'll convert it now.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Weijun Wang]

On Wed, 19 May 2021 23:50:04 GMT, Phil Race  wrote:

>> I just made it P3 (P4 was the default value), and I will target it to 17 
>> once JEP 411 is targeted 17. But I think it's probably not a good idea to 
>> include it inside *this* PR. There are some middle ground where it's 
>> debatable if a change is worth doing (Ex: which is uglier between an 
>> a-liitle-faraway-annotation and a temporary variable?) so it's not obvious 
>> what the scope of the refactoring can be. Thus it will be divided into 
>> smaller sub-tasks. It's not totally impossible that part of the work will be 
>> delayed to next release.
>
> Well .. as an enhancement (P3 or otherwise) I can see it being dropped and 
> definitely if it misses the fork,
> and I don't get why you can't do it here. And if it isn't done the JEP isn't 
> really ready.
> I already pasted the patch for Component.java and it took about 60 seconds to 
> do that.
> Then yes, you would have to deal with the fact that now you need to reapply 
> your automated tool to the file but this is just work you'd have to have done 
> anyway if it was already refactored.
> 
> I only *noticed* Component and Container. And stopped there to raise the 
> question. How many more cases are there ?

I can make it a bug.

I don't want to do it here is because it involves indefinite amount of manual 
work and we will see everyone having their preferences. The more time we spend 
on this PR the more likely there will be merge conflict. There are just too 
many files here.

And no matter if we include that change in this PR or after it, it will be 
after the automatic conversion. In fact, the data about which cases are more 
worth fixing come from the automatic conversion itself. Also, as the manual 
work will be done part by part, if the automatic conversion is after it, there 
will be rounds and rounds of history rewriting and force push. This is quite 
unfriendly to the reviewers.

Altogether, there are 117 class-level annotations. Unfortunately, 
`java.awt.Component` is the one with the biggest class -- estimated number of 
bytes that the annotation covers is over 380K. In the client area, the 2nd 
place is `java.awt.Container`, and then we have `sun.font.SunFontManager`, 
`java.awt.Window`, and `java.awt.Toolkit` which are over 100KB, and other 25 
classes over 10KB, and other 11 classes smaller than 10KB.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Weijun Wang]

On Wed, 19 May 2021 22:04:57 GMT, Phil Race  wrote:

>> Correct, there are ways to modify the code to make it more 
>> annotation-friendly. We thought about whether it's good to do it before 
>> adding the annotations or after it. Our decision now is to do it after 
>> because it will be more easy to see why it's necessary and we can take time 
>> to do them little by little. A new enhancement at 
>> https://bugs.openjdk.java.net/browse/JDK-8267432 is filed.
>
> I don't think it is a separate P4 enhancement (?) that someone will (maybe) 
> do next release.
> I think it should all be taken care of as part of this proposed change.

I just made it P3 (P4 was the default value), and I will target it to 17 once 
JEP 411 is targeted 17. But I think it's probably not a good idea to include it 
inside *this* PR. There are some middle ground where it's debatable if a change 
is worth doing (Ex: which is uglier between an a-liitle-faraway-annotation and 
a temporary variable?) so it's not obvious what the scope of the refactoring 
can be. Thus it will be divided into smaller sub-tasks. It's not totally 
impossible that part of the work will be delayed to next release.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Weijun Wang]

On Wed, 19 May 2021 19:31:24 GMT, Phil Race  wrote:

>> This happens when a deprecated method is called inside a static block. The 
>> annotation can only be added to a declaration and here it must be the whole 
>> class. The call in this file is
>> 
>> s = java.security.AccessController.doPrivileged(
>> new 
>> GetPropertyAction("awt.image.redrawrate"));
>
> That's a sad limitation of the annotation stuff then, but I don't think that 
> it is insurmountable.
> You can define a static private method to contain this and call it from the 
> static initializer block.
> Much better than applying the annotation to an entire class.
> 
> --- a/src/java.desktop/share/classes/java/awt/Component.java
> +++ b/src/java.desktop/share/classes/java/awt/Component.java
> @@ -618,6 +618,17 @@ public abstract class Component implements 
> ImageObserver, MenuContainer,
>   */
>  static boolean isInc;
>  static int incRate;
> +
> +private static void initIncRate() {
> +String s = java.security.AccessController.doPrivileged(
> + new 
> GetPropertyAction("awt.image.incrementaldraw"));
> +isInc = (s == null || s.equals("true"));
> +
> +s = java.security.AccessController.doPrivileged(
> +  new GetPropertyAction("awt.image.redrawrate"));
> +incRate = (s != null) ? Integer.parseInt(s) : 100;
> +}
> +
>  static {
>  /* ensure that the necessary native libraries are loaded */
>  Toolkit.loadLibraries();
> @@ -625,14 +636,7 @@ public abstract class Component implements 
> ImageObserver, MenuContainer,
>  if (!GraphicsEnvironment.isHeadless()) {
>  initIDs();
>  }
> -
> -String s = java.security.AccessController.doPrivileged(
> -   new 
> GetPropertyAction("awt.image.incrementaldraw"));
> -isInc = (s == null || s.equals("true"));
> -
> -s = java.security.AccessController.doPrivileged(
> -new 
> GetPropertyAction("awt.image.redrawrate"));
> -incRate = (s != null) ? Integer.parseInt(s) : 100;
> +initIncRate();
>  }

Correct, there are ways to modify the code to make it more annotation-friendly. 
We thought about whether it's good to do it before adding the annotations or 
after it. Our decision now is to do it after because it will be more easy to 
see why it's necessary and we can take time to do them little by little. A new 
enhancement at https://bugs.openjdk.java.net/browse/JDK-8267432 is filed.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Weijun Wang]

On Wed, 19 May 2021 18:44:06 GMT, Weijun Wang  wrote:

>> Similar as the one above, it's because of
>> 
>> static {
>> // Don't lazy-read because every app uses invalidate()
>> isJavaAwtSmartInvalidate = AccessController.doPrivileged(
>> new GetBooleanAction("java.awt.smartInvalidate"));
>> }
>
> We are thinking of more tweaks after this overall change to make the 
> annotation more precise. For example, in this case we can assign the 
> `doPrivileged` result to a local variable right at its declaration, and then 
> assign it to `isJavaAwtSmartInvalidate`. Some people might think this is 
> ugly. Such manual code changes need to done little by little to ease code 
> reviewing.

I know it's not easy to read the commit and that's why I make the 3rd commit 
totally automatic. Hopefully you have more confidence on the program than my 
hand. All annotations are added to the nearest declarations.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Weijun Wang]

On Wed, 19 May 2021 18:39:10 GMT, Weijun Wang  wrote:

>> src/java.desktop/share/classes/java/awt/Container.java line 97:
>> 
>>> 95:  * @since 1.0
>>> 96:  */
>>> 97: @SuppressWarnings("removal")
>> 
>> Same issue as with Component. a > 5,000 line file that uses AccessController 
>> in just 4 places.
>> 
>> Where else are you adding this to entire classes instead of the specific 
>> site ?
>
> Similar as the one above, it's because of
> 
> static {
> // Don't lazy-read because every app uses invalidate()
> isJavaAwtSmartInvalidate = AccessController.doPrivileged(
> new GetBooleanAction("java.awt.smartInvalidate"));
> }

We are thinking of more tweaks after this overall change to make the annotation 
more precise. For example, in this case we can assign the `doPrivileged` result 
to a local variable right at its declaration, and then assign it to 
`isJavaAwtSmartInvalidate`. Some people might think this is ugly. Such manual 
code changes need to done little by little to ease code reviewing.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Weijun Wang]

On Wed, 19 May 2021 18:26:25 GMT, Phil Race  wrote:

>> Weijun Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java
>
> src/java.desktop/share/classes/java/awt/Component.java line 217:
> 
>> 215:  * @author  Sami Shaio
>> 216:  */
>> 217: @SuppressWarnings("removal")
> 
> Why is this placed on the *entire class* ??
> This class is 10535 lines long and mentions AccessControl something like 8 
> places it uses AccessController or AcessControlContext.

This happens when a deprecated method is called inside a static block. The 
annotation can only be added to a declaration and here it must be the whole 
class. The call in this file is

s = java.security.AccessController.doPrivileged(
new 
GetPropertyAction("awt.image.redrawrate"));

> src/java.desktop/share/classes/java/awt/Container.java line 97:
> 
>> 95:  * @since 1.0
>> 96:  */
>> 97: @SuppressWarnings("removal")
> 
> Same issue as with Component. a > 5,000 line file that uses AccessController 
> in just 4 places.
> 
> Where else are you adding this to entire classes instead of the specific site 
> ?

Similar as the one above, it's because of

static {
// Don't lazy-read because every app uses invalidate()
isJavaAwtSmartInvalidate = AccessController.doPrivileged(
new GetBooleanAction("java.awt.smartInvalidate"));
}

> test/jdk/java/awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java line 59:
> 
>> 57: ProcessCommunicator
>> 58: .executeChildProcess(Consumer.class, new 
>> String[0]);
>> 59: if (!"Hello".equals(processResults.getStdOut())) {
> 
> Who or what prompted this change ?

The child process is started with `-Djava.security.manager=allow` (after the 
other PR) and a warning will be printed to stderr. Therefore I move the message 
to stdout.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v3]

[Weijun Wang]

> 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:

  fixing awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4073/files
  - new: https://git.openjdk.java.net/jdk/pull/4073/files/bb73093a..c4221b5f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=01-02

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4073.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v2]

[Weijun Wang]

On Tue, 18 May 2021 21:21:45 GMT, Weijun Wang  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:
> 
>   feedback from Sean, Phil and Alan

A new commit fixing `awt/datatransfer/DataFlavor/DataFlavorRemoteTest.java` 
test in tier4. The test compares stderr output with a known value but we have a 
new warning written to stderr now. It's now using stdout instead. @prrace, 
Please confirm this is acceptable. Thanks.

-

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

Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager [v2]

[Weijun Wang]

> Please review the test changes for [JEP 
> 411](https://openjdk.java.net/jeps/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](https://bugs.openjdk.java.net/browse/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 https://github.com/openjdk/jdk/pull/4073.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  adjust order of VM options

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4071/files
  - new: https://git.openjdk.java.net/jdk/pull/4071/files/900c28c0..bfa955ad

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=00-01

  Stats: 59 lines in 18 files changed: 8 ins; 6 del; 45 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal [v2]

[Weijun Wang]

> 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.

Weijun Wang has updated the pull request incrementally with one additional 
commit since the last revision:

  feedback from Sean, Phil and Alan

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4073/files
  - new: https://git.openjdk.java.net/jdk/pull/4073/files/eb6c566f..bb73093a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=00-01

  Stats: 9 lines in 3 files changed: 4 ins; 1 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4073.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal

[Weijun Wang]

On Tue, 18 May 2021 18:38:52 GMT, Weijun Wang  wrote:

>> src/java.base/share/classes/java/security/AccessController.java line 877:
>> 
>>> 875: @CallerSensitive
>>> 876: public static  T doPrivileged(PrivilegedExceptionAction 
>>> action,
>>> 877:  @SuppressWarnings("removal") 
>>> AccessControlContext context, Permission... perms)
>> 
>> you might find it easier if you put the Permissions parameter on a new line. 
>> There are several places in AccessController where the same thing happens.
>
> I'll try to update my auto-annotating program.

Turns out this only happens in this class:

$ rg '^\s*@SuppressWarnings("removal").*?,.'
src/java.base/share/classes/java/security/AccessController.java:449:
@SuppressWarnings("removal") AccessControlContext context, Permission... perms) 
{
src/java.base/share/classes/java/security/AccessController.java:514:
@SuppressWarnings("removal") AccessControlContext context, Permission... perms) 
{
src/java.base/share/classes/java/security/AccessController.java:877:
 @SuppressWarnings("removal") AccessControlContext 
context, Permission... perms)

I'll fix them manually.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal

[Weijun Wang]

On Tue, 18 May 2021 17:36:55 GMT, Alan Bateman  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.
>
> src/java.base/share/classes/java/security/AccessController.java line 877:
> 
>> 875: @CallerSensitive
>> 876: public static  T doPrivileged(PrivilegedExceptionAction 
>> action,
>> 877:  @SuppressWarnings("removal") 
>> AccessControlContext context, Permission... perms)
> 
> you might find it easier if you put the Permissions parameter on a new line. 
> There are several places in AccessController where the same thing happens.

My rule is that a new line is added if there's only whitespaces before the 
insert position and the previous line does not end with a comma. In this case, 
unless I move `Permission... perms` onto a new line that an annotation on a new 
line looks like covering both `context` and `perms`.

-

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

Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager

[Weijun Wang]

On Tue, 18 May 2021 05:48:56 GMT, Alan Bateman  wrote:

> 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 and will go through more. Yes it looks 
good to group system property settings together. Thanks.

-

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

Re: RFR: 8267184: JEP 411: Add -Djava.security.manager=allow to tests calling System.setSecurityManager

[Weijun Wang]

On Tue, 18 May 2021 11:12:00 GMT, Daniel Fuchs  wrote:

>> Please review the test changes for [JEP 
>> 411](https://openjdk.java.net/jeps/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](https://bugs.openjdk.java.net/browse/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 https://github.com/openjdk/jdk/pull/4073.
>
> test/jdk/javax/management/remote/mandatory/notif/NotificationEmissionTest.java
>  line 34:
> 
>> 32:  * @run clean NotificationEmissionTest
>> 33:  * @run build NotificationEmissionTest
>> 34:  * @run main NotificationEmissionTest 1
> 
> 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?

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.

-

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

Re: RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal

[Weijun Wang]

On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang  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.

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) thr

RFR: 8266459: Implement JEP 411: Deprecate the Security Manager for Removal

[Weijun Wang]

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.

-

Commit messages:
 - add supresswarnings annotations automatically
 - manual change before automatic annotating
 - 8266459: Implement JEP 411: Deprecate the Security Manager for Removal

Changes: https://git.openjdk.java.net/jdk/pull/4073/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4073&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8266459
  Stats: 2018 lines in 826 files changed: 1884 ins; 9 del; 125 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4073.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4073/head:pull/4073

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

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

[Weijun Wang]

Please review the test changes for [JEP 411](https://openjdk.java.net/jeps/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. 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](https://bugs.openjdk.java.net/browse/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 https://github.com/openjdk/jdk/pull/4073.

-

Commit messages:
 - test for awt
 - test for hotspot-gc
 - test for compiler
 - test for net
 - test for core-libs
 - test for beans
 - test for xml
 - test for nio
 - test for hotspot-runtime
 - test for security
 - ... and 7 more: https://git.openjdk.java.net/jdk/compare/79b39445...900c28c0

Changes: https://git.openjdk.java.net/jdk/pull/4071/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4071&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267184
  Stats: 1024 lines in 852 files changed: 249 ins; 8 del; 767 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4071.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4071/head:pull/4071

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

Re: RFR: 8080272 Refactor I/O stream copying to use java.io.InputStream.transferTo [v8]

[Weijun Wang]

On Mon, 21 Dec 2020 09:16:11 GMT, Andrey Turbanov 
 wrote:

>> 8080272  Refactor I/O stream copying to use java.io.InputStream.transferTo
>
> Andrey Turbanov has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   8080272: Refactor I/O stream copying to use java.io.InputStream.transferTo
>   revert changes in Apache Santuario

The other security-related code changes look good to me.

src/java.xml.crypto/share/classes/org/jcp/xml/dsig/internal/dom/Utils.java line 
49:

> 47: throws IOException
> 48: {
> 49: return is.readAllBytes();

This is also from Apache Santuario. It's better to keep it unchanged.

-

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

Re: RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports - (core libraries)

[Weijun Wang]

There are several security-related files (name.contains("security")) and they 
all look fine.

--Max


> On May 4, 2020, at 1:12 PM, Mikael Vidstedt  
> wrote:
> 
> 
> Please review this change which implements part of JEP 381:
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
> webrev: 
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/corelibs/open/webrev/
> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
> 
> 
> Note: When reviewing this, please be aware that this exercise was *extremely* 
> mind-numbing, so I appreciate your help reviewing all the individual changes 
> carefully. You may want to get that coffee cup filled up (or whatever keeps 
> you awake)!
> 
> 
> Background:
> 
> Because of the size of the total patch and wide range of areas touched, this 
> patch is one out of in total six partial patches which together make up the 
> necessary changes to remove the Solaris and SPARC ports. The other patches 
> are being sent out for review to mailing lists appropriate for the respective 
> areas the touch. An email will be sent to jdk-dev summarizing all the 
> patches/reviews. To be clear: this patch is *not* in itself complete and 
> stand-alone - all of the (six) patches are needed to form a complete patch. 
> Some changes in this patch may look wrong or incomplete unless also looking 
> at the corresponding changes in other areas.
> 
> For convenience, I’m including a link below[1] to the full webrev, but in 
> case you have comments on changes in other areas, outside of the files 
> included in this thread, please provide those comments directly in the thread 
> on the appropriate mailing list for that area if possible.
> 
> In case it helps, the changes were effectively produced by searching for and 
> updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
> More information about the areas impacted can be found in the JEP itself.
> 
> 
> Testing:
> 
> A slightly earlier version of this change successfully passed tier1-8, as 
> well as client tier1-2. Additional testing will be done after the first round 
> of reviews has been completed.
> 
> Cheers,
> Mikael
> 
> [1] 
> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
> 

Re: Need sponsor to fix Javadoc warnings

[Weijun Wang]

The additional patch looks fine to me.

Thanks,
Max

> On Apr 12, 2020, at 3:23 AM, Vipin Sharma  wrote:
> 
> Hi Pavel,
> 
>> On Apr 9, 2020, at 2:45 AM, Pavel Rappo  wrote:
>> 
>> If your new patch addresses a similar type of problem, please send it in 
>> reply to this email,
>> so that I could merge it with the existing patch. Let's try to minimize 
>> process overhead if possible.
>> 
> This is additional patch:
> 
> --- old/src/java.base/share/classes/jdk/internal/icu/text/StringPrep.java 
> 2020-04-12 00:33:54.818724363 +0530
> +++ new/src/java.base/share/classes/jdk/internal/icu/text/StringPrep.java 
> 2020-04-12 00:33:54.398714466 +0530
> @@ -142,7 +142,7 @@
>/**
> * Called by com.ibm.icu.util.Trie to extract from a lead surrogate's
> * data the index array offset of the indexes for that lead surrogate.
> -* @param property data value for a surrogate from the trie, including
> +* @param value data value for a surrogate from the trie, including
> *the folding offset
> * @return data offset or 0 if there is no data for the lead surrogate
> */
> --- 
> old/src/java.base/share/classes/sun/net/www/content/text/PlainTextInputStream.java
> 2020-04-12 00:33:55.778746974 +0530
> +++ 
> new/src/java.base/share/classes/sun/net/www/content/text/PlainTextInputStream.java
> 2020-04-12 00:33:55.346736801 +0530
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 1996, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 1996, 2020, Oracle and/or its affiliates. All rights 
> reserved.
>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>  *
>  * This code is free software; you can redistribute it and/or modify it
> @@ -39,7 +39,7 @@
> 
> /**
>  * Calls FilterInputStream's constructor.
> - * @param an InputStream
> + * @param is an InputStream
>  */
> PlainTextInputStream(InputStream is) {
> super(is);
> --- 
> old/src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>  2020-04-12 00:33:56.726769287 +0530
> +++ 
> new/src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
>  2020-04-12 00:33:56.306759403 +0530
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2012, 2017, Oracle and/or its affiliates. All rights 
> reserved.
> + * Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights 
> reserved.
>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>  *
>  * This code is free software; you can redistribute it and/or modify it
> @@ -881,7 +881,7 @@
>  * only CRLs signed with a different key (but the same issuer
>  * name) as the certificate being checked.
>  *
> - * @param currCert the X509Certificate to be checked
> + * @param cert the X509Certificate to be checked
>  * @param prevKey the PublicKey that failed
>  * @param signFlag true if that key was trusted to sign CRLs
>  * @param stackedCerts a Set of 
> X509Certificates>
> --- 
> old/src/java.base/share/classes/sun/security/provider/certpath/URICertStore.java
>   2020-04-12 00:33:57.658791207 +0530
> +++ 
> new/src/java.base/share/classes/sun/security/provider/certpath/URICertStore.java
>   2020-04-12 00:33:57.250781612 +0530
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2006, 2019, Oracle and/or its affiliates. All rights 
> reserved.
> + * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights 
> reserved.
>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>  *
>  * This code is free software; you can redistribute it and/or modify it
> @@ -166,7 +166,7 @@
> /**
>  * Creates a URICertStore.
>  *
> - * @param parameters specifying the URI
> + * @param params parameters specifying the URI
>  */
> URICertStore(CertStoreParameters params)
> throws InvalidAlgorithmParameterException, NoSuchAlgorithmException {
> --- 
> old/src/java.base/share/classes/sun/security/ssl/StatusResponseManager.java   
> 2020-04-12 00:33:58.602813394 +0530
> +++ 
> new/src/java.base/share/classes/sun/security/ssl/StatusResponseManager.java   
> 2020-04-12 00:33:58.178803431 +0530
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights 
> reserved.
> + * Copyright (c) 2015, 2020, Oracle and/or its affiliates. All rights 
> reserved.
>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>  *
>  * This code is free software; you can redistribute it and/or modify it
> @@ -483,7 +483,7 @@
>  * and its corresponding CertId.
>  *
>  * @param subjectCert the certificate to be checked for revocation
> - * @param cid the CertId for {@code subjectCert}
> + * @param certId the CertId for {@code subjectCert}
>  */
> StatusInfo(X509Certificate subjectCert, CertId certId) {
> cert = subjectCert;
> --- old/src/java.base/share/classes/sun/security/timesta

Re: Is SPNEGO supposed to work out of the box on domain joined Windows client

[Weijun Wang]

> On Dec 12, 2019, at 8:23 AM, Daniel Fuchs  wrote:
> 
> Hi,
> 
> By default transparent authentication is disabled on windows.
> You may have to specify a non-default value for the
> jdk.http.ntlm.transparentAuth property [1], or configure
> an Authenticator [2] that has the appropriate credentials.

The above is for NTLM.

For the Negotiate scheme, it's almost out-of-box.

1. Java is able to find realm and KDC via environment variables
2. Java can use the LSA cache if a realm user is logged in

but you'll need a special registry key [1] set for #2 above.

Or you can bridge to a native GSS library, you need to set the system property 
sun.security.jgss.native to true. JDK 13 contains its own native GSS library 
but if you're still on JDK 11, you also need to point the system property 
sun.security.jgss.lib to a 3rd-party GSS library (Ex: from MIT).

--Max

[1] 
https://support.microsoft.com/en-us/help/2627903/access-to-session-keys-not-possible-using-a-restricted-token

> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8225506
> [2] 
> https://docs.oracle.com/en/java/javase/13/docs/api/java.base/java/net/HttpURLConnection.html#setAuthenticator(java.net.Authenticator)
> 
> best regards,
> 
> -- daniel
> 
> On 10/12/19 21:11, Marcin Wiśnicki wrote:
>> Forgive me for asking possibly silly question but I looked everywhere and 
>> couldn't find a simple answer to this question:
>> If I use standard java.net  classes to establish connection 
>> to HTTP server that uses SPNEGO authentication (AD) from a Windows machine 
>> that's joined to AD, without further configuration, is this supposed to work?
> 

Re: How to deal with multi-step WWW-Authenticate with the new HttpClient

[Weijun Wang]

Sorry, after adding "-Djdk.httpclient.HttpClient.log=headers,errors,channel" I 
found out it's my problem.

> On Apr 17, 2019, at 11:09 AM, Weijun Wang  wrote:
> 
> Hi, All,
> 
> I am trying the new HttpClient to deal with a multi-step Negotiate 
> authentication with Windows IIS, and here is my code.
> 
> HttpClient hc = HttpClient.newBuilder().build();
> var req = HttpRequest.newBuilder().uri(new URI(args[0]));
> while (true) {
>var resp = hc.send(req.build(), HttpResponse.BodyHandlers.ofString());
>System.out.println("");
>System.out.println(resp.statusCode());
>String auth = resp.headers().allValues("WWW-Authenticate")
>.stream()
>.filter(s -> s.startsWith("Negotiate"))
>.findFirst()
>.orElseThrow()
>.substring(9)
>.trim();
>System.out.println("incoming " + auth);
>byte[] in = auth.isEmpty() ? new byte[0] :Base64.getDecoder().decode(auth);
>byte[] out = calculate_token_from_incoming(in);
>if (out == null) break; // if status is still >400. No way to continue
>String sent = Base64.getEncoder().encodeToString(out);
>System.out.println("");
>System.out.println("outgoing " + sent);
>req.header("Authorization", "Negotiate " + sent);

I am using the wrong method here, should be "setHeader".

Thanks,
Max

> }
> 
> This works when there is only one request and one reply, but fails when there 
> is more.
> 
> Is this the correct way? Is there some keep-connnection thing I need to care 
> about?
> 
> Thanks,
> Max
> 
> p.s. Or maybe there is something wrong with IIS. This is a corner case.
> 

How to deal with multi-step WWW-Authenticate with the new HttpClient

[Weijun Wang]

Hi, All,

I am trying the new HttpClient to deal with a multi-step Negotiate 
authentication with Windows IIS, and here is my code.

HttpClient hc = HttpClient.newBuilder().build();
var req = HttpRequest.newBuilder().uri(new URI(args[0]));
while (true) {
var resp = hc.send(req.build(), HttpResponse.BodyHandlers.ofString());
System.out.println("");
System.out.println(resp.statusCode());
String auth = resp.headers().allValues("WWW-Authenticate")
.stream()
.filter(s -> s.startsWith("Negotiate"))
.findFirst()
.orElseThrow()
.substring(9)
.trim();
System.out.println("incoming " + auth);
byte[] in = auth.isEmpty() ? new byte[0] :Base64.getDecoder().decode(auth);
byte[] out = calculate_token_from_incoming(in);
if (out == null) break; // if status is still >400. No way to continue
String sent = Base64.getEncoder().encodeToString(out);
System.out.println("");
System.out.println("outgoing " + sent);
req.header("Authorization", "Negotiate " + sent);
}

This works when there is only one request and one reply, but fails when there 
is more.

Is this the correct way? Is there some keep-connnection thing I need to care 
about?

Thanks,
Max

p.s. Or maybe there is something wrong with IIS. This is a corner case.

Re: RFR 8220598: Malformed copyright year range in a few files in java.base

[Weijun Wang]

+1 (on the security class).

Thanks,
Max

> On Mar 14, 2019, at 1:10 AM, Lance Andersen  wrote:
> 
> +1
>> On Mar 13, 2019, at 1:04 PM, Chris Hegarty  wrote:
>> 
>> Trivially, there should be a comma after the year. Just add it.
>> 
>> 
>> $ hg diff src/java.base/share/classes/jdk/ src/java.base/share/classes/sun
>> diff --git 
>> a/src/java.base/share/classes/jdk/internal/util/ArraysSupport.java 
>> b/src/java.base/share/classes/jdk/internal/util/ArraysSupport.java
>> --- a/src/java.base/share/classes/jdk/internal/util/ArraysSupport.java
>> +++ b/src/java.base/share/classes/jdk/internal/util/ArraysSupport.java
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (c) 2015, 2017 Oracle and/or its affiliates. All rights 
>> reserved.
>> + * Copyright (c) 2015, 2017, Oracle and/or its affiliates. All rights 
>> reserved.
>>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>  *
>>  * This code is free software; you can redistribute it and/or modify it
>> diff --git 
>> a/src/java.base/share/classes/sun/net/spi/DefaultProxySelector.java 
>> b/src/java.base/share/classes/sun/net/spi/DefaultProxySelector.java
>> --- a/src/java.base/share/classes/sun/net/spi/DefaultProxySelector.java
>> +++ b/src/java.base/share/classes/sun/net/spi/DefaultProxySelector.java
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (c) 2003, 2018 Oracle and/or its affiliates. All rights 
>> reserved.
>> + * Copyright (c) 2003, 2018, Oracle and/or its affiliates. All rights 
>> reserved.
>>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>  *
>>  * This code is free software; you can redistribute it and/or modify it
>> diff --git a/src/java.base/share/classes/sun/security/util/IOUtils.java 
>> b/src/java.base/share/classes/sun/security/util/IOUtils.java
>> --- a/src/java.base/share/classes/sun/security/util/IOUtils.java
>> +++ b/src/java.base/share/classes/sun/security/util/IOUtils.java
>> @@ -1,5 +1,5 @@
>> /*
>> - * Copyright (c) 2009, 2017 Oracle and/or its affiliates. All rights 
>> reserved.
>> + * Copyright (c) 2009, 2017, Oracle and/or its affiliates. All rights 
>> reserved.
>>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>  *
>>  * This code is free software; you can redistribute it and/or modify it
>> 
>> -Chris.
> 
> 
> 
> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
> Oracle Java Engineering 
> 1 Network Drive 
> Burlington, MA 01803
> lance.ander...@oracle.com
> 
> 
> 

Re: A parallel HttpClient sendAync question

[Weijun Wang]

> 
> That's what I would use too. Though Chris Y.'s suggestion below
> should also work:
> 
> On 13/11/2018 03:41, Chris Yin wrote: > lines.flapMap(x -> 
> Stream.ofNullable(findURIFrom(x)))
>>.map(l -> download(c, l))
>>.collect(Collectors.toList())
>>.forEach(f -> f.join());
> 
> since the collection should exhaust the stream before starting
> the forEach loop.

Yes, this should work.

While I know the last forEach here is not Stream::forEach, I cannot stop 
thinking there are 2 terminal operations in this call chain. Almost feel like 
cheating. :-)

--Max

> 
> best regards,
> 
> -- daniel
> 

Re: A parallel HttpClient sendAync question

[Weijun Wang]

> On Nov 13, 2018, at 11:41 AM, Chris Yin  wrote:
> 
>> 
>> On 13 Nov 2018, at 10:35 AM, Weijun Wang  wrote:
>> 
>> I'm scanning a file and downloading links inside:
>> 
>> lines.flapMap(x -> Stream.ofNullable(findURIFrom(x)))
>>.map(l -> download(c, l))
>>.forEach(f -> f.join());
>> 
>> CompletableFuture> download(HttpClient c, URI link) {
>>   return c.sendAsync(HttpRequest.newBuilder(link).build(),
>>   HttpResponse.BodyHandlers.ofFile(Path.of(link.getPath(;
>> }
>> 
>> However, it seems the download is one by one and not parallel. I guess maybe 
>> map() is lazy and each request is only send when forEach() is called and the 
>> next one is only sent after join().
>> 
>> I can only collect the jobs into a list and then call join() on 
>> CompletableFuture.allOf(list). Is there a simpler way?
> 
> Just guess your Stream (lines) is not parallel, so either use parallel stream 
> or collect into list before join should work?
> 
> lines.parallel().flapMap(x -> Stream.ofNullable(findURIFrom(x)))
>.map(l -> download(c, l))
>.forEach(f -> f.join());

Does not work. Still downloading sequentially.

> 
> Or
> 
> lines.flapMap(x -> Stream.ofNullable(findURIFrom(x)))
>.map(l -> download(c, l))
>.collect(Collectors.toList())
>.forEach(f -> f.join());
> 
> Regards,
> Chris
> 
>> 
>> Thanks
>> Max

Re: A parallel HttpClient sendAsync question

[Weijun Wang]

> On Nov 13, 2018, at 11:09 AM, Pavel Rappo  wrote:
> 
>> On 13 Nov 2018, at 02:35, Weijun Wang  wrote:
>> 
>> I'm scanning a file and downloading links inside:
>> 
>> lines.flapMap(x -> Stream.ofNullable(findURIFrom(x)))
>>.map(l -> download(c, l))
>>.forEach(f -> f.join());
>> 
>> CompletableFuture> download(HttpClient c, URI link) {
>>   return c.sendAsync(HttpRequest.newBuilder(link).build(),
>>   HttpResponse.BodyHandlers.ofFile(Path.of(link.getPath(;
>> }
>> 
>> However, it seems the download is one by one and not parallel.
> 
> 1. CompletableFuture.join waits until the result (or exception) is available
> 2. I guess the "lines" stream is created from Files.lines(Path)?

It's from a blocking HTTP request:

   c.send(req, HttpResponse.BodyHandlers.ofLines()).body()

> If so, then
> 
>* this method does not read all lines into a {@code List}, but instead
>* populates lazily as the stream is consumed
> 
>> I can only collect the jobs into a list and then call join() on 
>> CompletableFuture.allOf(list). Is there a simpler way?
> 
> This seems to be a correct way of waiting until all the links have been
> downloaded (possibly in parallel, depending on the Executor you use for
> HttpClient) or an error occurred.

I didn't choose any executor but @implNote says it will be a thread pool.

BTW, I am expecting 1000 links but after 976 were downloaded it hangs and I 
have to Ctrl-C. I wish there is a way that when there are no new download 
within 10 seconds I can stop the whole process and print out which jobs are not 
finished yet.

Thanks
Max

> 
> 
>