TLS v1.3 extensions in TLS v1.2 handshake

2021-05-24 Thread Fridrich Strba 

Hello, good people,

The java 11 implementation of TLS v1.3 was backported into java 8 since 
some CPUs and it results sometimes in new handshake failures with 
hard-to-updage-firmware devices whose shell life might be still long.


We somehow debugged those failures and some devices bomb because of 
TLSv1.2 handshake containing the signature_algorihms_cert and 
supported_versions extensions.


I would love to propose for Java 8 the attached patch that would make 
the TLSv1.2 handshake to look exactly as it was looking in 8u252. The 
TLSv1.3 handshake still contains both extensions as it should. This 
could solve the differences of Java 8 behaviour between different update 
numbers.


Please, have a look and comment

Cheers

Fridrich

--- openjdk/jdk/src/share/classes/sun/security/ssl/SSLExtension.java	2021-05-21 08:10:00.830159018 +0200
+++ openjdk/jdk/src/share/classes/sun/security/ssl/SSLExtension.java	2021-05-21 08:11:40.258772361 +0200
@@ -198,7 +198,7 @@
 
 CH_SIGNATURE_ALGORITHMS_CERT (0x0032, "signature_algorithms_cert",
 SSLHandshake.CLIENT_HELLO,
-ProtocolVersion.PROTOCOLS_12_13,
+ProtocolVersion.PROTOCOLS_OF_13,
 CertSignAlgsExtension.chNetworkProducer,
 CertSignAlgsExtension.chOnLoadConsumer,
 null,
@@ -319,7 +319,7 @@
 
 CH_SUPPORTED_VERSIONS   (0x002B, "supported_versions",
 SSLHandshake.CLIENT_HELLO,
-ProtocolVersion.PROTOCOLS_TO_13,
+ProtocolVersion.PROTOCOLS_OF_13,
 SupportedVersionsExtension.chNetworkProducer,
 SupportedVersionsExtension.chOnLoadConsumer,
 null,

Re: RFR: 8255674: SSLEngine class description is missing "case" in switch statement

2021-05-24 Thread Xue-Lei Andrew Fan 
On Tue, 25 May 2021 04:36:53 GMT, Jaikiran Pai  wrote:

> Can I please get a review for this trivial fix in the code sample in javadoc 
> comment of `javax.net.ssl.SSLEngine` class?
> 
> I've run `make docs-image` locally and the generated javadoc after this 
> change looks fine.

Marked as reviewed by xuelei (Reviewer).

-

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

RFR: 8255674: SSLEngine class description is missing "case" in switch statement

2021-05-24 Thread Jaikiran Pai 
Can I please get a review for this trivial fix in the code sample in javadoc 
comment of `javax.net.ssl.SSLEngine` class?

I've run `make docs-image` locally and the generated javadoc after this change 
looks fine.

-

Commit messages:
 - 8255674 SSLEngine class description is missing "case" in switch statement

Changes: https://git.openjdk.java.net/jdk/pull/4180/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4180=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255674
  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4180.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4180/head:pull/4180

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

Re: RFR: 8267587: Update java.util to use enhanced switch [v2]

2021-05-24 Thread Tagir F . Valeev 
> Inspired by PR#4088. Most of the changes are done automatically using 
> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
> including indentations, moving comments, extracting common cast out of switch 
> expression branches, etc.
> 
> I also noticed that there are some switches having one branch only in 
> JapaneseImperialCalendar.java. Probably it would be better to replace them 
> with `if` statement?

Tagir F. Valeev has updated the pull request incrementally with one additional 
commit since the last revision:

  Unindent switch cases to simplify the review

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4161/files
  - new: https://git.openjdk.java.net/jdk/pull/4161/files/807d780d..2e966d0f

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4161=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4161=00-01

  Stats: 64 lines in 1 file changed: 27 ins; 27 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4161.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4161/head:pull/4161

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

Re: RFR: 8267587: Update java.util to use enhanced switch

2021-05-24 Thread Tagir F . Valeev 
On Mon, 24 May 2021 13:46:58 GMT, Daniel Fuchs  wrote:

>> Inspired by PR#4088. Most of the changes are done automatically using 
>> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
>> including indentations, moving comments, extracting common cast out of 
>> switch expression branches, etc.
>> 
>> I also noticed that there are some switches having one branch only in 
>> JapaneseImperialCalendar.java. Probably it would be better to replace them 
>> with `if` statement?
>
> src/java.base/share/classes/java/util/zip/ZipOutputStream.java line 261:
> 
>> 259: ZipEntry e = current.entry;
>> 260: switch (e.method) {
>> 261: case DEFLATED -> {
> 
> Same remark here - it's not clear what's going on.

Here, it's mostly indentation change (+4 spaces). You can set "Hide whitespace 
changes" and see:
![image](https://user-images.githubusercontent.com/5114450/119436860-3259b680-bd47-11eb-92d8-f940493d08c2.png)
Alternatively, I can indent back this switch, placing `case` on the same 
indentation level as `switch`. I'm not sure about recommended code style in 
OpenJDK project, it looks like it's not very consistent.

-

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

Re: RFR: 8267587: Update java.util to use enhanced switch

2021-05-24 Thread Tagir F . Valeev 
On Mon, 24 May 2021 13:44:36 GMT, Daniel Fuchs  wrote:

>> Inspired by PR#4088. Most of the changes are done automatically using 
>> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
>> including indentations, moving comments, extracting common cast out of 
>> switch expression branches, etc.
>> 
>> I also noticed that there are some switches having one branch only in 
>> JapaneseImperialCalendar.java. Probably it would be better to replace them 
>> with `if` statement?
>
> src/java.base/share/classes/java/util/regex/CharPredicates.java line 217:
> 
>> 215: case "WORD" -> WORD();
>> 216: default -> null;
>> 217: };
> 
> This file has lots of changes which are difficult to review. Maybe it should 
> be split out of this PR.

*sigh* GitHub diff tool is really poor and outdated. Here's how it looks in 
IntelliJ IDEA diff view:
![image](https://user-images.githubusercontent.com/5114450/119436474-6b455b80-bd46-11eb-8865-8b7f30826a8d.png)

-

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

Re: RFR: 8248268: Support KWP in addition to KW [v7]

2021-05-24 Thread Valerie Peng 
On Sat, 22 May 2021 01:02:50 GMT, Xue-Lei Andrew Fan  wrote:

>> Valerie Peng has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains seven commits:
>> 
>>  - Merge master into JDK-8248268
>>  - Minor update to address review comments.
>>  - Changed AESParameters to allow 4-byte, 8-byte IVs and removed
>>KWParameters and KWPParameters.
>>  - Refactor code to reduce code duplication
>>Address review comments
>>Add more test vectors
>>  - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
>>AES/KWP/NoPadding
>>  - Restored Iv algorithm parameters impl.
>>  - 8248268: Support KWP in addition to KW
>>
>>Updated existing AESWrap support with AES/KW/NoPadding cipher
>>transformation. Added support for AES/KWP/NoPadding and
>>AES/KW/PKCS5Padding support to SunJCE provider.
>
> src/java.base/share/classes/com/sun/crypto/provider/ConstructKeys.java line 
> 176:
> 
>> 174: return ConstructKeys.constructPublicKey(encoding, 
>> keyAlgorithm);
>> 175: default:
>> 176: throw new RuntimeException("Unsupported key type");
> 
> Good improvement to thrown an exception rather than return "null"!
> 
> Is NoSuchAlgorithmException fitted better the specification?  See also the 
> comment in  KeyWrapCipher.engineUnwrap().

Yes, will change.

> src/java.base/share/classes/com/sun/crypto/provider/KeyWrapCipher.java line 
> 710:
> 
>> 708: outLen = helperDecrypt(dataBuf, dataIdx);
>> 709: return ConstructKeys.constructKey(dataBuf, 0, outLen,
>> 710: wrappedKeyAlgorithm, wrappedKeyType);
> 
> Per the specification of engineUnwrap,  maybe, the 
> ConstructKeys.constructKey() implementation could throw 
> NoSuchAlgorithmException if wrappedKeyType is unknown.  See the common in the 
> the ConstructKeys.constructKey() implementation.

Sure, makes sense.

-

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

Re: RFR: 8248268: Support KWP in addition to KW [v7]

2021-05-24 Thread Michael StJohns 

Some more general comments - related to the restructuring.

In AESKeyWrap at 152-155 - that check probably should be moved to W().   
KWP should do the formatting prior to passing the data to W().  Also at 
185-187 - move that to W_INV().


AESKeyWrap at 158 - shouldn't you be returning the cipher text length?  
That's what the comment in FeedbackCipher says.  W() should probably be 
returning the final length.


The length of the final ciphertext data should be 8 bytes longer than 
the plaintext. decryptFinal() seems to do the right thing by decreasing 
the length returned.   But again - shouldn't that be the purview of W_INV()?


The call in KeyWrapCipher.engineGetOutputSize() should probably also be 
passed into KWUtil rather than being  done in KeyWrapCipher.  And the 
more I look at this, the more I think that all of the engineUpdate 
operations should throw UnsupportedOperationException - it would 
certainly simplify the logic.  That would make the call return either  
inputLength + 8 or inputLength - 8 depending on mode.


KWUtil.W() - should probably check that in.length >= inLen + 8 and throw 
a ShortBufferException if not.


Would it be useful to add a comment in KeyWrapCipher that  warns 
maintainers that  AESKeyWrap(Padded).encryptFinal() and decryptFinal() 
uses the input buffer as the output buffer? That's a reasonable approach 
for decryption, but I'm wondering if you might want to support in-place 
encryption as there's no spec prohibition requiring data to be held 
until the encryption is complete.


Mike









On 5/24/2021 7:01 PM, Valerie Peng wrote:

On Fri, 21 May 2021 20:44:57 GMT, Xue-Lei Andrew Fan  wrote:


Valerie Peng has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains seven commits:

  - Merge master into JDK-8248268
  - Minor update to address review comments.
  - Changed AESParameters to allow 4-byte, 8-byte IVs and removed
KWParameters and KWPParameters.
  - Refactor code to reduce code duplication
Address review comments
Add more test vectors
  - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
AES/KWP/NoPadding
  - Restored Iv algorithm parameters impl.
  - 8248268: Support KWP in addition to KW

Updated existing AESWrap support with AES/KW/NoPadding cipher

transformation. Added support for AES/KWP/NoPadding and
AES/KW/PKCS5Padding support to SunJCE provider.

src/java.base/share/classes/com/sun/crypto/provider/AESParameters.java line 50:


48:
49: public AESParameters() {
50: core = new BlockCipherParamsCore(AESConstants.AES_BLOCK_SIZE, 4, 8);

A cipher object may not take different IV sizes at the same time.  I was just 
wondering how it could be used in practice.  Maybe something like:


AlgorithmParameters algParams = AlgorithmParameters.getInstance("AES");
algParams.init(ivParameterSpec);

The IV parameter is given with the init() method.  Then, it may be not 
necessary to construct the BlockCipherParamsCore object will all potential IV 
sizes.  See the comments in BlockCipherParamsCore.

Cipher objects normally takes just one iv size. BlockCipherParamsCore is used 
by various impls of AlgorithmParametersSpi class which may be used with 
different block cipher algorithms. The iv parameter is given with the 
AlgorithmParametersSpi.init() method and invalid iv will lead to exceptions. 
Since there are iv size checks built in BlockCipherParamsCore already, it seems 
safer to relax the check a bit for AES (4 and 8 for KWP and KW). The other 
choice is to just remove the size check from BlockCipherParamsCore for AES 
algorithm, but then we'd not be able to reject invalid iv lengths as before.


src/java.base/share/classes/com/sun/crypto/provider/BlockCipherParamsCore.java 
line 52:


50: private byte[] iv = null;
51:
52: private int[] moreSizes = null;

The moreSizes is not used other than the init() method field.  It may be not 
easy to check the specific size if we cache all supported sized in the object.  
For example, if the required IV size if 8 bytes, it may be a problem about how 
to make sure the iv size is 8 bytes exactly for a specific algorithm.

Maybe, we could just have a ivSize filed.  The default value is block_size, 
which coupe be set with the init(ivParameterSpec) method.


 
 private int ivSize;
 ...
BlockCipherParamsCore(int blkSize) {
block_size = blkSize;
ivSize = blkSize;
 }
 ...
void init(AlgorithmParameterSpec paramSpec) {
 ivSize = ...;  // reset IV size.
 }

 // use ivSize while encoding and decoding.

The problem with this is that this assumes that the specified paramSpec 
argument of the init() method is always valid.

-

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

Re: RFR: 8248268: Support KWP in addition to KW [v7]

2021-05-24 Thread Valerie Peng 
On Sat, 22 May 2021 00:45:27 GMT, Xue-Lei Andrew Fan  wrote:

>> Valerie Peng has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains seven commits:
>> 
>>  - Merge master into JDK-8248268
>>  - Minor update to address review comments.
>>  - Changed AESParameters to allow 4-byte, 8-byte IVs and removed
>>KWParameters and KWPParameters.
>>  - Refactor code to reduce code duplication
>>Address review comments
>>Add more test vectors
>>  - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
>>AES/KWP/NoPadding
>>  - Restored Iv algorithm parameters impl.
>>  - 8248268: Support KWP in addition to KW
>>
>>Updated existing AESWrap support with AES/KW/NoPadding cipher
>>transformation. Added support for AES/KWP/NoPadding and
>>AES/KW/PKCS5Padding support to SunJCE provider.
>
> src/java.base/share/classes/com/sun/crypto/provider/ConstructKeys.java line 
> 190:
> 
>> 188: return constructKey(encoding, keyAlgorithm, keyType);
>> 189: } else {
>> 190: byte[] encoding2 = Arrays.copyOfRange(encoding, ofs, 
>> ofs + len);
> 
> The array will be copied again in the X509EncodedKeySpec and 
> PKCS8EncodedKeySpec.  It is out of this scope, but it may be a performance 
> improvement if adding X509EncodedKeySpec and PKCS8EncodedKeySpec constructors 
> that accept array offset, so that the array copy here could be avoid.

Right, maybe there is no constructor which takes offset and length due to 
trying to avoid the cost of validating them. I recall seeing other *Spec 
classes whose list of constructors are similar to 
X509EncodedKeySpec/PKCS8EncodedKeySpec as well.

> src/java.base/share/classes/com/sun/crypto/provider/ConstructKeys.java line 
> 192:
> 
>> 190: byte[] encoding2 = Arrays.copyOfRange(encoding, ofs, 
>> ofs + len);
>> 191: try {
>> 192: return constructKey(encoding2, keyAlgorithm, 
>> keyType);
> 
> The two constructKey methods are depended on each other.  It may improve the 
> readability if only having one-way dependence, by moving the switch-block 
> logic from the former constructKey() to the later one.
> 
> 
>  static final Key constructKey(byte[] encoding, String keyAlgorithm,
>int keyType) throws InvalidKeyException, NoSuchAlgorithmException {
> return constructKey(encoding, 0. encoding.length, keyAlgorithm, keyType);
> }
> 
> 
> With this re-org, the array copy could be avoid if the key type is unknown.

Yes, I will refactor the two constructKey() methods as you suggested.

-

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

Re: RFR: 8248268: Support KWP in addition to KW [v7]

2021-05-24 Thread Valerie Peng 
On Fri, 21 May 2021 20:44:57 GMT, Xue-Lei Andrew Fan  wrote:

>> Valerie Peng has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains seven commits:
>> 
>>  - Merge master into JDK-8248268
>>  - Minor update to address review comments.
>>  - Changed AESParameters to allow 4-byte, 8-byte IVs and removed
>>KWParameters and KWPParameters.
>>  - Refactor code to reduce code duplication
>>Address review comments
>>Add more test vectors
>>  - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
>>AES/KWP/NoPadding
>>  - Restored Iv algorithm parameters impl.
>>  - 8248268: Support KWP in addition to KW
>>
>>Updated existing AESWrap support with AES/KW/NoPadding cipher
>>transformation. Added support for AES/KWP/NoPadding and
>>AES/KW/PKCS5Padding support to SunJCE provider.
>
> src/java.base/share/classes/com/sun/crypto/provider/AESParameters.java line 
> 50:
> 
>> 48: 
>> 49: public AESParameters() {
>> 50: core = new BlockCipherParamsCore(AESConstants.AES_BLOCK_SIZE, 4, 
>> 8);
> 
> A cipher object may not take different IV sizes at the same time.  I was just 
> wondering how it could be used in practice.  Maybe something like:
> 
> 
> AlgorithmParameters algParams = AlgorithmParameters.getInstance("AES");
> algParams.init(ivParameterSpec);
> 
> The IV parameter is given with the init() method.  Then, it may be not 
> necessary to construct the BlockCipherParamsCore object will all potential IV 
> sizes.  See the comments in BlockCipherParamsCore.

Cipher objects normally takes just one iv size. BlockCipherParamsCore is used 
by various impls of AlgorithmParametersSpi class which may be used with 
different block cipher algorithms. The iv parameter is given with the 
AlgorithmParametersSpi.init() method and invalid iv will lead to exceptions. 
Since there are iv size checks built in BlockCipherParamsCore already, it seems 
safer to relax the check a bit for AES (4 and 8 for KWP and KW). The other 
choice is to just remove the size check from BlockCipherParamsCore for AES 
algorithm, but then we'd not be able to reject invalid iv lengths as before.

> src/java.base/share/classes/com/sun/crypto/provider/BlockCipherParamsCore.java
>  line 52:
> 
>> 50: private byte[] iv = null;
>> 51: 
>> 52: private int[] moreSizes = null;
> 
> The moreSizes is not used other than the init() method field.  It may be not 
> easy to check the specific size if we cache all supported sized in the 
> object.  For example, if the required IV size if 8 bytes, it may be a problem 
> about how to make sure the iv size is 8 bytes exactly for a specific 
> algorithm.
> 
> Maybe, we could just have a ivSize filed.  The default value is block_size, 
> which coupe be set with the init(ivParameterSpec) method.
> 
> 
> 
> private int ivSize;
> ...
>BlockCipherParamsCore(int blkSize) {
>block_size = blkSize;
>ivSize = blkSize;
> }
> ...
>void init(AlgorithmParameterSpec paramSpec) {
> ivSize = ...;  // reset IV size.
> }
> 
> // use ivSize while encoding and decoding.

The problem with this is that this assumes that the specified paramSpec 
argument of the init() method is always valid.

-

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

Re: RFR: 8255557: Decouple GCM from CipherCore [v5]

2021-05-24 Thread Anthony Scarpino 
On Mon, 24 May 2021 20:09:06 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Review comments update
>
> test/jdk/com/sun/crypto/provider/Cipher/AES/TestAESCipher.java line 90:
> 
>> 88: i++;
>> 89: }
>> 90: return b;
> 
> The generated data seems too similar, all starts with 0 and increment. Maybe 
> use i = len to start with? Or take some additional argument for starting 
> value?

I wanted consistent data because it's harder to solve crypto failures when you 
cannot see consistent pattern in the data.  For example, the GCM case where the 
ciphertext is correct, but the tag is wrong.  It is easier to know that the 
problem is in GHASH and not GCTR.
I don't see any negatives to having similar data.

> test/jdk/com/sun/crypto/provider/Cipher/AES/TestAESCipher.java line 117:
> 
>> 115: //int offset = ci.update(plainText, 0, plainText.length, 
>> cipherText, 0);
>> 116: //ci.doFinal(cipherText, offset);
>> 117: ci.doFinal(plainText, 0, plainText.length, cipherText, 0);
> 
> This change seems un-necessary? I think it's better to not change it so 
> multi-part enc/dec are tested.

Because it's commented out, I'm not sure I intended to change that.  I think I 
was debugging something and never reverted the code

-

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

Re: RFR: 8248268: Support KWP in addition to KW [v7]

2021-05-24 Thread Valerie Peng 
On Fri, 21 May 2021 19:15:49 GMT, Xue-Lei Andrew Fan  wrote:

>> Valerie Peng has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains seven commits:
>> 
>>  - Merge master into JDK-8248268
>>  - Minor update to address review comments.
>>  - Changed AESParameters to allow 4-byte, 8-byte IVs and removed
>>KWParameters and KWPParameters.
>>  - Refactor code to reduce code duplication
>>Address review comments
>>Add more test vectors
>>  - Changed AlgorithmParameters impls to register under AES/KW/NoPadding and
>>AES/KWP/NoPadding
>>  - Restored Iv algorithm parameters impl.
>>  - 8248268: Support KWP in addition to KW
>>
>>Updated existing AESWrap support with AES/KW/NoPadding cipher
>>transformation. Added support for AES/KWP/NoPadding and
>>AES/KW/PKCS5Padding support to SunJCE provider.
>
> src/java.base/share/classes/com/sun/crypto/provider/BlockCipherParamsCore.java
>  line 81:
> 
>> 79: expectedLen + " bytes long");
>> 80: }
>> 81: iv = tmpIv.clone();
> 
> The moreSizes is not used after initialization.  The iv/tmpIv could be a 
> value other than the block_size.   The getEncoded() method would use the iv 
> value for the encoding.  While in the decoding method init(byte[]) method, 
> the IV sizes other block_size is not considered, and IOE will be thrown.  
> Could this be a problem?

Right, good catch, I will fix it.

-

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

Re: RFR: 8255557: Decouple GCM from CipherCore [v5]

2021-05-24 Thread Anthony Scarpino 
On Mon, 24 May 2021 20:38:29 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Review comments update
>
> test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMShortBuffer.java line 34:
> 
>> 32: /*
>> 33:  * @test
>> 34:  * @summary Call decrypt doFinal() with different output values to see 
>> if the
> 
> Missing bug id?

Here is my logic
It's not a regression test to verify a specific bug id, it's just a general 
functionality test.  Therefore the bug id does not point to a previous failure.

-

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

RFR: 8267543: Post JEP 411 refactoring: security

2021-05-24 Thread Weijun Wang 
For all modified files in #4073 having "security", "crypto", or "ssl" in their 
names. Codes are refactored to bring a `@SuppressWarning` annotation nearer to 
the deprecated API usage and also shrink the size of its target.

-

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

Commit messages:
 - 8267543: Post JEP 411 refactoring: security

Changes: https://git.openjdk.java.net/jdk/pull/4172/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4172=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8267543
  Stats: 111 lines in 19 files changed: 34 ins; 35 del; 42 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4172.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4172/head:pull/4172

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

Re: RFR: 8255557: Decouple GCM from CipherCore [v5]

2021-05-24 Thread Valerie Peng 
On Fri, 21 May 2021 21:18:34 GMT, Anthony Scarpino  
wrote:

>> Hi,
>> 
>> I need a review of this rather large change to GCM.  GCM will no longer use 
>> CipherCore, and AESCrypt  to handle it's buffers and other objects.  It is 
>> also a major code redesign limits the amount of data copies and make some 
>> performance-based decisions.
>> 
>> Thanks
>> 
>> Tony
>
> Anthony Scarpino has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Review comments update

Here are comments for the test changes.
Thanks,
Valerie

test/jdk/com/sun/crypto/provider/Cipher/AEAD/Encrypt.java line 236:

> 234: HexFormat hex = HexFormat.of().withUpperCase();
> 235: if (!Arrays.equals(output, outputTexts.get(k))) {
> 236: System.out.println("Combination #" + k + 1 + "\nresult   
>  " +

nit: Why "+1"? The number should match the exception message, otherwise, very 
confusing.

test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java line 673:

> 671: 
> 672: // Test update-update-doFinal with byte arrays and preset data 
> sizes
> 673: t = new GCMBufferTest("AES/GCM/NoPadding",

This change seems un-necessary? Why separating the declaration to line 631?

test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMShortBuffer.java line 34:

> 32: /*
> 33:  * @test
> 34:  * @summary Call decrypt doFinal() with different output values to see if 
> the

Missing bug id?

test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMShortBuffer.java line 56:

> 54: throw e;
> 55: }
> 56: c.doFinal(cipherText, 1, len, pt, 0);

Should check the return value of doFinal() to match the expected output size?

test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMShortBuffer.java line 78:

> 76: throw e;
> 77: }
> 78: c.doFinal(ByteBuffer.wrap(cipherText, 1, len), out);

Should check the return value of doFinal() to match the expected output size?

test/jdk/com/sun/crypto/provider/Cipher/AES/TestAESCipher.java line 90:

> 88: i++;
> 89: }
> 90: return b;

The generated data seems too similar, all starts with 0 and increment. Maybe 
use i = len to start with? Or take some additional argument for starting value?

test/jdk/com/sun/crypto/provider/Cipher/AES/TestAESCipher.java line 98:

> 96: 
> 97: byte[] iv = makeData(16);
> 98: AlgorithmParameterSpec aps = new GCMParameterSpec(128, new 
> byte[16]);

Use the already generated iv instead of new byte[16]?

test/jdk/com/sun/crypto/provider/Cipher/AES/TestAESCipher.java line 117:

> 115: //int offset = ci.update(plainText, 0, plainText.length, 
> cipherText, 0);
> 116: //ci.doFinal(cipherText, offset);
> 117: ci.doFinal(plainText, 0, plainText.length, cipherText, 0);

This change seems un-necessary? I think it's better to not change it so 
multi-part enc/dec are tested.

-

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

Re: RFR: 8255557: Decouple GCM from CipherCore [v2]

2021-05-24 Thread Anthony Scarpino 
On Mon, 24 May 2021 18:15:17 GMT, Valerie Peng  wrote:

>> src.array() throws an exception if it's read only
>
> Other files seem to use src.hasArray() call which also checked for read only. 
> Maybe that's what you meant to use?

Yes, I could use hasArray().  I hadn't started using that until later in 
development and never went back and changed lines like these

-

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

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

2021-05-24 Thread 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

Re: RFR: 8255557: Decouple GCM from CipherCore [v4]

2021-05-24 Thread Valerie Peng 
On Fri, 21 May 2021 04:07:05 GMT, Anthony Scarpino  
wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 640:
>> 
>>> 638:  * @return number of bytes used from 'in'
>>> 639:  */
>>> 640: int mergeBlock(byte[] buffer, int bufOfs, int bufLen, byte[] 
>>> in,
>> 
>> Can be made 'static'?
>
> mergeBlock contains blockSize which isn't static

I see...

-

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

Re: RFR: 8255557: Decouple GCM from CipherCore [v3]

2021-05-24 Thread Valerie Peng 
On Mon, 24 May 2021 16:54:39 GMT, Anthony Scarpino  
wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 338:
>> 
>>> 336: public int doFinal(ByteBuffer src, ByteBuffer dst) {
>>> 337: return doFinal(src, src.remaining());
>>> 338: }
>> 
>> Have you considered changing the argument list of existing 
>> update/doFinal(...) methods? Less calls.
>
> I had considered it, but there are a number of methods, particularly in the 
> in decrypt operation, where the non-interface methods are called.  I didn't 
> think it was right for all those calls to have "null" and "0" for output 
> arguments they don't use.
> It's possible this interface may be not needed with a future Intel intrinsic, 
> so we shall see.

Ok.

-

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

Re: RFR: 8255557: Decouple GCM from CipherCore [v2]

2021-05-24 Thread Valerie Peng 
On Mon, 24 May 2021 16:34:51 GMT, Anthony Scarpino  
wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 203:
>> 
>>> 201: // allocating and copying for direct bytebuffers
>>> 202: if (!src.isDirect() && !dst.isDirect() &&
>>> 203: !src.isReadOnly() && !dst.isReadOnly()) {
>> 
>> Why do we need to check for src being isReadOnly() since we are not writing 
>> bytes into src? As for dst, if it's read only, then we should probably not 
>> proceed further? The other update method which takes ByteBuffer dst did not 
>> check if it's read only. A bit inconsistent?
>
> src.array() throws an exception if it's read only

Other files seem to use src.hasArray() call which also checked for read only. 
Maybe that's what you meant to use?

>> src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 283:
>> 
>>> 281: // allocating and copying for direct bytebuffers
>>> 282: if (!src.isDirect() && !dst.isDirect() &&
>>> 283: !src.isReadOnly() && !dst.isReadOnly()) {
>> 
>> Same question regarding the isReadOnly() calls as in the update(ByteBuffer, 
>> ByteBuffer) method.
>
> src.array() throws an exception if it's read only

Same, meant to use src.hasArray()?

-

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

Re: RFR: 8236671: NullPointerException in JKS keystore [v2]

2021-05-24 Thread Will Sargent 
I have tried to sign up to the bug tracking system (through reset password
I think?) but I'm not getting an email out, so I can't add to the bug.

I have created a test case in Github:

https://github.com/wsargent/jca-key-failure/

The stack trace shows the invalid key store entry after saving and loading
it again.

https://github.com/wsargent/jca-key-failure/blob/main/src/main/java/com/tersesystems/jcakeyfailure/JcaKeyFailure.java#L68

On Fri, Apr 30, 2021 at 12:40 PM Seán Coffey  wrote:

> Thanks for the feedback Will. It would be useful if you can provide a
> testcase and/or add comments to JDK-8266351
>  on your experience.
>
> regards,
> Sean.
> On 30/04/2021 17:54, Will Sargent wrote:
>
> > KeyStore specification will be tightened up via another bug record
>
> This would be super helpful, as one thing that confuses me is what the
> relationship is between a key entry and a key alias -- in particular, the
> existence alias doesn't seem to guarantee a valid entry that can be
> retrieved.
>
> In JDK 11 it's possible to create a private key with a keystore using
> pkcs12.setKeyEntry() (see link below):
>
>
> https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java#L135
>
> and then have a null pointer exception when retrieving the entry from the
> alias because the certificate chain is null (see commented out "testSystem"
> use case):
>
>
> https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java#L27
>
> I can write this up into a formal bug if that helps.
>
> On Fri, Apr 30, 2021 at 2:30 AM Sean Coffey 
> wrote:
>
>> On Wed, 28 Apr 2021 12:39:42 GMT, Sean Coffey 
>> wrote:
>>
>> >> Trivial enough change. Improved the exception thrown from JceKeyStore
>> also.
>> >
>> > Sean Coffey 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 four additional
>> commits since the last revision:
>> >
>> >  - Check for null before try block
>> >  - Merge branch 'master' of https://github.com/openjdk/jdk into
>> JDK-8236671-NPE
>> >  - Fix white space
>> >  - 8236671: NullPointerException in JKS keystore
>>
>> KeyStore specification will be tightened up via another bug record:
>> https://bugs.openjdk.java.net/browse/JDK-8266351
>>
>> -
>>
>> PR: https://git.openjdk.java.net/jdk/pull/3588
>>
>

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

2021-05-24 Thread 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: 8255557: Decouple GCM from CipherCore [v3]

2021-05-24 Thread Anthony Scarpino 
On Tue, 18 May 2021 18:38:40 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   cleanup
>
> src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 338:
> 
>> 336: public int doFinal(ByteBuffer src, ByteBuffer dst) {
>> 337: return doFinal(src, src.remaining());
>> 338: }
> 
> Have you considered changing the argument list of existing 
> update/doFinal(...) methods? Less calls.

I had considered it, but there are a number of methods, particularly in the in 
decrypt operation, where the non-interface methods are called.  I didn't think 
it was right for all those calls to have "null" and "0" for output arguments 
they don't use.
It's possible this interface may be not needed with a future Intel intrinsic, 
so we shall see.

-

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

Re: RFR: 8255557: Decouple GCM from CipherCore [v2]

2021-05-24 Thread Anthony Scarpino 
On Mon, 17 May 2021 22:42:40 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with two 
>> additional commits since the last revision:
>> 
>>  - review comment updates
>>  - Fixed the lack of overlap detection with GCMEncrypt.update()
>
> src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 203:
> 
>> 201: // allocating and copying for direct bytebuffers
>> 202: if (!src.isDirect() && !dst.isDirect() &&
>> 203: !src.isReadOnly() && !dst.isReadOnly()) {
> 
> Why do we need to check for src being isReadOnly() since we are not writing 
> bytes into src? As for dst, if it's read only, then we should probably not 
> proceed further? The other update method which takes ByteBuffer dst did not 
> check if it's read only. A bit inconsistent?

src.array() throws an exception if it's read only

> src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 283:
> 
>> 281: // allocating and copying for direct bytebuffers
>> 282: if (!src.isDirect() && !dst.isDirect() &&
>> 283: !src.isReadOnly() && !dst.isReadOnly()) {
> 
> Same question regarding the isReadOnly() calls as in the update(ByteBuffer, 
> ByteBuffer) method.

src.array() throws an exception if it's read only

-

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

Re: RFR: 8255557: Decouple GCM from CipherCore [v3]

2021-05-24 Thread Anthony Scarpino 
On Tue, 18 May 2021 17:24:13 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   cleanup
>
> src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 189:
> 
>> 187: ct.position(ct.position());
>> 188: return processed;
>> 189: } else if (!ct.isReadOnly()) {
> 
> Maybe you meant "ct.hasArray()" instead of "!ct.isReadOnly()"?

hasArray() checks both isReadonly() and isDirect().  Since I already did 
isDirect() in the previous if, I just need to check readonly here

-

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

Integrated: 8267584: The java.security.krb5.realm system property only needs to be defined once

2021-05-24 Thread Weijun Wang 
On Sun, 23 May 2021 20:42:24 GMT, Weijun Wang  wrote:

> A system property only needs to be put in a `{@systemProperty}` tag where 
> it's first defined. For this one, it's 
> https://github.com/openjdk/jdk/blob/cf21c5ef116c136f09ac5be0d68f02553d0c7a70/src/java.security.jgss/share/classes/javax/security/auth/kerberos/package-info.java#L41.

This pull request has now been integrated.

Changeset: 838a0071
Author:Weijun Wang 
URL:   
https://git.openjdk.java.net/jdk/commit/838a0071030e9c8b9ab57df39a4e0384d433a2bc
Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod

8267584: The java.security.krb5.realm system property only needs to be defined 
once

Reviewed-by: mullan

-

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

Integrated: 8266400: importkeystore fails to a password less pkcs12 keystore

2021-05-24 Thread Hai-May Chao 
On Wed, 19 May 2021 19:01:21 GMT, Hai-May Chao  wrote:

> Please review the fix to address keytool -importkeystore failure when 
> importing to a password-less PKCS12 keystore.

This pull request has now been integrated.

Changeset: f2d880c1
Author:Hai-May Chao 
URL:   
https://git.openjdk.java.net/jdk/commit/f2d880c11a99ef587e7e7c0398f0834c32a22d63
Stats: 92 lines in 2 files changed: 89 ins; 0 del; 3 mod

8266400: importkeystore fails to a password less pkcs12 keystore

Reviewed-by: weijun

-

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

Re: RFR: 8266400: importkeystore fails to a password less pkcs12 keystore

2021-05-24 Thread Weijun Wang 
On Wed, 19 May 2021 19:01:21 GMT, Hai-May Chao  wrote:

> Please review the fix to address keytool -importkeystore failure when 
> importing to a password-less PKCS12 keystore.

Marked as reviewed by weijun (Reviewer).

-

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

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

2021-05-24 Thread 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=4071=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4071=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: 8267584: The java.security.krb5.realm system property only needs to be defined once

2021-05-24 Thread Sean Mullan 
On Sun, 23 May 2021 20:42:24 GMT, Weijun Wang  wrote:

> A system property only needs to be put in a `{@systemProperty}` tag where 
> it's first defined. For this one, it's 
> https://github.com/openjdk/jdk/blob/cf21c5ef116c136f09ac5be0d68f02553d0c7a70/src/java.security.jgss/share/classes/javax/security/auth/kerberos/package-info.java#L41.

Marked as reviewed by mullan (Reviewer).

-

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

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

2021-05-24 Thread 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]

2021-05-24 Thread 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=4073=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4073=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: 8267587: Update java.util to use enhanced switch

2021-05-24 Thread Daniel Fuchs 
On Mon, 24 May 2021 04:20:23 GMT, Tagir F. Valeev  wrote:

> Inspired by PR#4088. Most of the changes are done automatically using 
> IntelliJ IDEA refactoring. Some manual adjustments are also performed, 
> including indentations, moving comments, extracting common cast out of switch 
> expression branches, etc.
> 
> I also noticed that there are some switches having one branch only in 
> JapaneseImperialCalendar.java. Probably it would be better to replace them 
> with `if` statement?

src/java.base/share/classes/java/util/regex/CharPredicates.java line 217:

> 215: case "WORD" -> WORD();
> 216: default -> null;
> 217: };

This file has lots of changes which are difficult to review. Maybe it should be 
split out of this PR.

src/java.base/share/classes/java/util/zip/ZipOutputStream.java line 261:

> 259: ZipEntry e = current.entry;
> 260: switch (e.method) {
> 261: case DEFLATED -> {

Same remark here - it's not clear what's going on.

-

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

Re: RFR: 8267110: Update java.util to use instanceof pattern variable [v5]

2021-05-24 Thread Patrick Concannon 
> Hi,
> 
> Could someone please review my code for updating the code in the `java.util` 
> package to make use of the `instanceof` pattern variable?
> 
> Kind regards,
> Patrick

Patrick Concannon 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 six additional 
commits since the last revision:

 - Merge remote-tracking branch 'origin/master' into JDK-8267110
 - 8267110: Reverted changes made to files in java.util.concurrent
 - Merge remote-tracking branch 'origin/master' into JDK-8267110
 - 8267110: Reverted changes in java/util/Formatter as primitive to boxed types 
may have semantic/performance implications
 - Merge remote-tracking branch 'origin/master' into JDK-8267110
 - 8267110: Update java.util to use instanceof pattern variable

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4088/files
  - new: https://git.openjdk.java.net/jdk/pull/4088/files/2c076a55..9bca9400

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4088=04
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4088=03-04

  Stats: 1931 lines in 65 files changed: 1061 ins; 429 del; 441 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4088.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4088/head:pull/4088

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

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

2021-05-24 Thread Daniel Fuchs 
On Fri, 21 May 2021 20:37:30 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.
>> 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

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.

-

Marked as reviewed by dfuchs (Reviewer).

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