RFR: 8257884: Re-enable sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java as automatic test

[Christoph Langer]

The test sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java caused sporadic 
noise because sometimes it opens more file handles than expected. It was moved 
to a manual test to quiesce this 
([JDK-8257670](https://bugs.openjdk.java.net/browse/JDK-8257670))

It would be good, however, to have this test as an automatic test to be able to 
spot potential regressions. The threshold for windows should be adapted.

-

Commit messages:
 - 8257884: Re-enable sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java as 
automatic test

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

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

Integrated: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305

[Jamil Nimeh]

On Sat, 5 Dec 2020 23:23:47 GMT, Jamil Nimeh  wrote:

> This fix corrects a problem where ChaCha20-Poly1305 objects prior to init 
> throw NPE when getParameters() is called.  It will now generate parameters 
> containing a random nonce on each pre-init call to getParameters(). 
> Post-initialization calls to the getParameters() method will always return 
> the same set of parameters until the next initialization occurs.

This pull request has now been integrated.

Changeset: 500ab457
Author:Jamil Nimeh 
URL:   https://git.openjdk.java.net/jdk/commit/500ab457
Stats: 125 lines in 2 files changed: 119 ins; 0 del; 6 mod

8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305

Reviewed-by: mullan, valeriep

-

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

Re: RFR: 8255917: runtime/cds/SharedBaseAddress.java failed "assert(reserved_rgn != 0LL) failed: No reserved region" [v3]

[Yumin Qi]

> Hi, Please review
>   Windows mapping for file into memory could not happen to reserved memory. 
> In mapping CDS archive we first reserve enough memory then before mapping, 
> release them. For cds archive and using class space, need split the whole 
> space into two, that is, release the whole reserved space and do reservation 
> to the two split spaces again, which is problematic that there is possibility 
> other thread or system can kick in to take the released space.
>   The fix is the first step of two steps:
>1) Do not split reserved memory;
>2) Remove splitting memory.
>This fix is first step, for Windows and use requested mapping address, 
> reserved for cds archive and ccs on a contiguous space separately, so there 
> is no need to call split. If any reservation failed, release them, go to 
> other way, but do not do the 'real' split either. For Windows (and using 
> class space), the reserved space will be released anyway. 
> 
> Tests:tier1-5,tier7

Yumin Qi has updated the pull request incrementally with 32 additional commits 
since the last revision:

 - Add total_space_rs, total reserved space to release_reserved_spaces and 
reserve_address_space_for_archives, made changes to check failed output on test.
 - 8253762: JFR: getField(String) should be able to access subfields
   
   Reviewed-by: mgronlun
 - 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks
   
   Reviewed-by: jnimeh
 - 8257796: [TESTBUG] TestUseSHA512IntrinsicsOptionOnSupportedCPU.java fails on 
x86_32
   
   Reviewed-by: kvn
 - 8257211: C2: Enable call devirtualization during post-parse phase
   
   Reviewed-by: kvn, neliasso, thartmann
 - 8257572: Deprecate the archaic signal-chaining interfaces: sigset and signal
   
   Reviewed-by: ihse, alanb, dcubed, erikj
 - 8257718: LogCompilation: late_inline doesnt work right for JDK 8 logs
   
   Reviewed-by: redestad, kvn
 - 8257799: Update JLS cross-references in java.compiler
   
   Reviewed-by: jjg
 - 8254939: macOS: unused function 'replicate4_imm'
   
   Reviewed-by: redestad, thartmann
 - 8257805: Add compiler/blackhole tests to tier1
   
   Reviewed-by: kvn
 - ... and 22 more: https://git.openjdk.java.net/jdk/compare/dd9ae050...f7958306

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1657/files
  - new: https://git.openjdk.java.net/jdk/pull/1657/files/dd9ae050..f7958306

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

  Stats: 8052 lines in 156 files changed: 4548 ins; 2755 del; 749 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1657.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1657/head:pull/1657

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v4]

[Jamil Nimeh]

> This fix corrects a problem where ChaCha20-Poly1305 objects prior to init 
> throw NPE when getParameters() is called.  It will now generate parameters 
> containing a random nonce on each pre-init call to getParameters(). 
> Post-initialization calls to the getParameters() method will always return 
> the same set of parameters until the next initialization occurs.

Jamil Nimeh has updated the pull request incrementally with one additional 
commit since the last revision:

  Simplfied getNonceFromParams to use IvParameterSpec.getIV()

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1644/files
  - new: https://git.openjdk.java.net/jdk/pull/1644/files/0f74545b..fc2cb52f

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

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

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

Re: RFR: 8257733: Move module-specific data from make to respective module [v2]

[Mandy Chung]

On Mon, 7 Dec 2020 19:31:59 GMT, Jonathan Gibbons  wrote:

>> Magnus Ihse Bursie has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Move to share/data, and move jdwp.spec to java.se
>
> I have reviewed all lines in the patch file with or near instances of 
> `jdk.compiler`

Hi Magnus,

I see the motivation of moving these build files for better identification of 
ownership.   Placing them under
`src/$MODULE/{share,$OS}/data` is one option.  Given that skara will 
automatically determine appropriate mailing lists of a PR, it seems that 
`make/modules/$MODULE/data` can be another alternative that skara can include 
this pattern in the mailing list configuration??   I don't yet have a strong 
preference while I don't consider everything under `make` must be owned by the 
build team though.  Do you see one option is better than the other?

-

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v2]

[Jamil Nimeh]

Oh, one other thing in case you weren't talking about this in the 
context of Cipher and instead were talking about the AlgorithmParameters 
object itself.  If that's all you've got in scope one approach is to 
call getParameterSpec(IvParameterSpec.class).getIV();  That will give 
you the byte array as well.  Now that I think about it, I should 
probably do that in my test code because I went and manually parsed the 
DER encoding.  :)  Glad you brought it up.


--Jamil

On 12/7/2020 5:39 PM, Jamil Nimeh wrote:


Hi Bernd, it's not a stupid question at all.  I think what you might 
be looking for is Cipher.getIV()?  In the case of ChaCha20-Poly1305, 
that method returns the nonce as a byte array where getParameters() 
returns an AlgorithmParameters object where the encoding is consistent 
with RFC 8103.


But both getParameters() and getIV() were doing the wrong thing 
(chucking NPE) when they should've either come up with a random param 
or null, respectively when in a pre-initialized state.


--Jamil

On 12/7/2020 5:19 PM, Bernd Eckenfels wrote:
BTW stupid - somewhat related - question, why does the nonce to be 
parsed out of a DER blob, shouldn’t there be an getter on the 
Parameter Spec object? Many protocols would need the raw array, is 
there a matching spec - or should we add one?


Gruss
Bernd
--
http://bernd.eckenfels.net

*Von:* security-dev  im Auftrag 
von Jamil Nimeh 

*Gesendet:* Monday, December 7, 2020 9:05:16 PM
*An:* security-dev@openjdk.java.net 
*Betreff:* Re: RFR: 8257769: Cipher.getParameters() throws NPE for 
ChaCha20-Poly1305 [v2]
On Mon, 7 Dec 2020 19:53:27 GMT, Valerie Peng  
wrote:


>> Jamil Nimeh has updated the pull request incrementally with one 
additional commit since the last revision:

>>
>>   pre-init getParameters nonce data is now a local variable
>
> 
src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java 
line 232:

>
>> 230: // this call should cause a random nonce to be 
generated, but

>> 231: // not attached to the object.
>> 232: byte[] nonceData = initialized ? nonce : 
createRandomNonce(null);

>
> The "initialized" variable is set to false in engineDoFinal() call. 
So, if users call getParameters() after finish cipher operation, this 
will return random nonces instead of the one used in previous doFinal 
operation. Will this be a little un-intuitive?


Unintuitive is a charitable way to put it.  After doFinal the Cipher 
technically isn't in an uninitialized state per the spec, it's 
supposed to be in the state it would be immediately following 
init().  So the wrong behavior would happen in this use case.  Will fix.


-

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v2]

[Jamil Nimeh]

Hi Bernd, it's not a stupid question at all.  I think what you might be 
looking for is Cipher.getIV()?  In the case of ChaCha20-Poly1305, that 
method returns the nonce as a byte array where getParameters() returns 
an AlgorithmParameters object where the encoding is consistent with RFC 
8103.


But both getParameters() and getIV() were doing the wrong thing 
(chucking NPE) when they should've either come up with a random param or 
null, respectively when in a pre-initialized state.


--Jamil

On 12/7/2020 5:19 PM, Bernd Eckenfels wrote:
BTW stupid - somewhat related - question, why does the nonce to be 
parsed out of a DER blob, shouldn’t there be an getter on the 
Parameter Spec object? Many protocols would need the raw array, is 
there a matching spec - or should we add one?


Gruss
Bernd
--
http://bernd.eckenfels.net

*Von:* security-dev  im Auftrag 
von Jamil Nimeh 

*Gesendet:* Monday, December 7, 2020 9:05:16 PM
*An:* security-dev@openjdk.java.net 
*Betreff:* Re: RFR: 8257769: Cipher.getParameters() throws NPE for 
ChaCha20-Poly1305 [v2]
On Mon, 7 Dec 2020 19:53:27 GMT, Valerie Peng  
wrote:


>> Jamil Nimeh has updated the pull request incrementally with one 
additional commit since the last revision:

>>
>>   pre-init getParameters nonce data is now a local variable
>
> 
src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java 
line 232:

>
>> 230: // this call should cause a random nonce to be 
generated, but

>> 231: // not attached to the object.
>> 232: byte[] nonceData = initialized ? nonce : 
createRandomNonce(null);

>
> The "initialized" variable is set to false in engineDoFinal() call. 
So, if users call getParameters() after finish cipher operation, this 
will return random nonces instead of the one used in previous doFinal 
operation. Will this be a little un-intuitive?


Unintuitive is a charitable way to put it.  After doFinal the Cipher 
technically isn't in an uninitialized state per the spec, it's 
supposed to be in the state it would be immediately following init().  
So the wrong behavior would happen in this use case.  Will fix.


-

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v2]

[Bernd Eckenfels]

BTW stupid - somewhat related - question, why does the nonce to be parsed out 
of a DER blob, shouldn’t there be an getter on the Parameter Spec object? Many 
protocols would need the raw array, is there a matching spec - or should we add 
one?

Gruss
Bernd
--
http://bernd.eckenfels.net

Von: security-dev  im Auftrag von Jamil 
Nimeh 
Gesendet: Monday, December 7, 2020 9:05:16 PM
An: security-dev@openjdk.java.net 
Betreff: Re: RFR: 8257769: Cipher.getParameters() throws NPE for 
ChaCha20-Poly1305 [v2]

On Mon, 7 Dec 2020 19:53:27 GMT, Valerie Peng  wrote:

>> Jamil Nimeh has updated the pull request incrementally with one additional 
>> commit since the last revision:
>>
>>   pre-init getParameters nonce data is now a local variable
>
> src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java line 
> 232:
>
>> 230: // this call should cause a random nonce to be generated, 
>> but
>> 231: // not attached to the object.
>> 232: byte[] nonceData = initialized ? nonce : 
>> createRandomNonce(null);
>
> The "initialized" variable is set to false in engineDoFinal() call. So, if 
> users call getParameters() after finish cipher operation, this will return 
> random nonces instead of the one used in previous doFinal operation. Will 
> this be a little un-intuitive?

Unintuitive is a charitable way to put it.  After doFinal the Cipher 
technically isn't in an uninitialized state per the spec, it's supposed to be 
in the state it would be immediately following init().  So the wrong behavior 
would happen in this use case.  Will fix.

-

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

Re: RFR: 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks [v2]

[Xue-Lei Andrew Fan]

> This test case, sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java, may be not 
> reliable if there are some other test cases or applications running at the 
> same time.  It's a good manual test, but might be not suitable for OpenJDK 
> automation regression test if it could be impacted.  Move it to manual test.

Xue-Lei Andrew Fan has updated the pull request incrementally with one 
additional commit since the last revision:

  typo correction

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1681/files
  - new: https://git.openjdk.java.net/jdk/pull/1681/files/4324892c..58ec75fa

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

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

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

Integrated: 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks

[Xue-Lei Andrew Fan]

On Mon, 7 Dec 2020 23:35:49 GMT, Xue-Lei Andrew Fan  wrote:

> This test case, sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java, may be not 
> reliable if there are some other test cases or applications running at the 
> same time.  It's a good manual test, but might be not suitable for OpenJDK 
> automation regression test if it could be impacted.  Move it to manual test.

This pull request has now been integrated.

Changeset: 39b8a2e6
Author:Xue-Lei Andrew Fan 
URL:   https://git.openjdk.java.net/jdk/commit/39b8a2e6
Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod

8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks

Reviewed-by: jnimeh

-

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

Re: RFR: 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks [v2]

[Xue-Lei Andrew Fan]

On Mon, 7 Dec 2020 23:44:41 GMT, Jamil Nimeh  wrote:

>> Xue-Lei Andrew Fan has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   typo correction
>
> test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java line 39:
> 
>> 37:  * @run main/manual SSLSocketLeak
>> 38:  */
>> 39: // Note: this test is not reliabe, run it manually.
> 
> Typo reliabe -> reliable

Thank you! Corrected.

-

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

Re: RFR: 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks

[Christoph Langer]

On Mon, 7 Dec 2020 23:35:49 GMT, Xue-Lei Andrew Fan  wrote:

> This test case, sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java, may be not 
> reliable if there are some other test cases or applications running at the 
> same time.  It's a good manual test, but might be not suitable for OpenJDK 
> automation regression test if it could be impacted.  Move it to manual test.

Did you check whether the test failure is maybe specific to windows? Comments 
at the JBS issue are suggesting that...

-

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

Re: RFR: 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks

[Jamil Nimeh]

On Mon, 7 Dec 2020 23:35:49 GMT, Xue-Lei Andrew Fan  wrote:

> This test case, sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java, may be not 
> reliable if there are some other test cases or applications running at the 
> same time.  It's a good manual test, but might be not suitable for OpenJDK 
> automation regression test if it could be impacted.  Move it to manual test.

Looks fine to me.

test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java line 39:

> 37:  * @run main/manual SSLSocketLeak
> 38:  */
> 39: // Note: this test is not reliabe, run it manually.

Typo reliabe -> reliable

-

Marked as reviewed by jnimeh (Reviewer).

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

RFR: 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks

[Xue-Lei Andrew Fan]

This test case, sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java, may be not 
reliable if there are some other test cases or applications running at the same 
time.  It's a good manual test, but might be not suitable for OpenJDK 
automation regression test if it could be impacted.  Move it to manual test.

-

Commit messages:
 - 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks

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

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v3]

[Valerie Peng]

On Mon, 7 Dec 2020 22:32:23 GMT, Jamil Nimeh  wrote:

>> This fix corrects a problem where ChaCha20-Poly1305 objects prior to init 
>> throw NPE when getParameters() is called.  It will now generate parameters 
>> containing a random nonce on each pre-init call to getParameters(). 
>> Post-initialization calls to the getParameters() method will always return 
>> the same set of parameters until the next initialization occurs.
>
> Jamil Nimeh has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix minor issues in getIV() and post-encryption getParameters() behavior

Marked as reviewed by valeriep (Reviewer).

-

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v3]

[Jamil Nimeh]

> This fix corrects a problem where ChaCha20-Poly1305 objects prior to init 
> throw NPE when getParameters() is called.  It will now generate parameters 
> containing a random nonce on each pre-init call to getParameters(). 
> Post-initialization calls to the getParameters() method will always return 
> the same set of parameters until the next initialization occurs.

Jamil Nimeh has updated the pull request incrementally with one additional 
commit since the last revision:

  Fix minor issues in getIV() and post-encryption getParameters() behavior

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1644/files
  - new: https://git.openjdk.java.net/jdk/pull/1644/files/b9a24482..0f74545b

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

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

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v2]

[Jamil Nimeh]

On Mon, 7 Dec 2020 19:53:27 GMT, Valerie Peng  wrote:

>> Jamil Nimeh has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   pre-init getParameters nonce data is now a local variable
>
> src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java line 
> 232:
> 
>> 230: // this call should cause a random nonce to be generated, 
>> but
>> 231: // not attached to the object.
>> 232: byte[] nonceData = initialized ? nonce : 
>> createRandomNonce(null);
> 
> The "initialized" variable is set to false in engineDoFinal() call. So, if 
> users call getParameters() after finish cipher operation, this will return 
> random nonces instead of the one used in previous doFinal operation. Will 
> this be a little un-intuitive?

Unintuitive is a charitable way to put it.  After doFinal the Cipher 
technically isn't in an uninitialized state per the spec, it's supposed to be 
in the state it would be immediately following init().  So the wrong behavior 
would happen in this use case.  Will fix.

-

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v2]

[Jamil Nimeh]

On Mon, 7 Dec 2020 19:35:27 GMT, Sean Mullan  wrote:

>> Jamil Nimeh has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   pre-init getParameters nonce data is now a local variable
>
> src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java line 
> 232:
> 
>> 230: // this call should cause a random nonce to be generated, 
>> but
>> 231: // not attached to the object.
>> 232: byte[] nonceData = initialized ? nonce : 
>> createRandomNonce(null);
> 
> A minor unrelated comment is that createRandomNonce() could be made static.

Easy enough to do.

-

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v2]

[Valerie Peng]

On Mon, 7 Dec 2020 17:50:25 GMT, Jamil Nimeh  wrote:

>> This fix corrects a problem where ChaCha20-Poly1305 objects prior to init 
>> throw NPE when getParameters() is called.  It will now generate parameters 
>> containing a random nonce on each pre-init call to getParameters(). 
>> Post-initialization calls to the getParameters() method will always return 
>> the same set of parameters until the next initialization occurs.
>
> Jamil Nimeh has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   pre-init getParameters nonce data is now a local variable

src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java line 
232:

> 230: // this call should cause a random nonce to be generated, but
> 231: // not attached to the object.
> 232: byte[] nonceData = initialized ? nonce : 
> createRandomNonce(null);

The "initialized" variable is set to false in engineDoFinal() call. So, if 
users call getParameters() after finish cipher operation, this will return 
random nonces instead of the one used in previous doFinal operation. Will this 
be a little un-intuitive?

-

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v2]

[Valerie Peng]

On Mon, 7 Dec 2020 17:50:25 GMT, Jamil Nimeh  wrote:

>> This fix corrects a problem where ChaCha20-Poly1305 objects prior to init 
>> throw NPE when getParameters() is called.  It will now generate parameters 
>> containing a random nonce on each pre-init call to getParameters(). 
>> Post-initialization calls to the getParameters() method will always return 
>> the same set of parameters until the next initialization occurs.
>
> Jamil Nimeh has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   pre-init getParameters nonce data is now a local variable

Outside of Cipher.getParameters(), there are other direct references of nonce 
which may lead to NPE which we should fix also. Inside engineGetIV(), it should 
check for null nonce before directly calling clone().

-

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v2]

[Sean Mullan]

On Mon, 7 Dec 2020 19:35:33 GMT, Sean Mullan  wrote:

>> Jamil Nimeh has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   pre-init getParameters nonce data is now a local variable
>
> Marked as reviewed by mullan (Reviewer).

The test should probably have the bugid added to it.

-

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v2]

[Sean Mullan]

On Mon, 7 Dec 2020 17:50:25 GMT, Jamil Nimeh  wrote:

>> This fix corrects a problem where ChaCha20-Poly1305 objects prior to init 
>> throw NPE when getParameters() is called.  It will now generate parameters 
>> containing a random nonce on each pre-init call to getParameters(). 
>> Post-initialization calls to the getParameters() method will always return 
>> the same set of parameters until the next initialization occurs.
>
> Jamil Nimeh has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   pre-init getParameters nonce data is now a local variable

Marked as reviewed by mullan (Reviewer).

src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java line 
232:

> 230: // this call should cause a random nonce to be generated, but
> 231: // not attached to the object.
> 232: byte[] nonceData = initialized ? nonce : 
> createRandomNonce(null);

A minor unrelated comment is that createRandomNonce() could be made static.

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module [v2]

[Jonathan Gibbons]

On Mon, 7 Dec 2020 14:27:45 GMT, Magnus Ihse Bursie  wrote:

>> A lot (but not all) of the data in make/data is tied to a specific module. 
>> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
>> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
>> make for the whole build.) 
>> 
>> These data files should move to the module they belong to. The are, after 
>> all, "source code" for that module that is "compiler" into resulting 
>> deliverables, for that module. (But the "source code" language is not Java 
>> or C, but typically a highly domain specific language or data format, and 
>> the "compilation" is, often, a specialized transformation.) 
>> 
>> This misplacement of the data directory is most visible at code review time. 
>> When such data is changed, most of the time build-dev (or the new build 
>> label) is involved, even though this has nothing to do with the build. While 
>> this is annoying, a worse problem is if the actual team that needs to review 
>> the patch (i.e., the team owning the module) is missed in the review.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Move to share/data, and move jdwp.spec to java.se

I have reviewed all lines in the patch file with or near instances of 
`jdk.compiler`

-

Marked as reviewed by jjg (Reviewer).

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305 [v2]

[Jamil Nimeh]

> This fix corrects a problem where ChaCha20-Poly1305 objects prior to init 
> throw NPE when getParameters() is called.  It will now generate parameters 
> containing a random nonce on each pre-init call to getParameters(). 
> Post-initialization calls to the getParameters() method will always return 
> the same set of parameters until the next initialization occurs.

Jamil Nimeh has updated the pull request incrementally with one additional 
commit since the last revision:

  pre-init getParameters nonce data is now a local variable

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1644/files
  - new: https://git.openjdk.java.net/jdk/pull/1644/files/56fd18c7..b9a24482

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

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

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305

[Jamil Nimeh]

On Mon, 7 Dec 2020 15:42:19 GMT, Sean Mullan  wrote:

>> This fix corrects a problem where ChaCha20-Poly1305 objects prior to init 
>> throw NPE when getParameters() is called.  It will now generate parameters 
>> containing a random nonce on each pre-init call to getParameters(). 
>> Post-initialization calls to the getParameters() method will always return 
>> the same set of parameters until the next initialization occurs.
>
> src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java line 
> 232:
> 
>> 230: // this call should cause a random nonce to be generated.
>> 231: if (!initialized || nonce == null) {
>> 232: nonce = createRandomNonce(null);
> 
> Should nonce be a local variable instead? I think you don't want the nonce 
> field to be set unless the caller passes back in the returned params in an 
> init call.

Yes, it should be a local variable.  I'll fix this.

-

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

Integrated: 8257788: Class fields could be local in the SunJSSE provider

[Xue-Lei Andrew Fan]

On Fri, 4 Dec 2020 22:58:07 GMT, Xue-Lei Andrew Fan  wrote:

> In the SunJSSE provider implementation, there are a few class fields are not 
> used other than the constructors. Those fields could be removed and replaced 
> with local variables.
> 
> Bug: https://bugs.openjdk.java.net/browse/JDK-8257788

This pull request has now been integrated.

Changeset: dcf63f85
Author:Xue-Lei Andrew Fan 
URL:   https://git.openjdk.java.net/jdk/commit/dcf63f85
Stats: 15 lines in 4 files changed: 0 ins; 11 del; 4 mod

8257788: Class fields could be local in the SunJSSE provider

Reviewed-by: shade

-

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

Re: RFR: 8257769: Cipher.getParameters() throws NPE for ChaCha20-Poly1305

[Sean Mullan]

On Sat, 5 Dec 2020 23:23:47 GMT, Jamil Nimeh  wrote:

> This fix corrects a problem where ChaCha20-Poly1305 objects prior to init 
> throw NPE when getParameters() is called.  It will now generate parameters 
> containing a random nonce on each pre-init call to getParameters(). 
> Post-initialization calls to the getParameters() method will always return 
> the same set of parameters until the next initialization occurs.

src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java line 
232:

> 230: // this call should cause a random nonce to be generated.
> 231: if (!initialized || nonce == null) {
> 232: nonce = createRandomNonce(null);

Should nonce be a local variable instead? I think you don't want the nonce 
field to be set unless the caller passes back in the returned params in an init 
call.

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module [v2]

[Magnus Ihse Bursie]

> A lot (but not all) of the data in make/data is tied to a specific module. 
> For instance, the publicsuffixlist is used by java.base, and fontconfig by 
> java.desktop. (A few directories, like mainmanifest, is *actually* used by 
> make for the whole build.) 
> 
> These data files should move to the module they belong to. The are, after 
> all, "source code" for that module that is "compiler" into resulting 
> deliverables, for that module. (But the "source code" language is not Java or 
> C, but typically a highly domain specific language or data format, and the 
> "compilation" is, often, a specialized transformation.) 
> 
> This misplacement of the data directory is most visible at code review time. 
> When such data is changed, most of the time build-dev (or the new build 
> label) is involved, even though this has nothing to do with the build. While 
> this is annoying, a worse problem is if the actual team that needs to review 
> the patch (i.e., the team owning the module) is missed in the review.

Magnus Ihse Bursie has updated the pull request incrementally with one 
additional commit since the last revision:

  Move to share/data, and move jdwp.spec to java.se

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1611/files
  - new: https://git.openjdk.java.net/jdk/pull/1611/files/8e775e40..f0047704

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

  Stats: 56 lines in 1565 files changed: 1 ins; 0 del; 55 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1611.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1611/head:pull/1611

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

On Mon, 7 Dec 2020 14:20:07 GMT, Magnus Ihse Bursie  wrote:

>> @erikj79 Good point, that makes much more sense.
>
> I'm not sure about the formal process for suggesting changes to a delivered 
> JEP, but this is the text I suggest should replace the current definition of 
> the new scheme:
> 
> src/$MODULE/{share,$OS}/classes/$PACKAGE/*.java
> native/include/*.{h,hpp}
>$LIBRARY/*.{c,cpp}
> conf/*
> legal/*
> data/*
> man/*
> lib/*
> 
> where:
> 
>   - $MODULE is a module name (_e.g._, `java.base`);
> 
>   - The `share` directory contains shared, cross-platform code, as
> before;
> 
>   - The `$OS` directory contains operating-system-specific code, as
> before, where `$OS` is one of `unix`, `windows`, _etc._;
> 
>   - The `classes` directory contains Java source files and resource files
> organized into a directory tree reflecting their API `$PACKAGE`
> hierarchy, as before;
> 
>   - The `native` directory contains C or C++ source files, as before but
> organized differently:
> 
> - The `include` directory contains C or C++ header files intended to
>   be exported for external use (_e.g._, `jni.h`);
> 
> - C or C++ source files are placed in a `$LIBRARY` directory, whose
>   name is that of the shared library or DLL into which the compiled
>   code will be linked (_e.g._, `libjava` or `libawt`); and, finally,
> 
>   - The `conf` directory contains configuration files meant to be edited
> by end users (_e.g._, `net.properties`).
> 
>   - The `legal` directory contains legal notices.
> 
>   - The `data` directory contains data files needed for building the module.
> 
>   - The `man` directory contains man pages in nroff or markdown format.
> 
>   - The `lib` directory contains configuration files not meant to be edited
> by end users.
> 
> Rendered as markdown, it would look somewhat like this:
> 
> src/$MODULE/{share,$OS}/classes/$PACKAGE/*.java
> native/include/*.{h,hpp}
>$LIBRARY/*.{c,cpp}
> conf/*
> legal/*
> data/*
> man/*
> lib/*
> 
> where:
> 
>   - $MODULE is a module name (_e.g._, `java.base`);
> 
>   - The `share` directory contains shared, cross-platform code, as
> before;
> 
>   - The `$OS` directory contains operating-system-specific code, as
> before, where `$OS` is one of `unix`, `windows`, _etc._;
> 
>   - The `classes` directory contains Java source files and resource files
> organized into a directory tree reflecting their API `$PACKAGE`
> hierarchy, as before;
> 
>   - The `native` directory contains C or C++ source files, as before but
> organized differently:
> 
> - The `include` directory contains C or C++ header files intended to
>   be exported for external use (_e.g._, `jni.h`);
> 
> - C or C++ source files are placed in a `$LIBRARY` directory, whose
>   name is that of the shared library or DLL into which the compiled
>   code will be linked (_e.g._, `libjava` or `libawt`); and, finally,
> 
>   - The `conf` directory contains configuration files meant to be edited
> by end users (_e.g._, `net.properties`).
> 
>   - The `legal` directory contains legal notices.
> 
>   - The `data` directory contains data files needed for building the module.
> 
>   - The `man` directory contains man pages in nroff or markdown format.
> 
>   - The `lib` directory contains configuration files not meant to be edited
> by end users.

I hope I understood the purpose of the `lib` directory correctly. Our only 
instance of this is for `java.base/*/lib/security/default.policy`.

I also noted that jdk.hotspot.agent violates this scheme by having a top-level 
`test` and `doc` directories, apart from `share` and the OS directories. The 
purposes of these two directories are not clear to me. The tests in `test` are 
definitely never executed. I don't know if this is an omission, or if they 
should be removed. The documentation in `doc` is not exported to the end 
product, nor is it references in any developer documentation. That too should 
either be removed, or moved to a more suitable home.

-

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

Re: RFR: 8257733: Move module-specific data from make to respective module

[Magnus Ihse Bursie]

On Fri, 4 Dec 2020 15:17:06 GMT, Magnus Ihse Bursie  wrote:

>> Regarding the chosen layout. Did you consider following the existing pattern 
>> of src//{share,}/data?
>
> @erikj79 Good point, that makes much more sense.

I'm not sure about the formal process for suggesting changes to a delivered 
JEP, but this is the text I suggest should replace the current definition of 
the new scheme:

src/$MODULE/{share,$OS}/classes/$PACKAGE/*.java
native/include/*.{h,hpp}
   $LIBRARY/*.{c,cpp}
conf/*
legal/*
data/*
man/*
lib/*

where:

  - $MODULE is a module name (_e.g._, `java.base`);

  - The `share` directory contains shared, cross-platform code, as
before;

  - The `$OS` directory contains operating-system-specific code, as
before, where `$OS` is one of `unix`, `windows`, _etc._;

  - The `classes` directory contains Java source files and resource files
organized into a directory tree reflecting their API `$PACKAGE`
hierarchy, as before;

  - The `native` directory contains C or C++ source files, as before but
organized differently:

- The `include` directory contains C or C++ header files intended to
  be exported for external use (_e.g._, `jni.h`);

- C or C++ source files are placed in a `$LIBRARY` directory, whose
  name is that of the shared library or DLL into which the compiled
  code will be linked (_e.g._, `libjava` or `libawt`); and, finally,

  - The `conf` directory contains configuration files meant to be edited
by end users (_e.g._, `net.properties`).

  - The `legal` directory contains legal notices.

  - The `data` directory contains data files needed for building the module.

  - The `man` directory contains man pages in nroff or markdown format.

  - The `lib` directory contains configuration files not meant to be edited
by end users.

Rendered as markdown, it would look somewhat like this:

src/$MODULE/{share,$OS}/classes/$PACKAGE/*.java
native/include/*.{h,hpp}
   $LIBRARY/*.{c,cpp}
conf/*
legal/*
data/*
man/*
lib/*

where:

  - $MODULE is a module name (_e.g._, `java.base`);

  - The `share` directory contains shared, cross-platform code, as
before;

  - The `$OS` directory contains operating-system-specific code, as
before, where `$OS` is one of `unix`, `windows`, _etc._;

  - The `classes` directory contains Java source files and resource files
organized into a directory tree reflecting their API `$PACKAGE`
hierarchy, as before;

  - The `native` directory contains C or C++ source files, as before but
organized differently:

- The `include` directory contains C or C++ header files intended to
  be exported for external use (_e.g._, `jni.h`);

- C or C++ source files are placed in a `$LIBRARY` directory, whose
  name is that of the shared library or DLL into which the compiled
  code will be linked (_e.g._, `libjava` or `libawt`); and, finally,

  - The `conf` directory contains configuration files meant to be edited
by end users (_e.g._, `net.properties`).

  - The `legal` directory contains legal notices.

  - The `data` directory contains data files needed for building the module.

  - The `man` directory contains man pages in nroff or markdown format.

  - The `lib` directory contains configuration files not meant to be edited
by end users.

-

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

Re: RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner

[Conor Cleary]

On Mon, 7 Dec 2020 11:08:37 GMT, Conor Cleary  wrote:

>> test/jdk/sun/net/www/http/KeepAliveStreamCleaner/KeepAliveStreamCleanerTestDriver.java
>>  line 26:
>> 
>>> 24: /*
>>> 25:  * @test
>>> 26:  * @modules java.base/sun.net.www.http
>> 
>> Hi Conor,
>> 
>> Can you add:
>> 
>> @bug 8255124
>> 
>> here? [8255124](https://bugs.openjdk.java.net/browse/JDK-8255124) is the 
>> issue whose fix this test is verifying.
>> 
>> best regards,
>> 
>> -- daniel
>
> Will do Daniel, thanks for pointing that out!

Should probably add a short summary as well

-

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

Re: RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner

[Conor Cleary]

On Mon, 7 Dec 2020 10:55:06 GMT, Daniel Fuchs  wrote:

>> The KeepAliveStreamCleaner in sun.net.ww.http package had been previously 
>> seen to fail with an IllegalMonitorStateException. This failure was caused 
>> by the use of `wait()` in a non synchronized block. This failure was 
>> mitigated through use of `await()` instead as is shown below (code can be 
>> viewed on L119 in 
>> src/java.base/share/classes/sun/net/www/http/KeepAliveStreamCleaner.java).
>> 
>> ...
>> long timeout = TIMEOUT;
>> while ((kace = poll()) == null) {
>> waiter.await(timeout, TimeUnit.MILLISECONDS); 
>> ...
>> While the code throwing the exception was fixed, a regression test was not 
>> made. So this patch adds a simple white-box test which calls the 
>> KeepAliveStreamCleaner's run method. `waiter.wait()` should always fail if 
>> not used in a synchronized block or method, so the test verifies that it is 
>> not used.
>
> test/jdk/sun/net/www/http/KeepAliveStreamCleaner/KeepAliveStreamCleanerTestDriver.java
>  line 26:
> 
>> 24: /*
>> 25:  * @test
>> 26:  * @modules java.base/sun.net.www.http
> 
> Hi Conor,
> 
> Can you add:
> 
> @bug 8255124
> 
> here? [8255124](https://bugs.openjdk.java.net/browse/JDK-8255124) is the 
> issue whose fix this test is verifying.
> 
> best regards,
> 
> -- daniel

Will do Daniel, thanks for pointing that out!

-

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

Re: RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner

[Daniel Fuchs]

On Mon, 7 Dec 2020 09:52:47 GMT, Conor Cleary  wrote:

> The KeepAliveStreamCleaner in sun.net.ww.http package had been previously 
> seen to fail with an IllegalMonitorStateException. This failure was caused by 
> the use of `wait()` in a non synchronized block. This failure was mitigated 
> through use of `await()` instead as is shown below (code can be viewed on 
> L119 in 
> src/java.base/share/classes/sun/net/www/http/KeepAliveStreamCleaner.java).
> 
> ...
> long timeout = TIMEOUT;
> while ((kace = poll()) == null) {
> waiter.await(timeout, TimeUnit.MILLISECONDS); 
> ...
> While the code throwing the exception was fixed, a regression test was not 
> made. So this patch adds a simple white-box test which calls the 
> KeepAliveStreamCleaner's run method. `waiter.wait()` should always fail if 
> not used in a synchronized block or method, so the test verifies that it is 
> not used.

test/jdk/sun/net/www/http/KeepAliveStreamCleaner/KeepAliveStreamCleanerTestDriver.java
 line 26:

> 24: /*
> 25:  * @test
> 26:  * @modules java.base/sun.net.www.http

Hi Conor,

Can you add:

@bug 8255124

here? [8255124](https://bugs.openjdk.java.net/browse/JDK-8255124) is the issue 
whose fix this test is verifying.

best regards,

-- daniel

-

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

RFR: 8255583: Investigate creating a test to trigger the condition in KeepAliveStreamCleaner

[Conor Cleary]

The KeepAliveStreamCleaner in sun.net.ww.http package had been previously seen 
to fail with an IllegalMonitorStateException. This failure was caused by the 
use of `wait()` in a non synchronized block. This failure was mitigated through 
use of `await()` instead as is shown below (code can be viewed on L119 in 
src/java.base/share/classes/sun/net/www/http/KeepAliveStreamCleaner.java).

...
long timeout = TIMEOUT;
while ((kace = poll()) == null) {
waiter.await(timeout, TimeUnit.MILLISECONDS); 
...
While the code throwing the exception was fixed, a regression test was not 
made. So this patch adds a simple white-box test which calls the 
KeepAliveStreamCleaner's run method. `waiter.wait()` should always fail if not 
used in a synchronized block or method, so the test verifies that it is not 
used.

-

Commit messages:
 - 8255583: Updated copyright notes
 - 8255583: Investigate creating a test to trigger the condition in 
KeepAliveStreamCleaner

Changes: https://git.openjdk.java.net/jdk/pull/1659/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1659&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255583
  Stats: 49 lines in 2 files changed: 42 ins; 6 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1659.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1659/head:pull/1659

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