Integrated: 8240256: Better resource cleaning for SunPKCS11 Provider

[Sean Coffey]

On Fri, 16 Apr 2021 11:24:57 GMT, Sean Coffey  wrote:

> Added capability to allow the PKCS11 Token to be destroyed once a session is 
> logged out from. New configuration properties via pkcs11 config file. Cleaned 
> up the native resource poller also.
> 
> New unit test case to test behaviour. Some PKCS11 tests refactored to allow 
> pkcs11 provider to be configured (and tested) with a config file of choice.
> 
> Reviewer request @valeriepeng

This pull request has now been integrated.

Changeset: bdeaeb47
Author:Sean Coffey 
URL:   
https://git.openjdk.java.net/jdk/commit/bdeaeb47d0155b9f233274cff90334e8dd761aae
Stats: 573 lines in 11 files changed: 467 ins; 68 del; 38 mod

8240256: Better resource cleaning for SunPKCS11 Provider

Reviewed-by: valeriep

-

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

Re: Authorization Layer post JEP 411

[Chapman Flack]

On 06/02/21 19:41, Peter Firmstone wrote:
> We need the power of AccessController's stack walk, StackWalker doesn't work
> with compiled code, only bytecode.

Is this correct? I have not tried it, but the apidocs had led me to
understand it did not distinguish much between JITed and interpreted code.
Even getByteCodeIndex does not mention any limitation when the frame is
JITed Java code (though it does say it will return a negative number in
the distinct case of an actual native method).

Regards,
-Chap

Authorization Layer post JEP 411

[Peter Firmstone]

A comment from Ron highlites our issue:


the JDK contains only things that either only the JDK can technically do


We have a need to distinguish between different sources of code, as well 
as user principles, and as well as Services.   Our services are loaded 
by separate ClassLoaders and are to some extent sandboxed as Ron's 
example suggests.


And we have a need to control access based on these entities.

We have always strived to be cross platform and tested on other JVM's 
such as J9.


It's just very hard to see any solutions without AccessController and 
AccessControlContext.


We don't need SecurityManager (although we still need a Policy provider, 
because ProtectionDomain calls it, but we don't need a policy 
implementation, just the provider, feel free to remove Java's PolicyFile 
implementation), if we added a provider interface to Guard.check and 
changed all permission checks to call their superclass method Guard.check.


That authorization layer provider could be called Authority and it can 
have one single method:


Authority::confirm(Permission p) throws SecurityException;

We need the power of AccessController's stack walk, StackWalker doesn't 
work with compiled code, only bytecode.


AccessController and AccessControlContext allow backward compatiblity 
for JAAS.   JAAS whether we like it or not, is the default authorisation 
layer framework.


http://word-bits.flurg.com/jaas-is-terrible-and-there-is-no-escape-from-it/

We could create a new property that bypasses the AccessController's 
stack walk for those who don't need to control CodeSource access. (Just 
create a ProtectionDomain containing a Subject).


Benefits:

With SecurityManager gone, people will no longer assume it has sole 
responsible for Security and OpenJDK devs won't carry a significant 
burden for it's maintenance.  Any security issues will be the 
responsibility of third party implementations, like mine.


The JDK won't provide an implementation, just the framework.

Those of us using the Principle of Least Privilege can continue to do so 
and we can participate in OpenJDK to maintain Permission checks where we 
need them and preserve context where appropriate.


JAAS will continue to remain functional and it's performance will 
increase significantly (it performs very well with my Policy 
implementation, even with stack walks).


--
Regards,
 
Peter Firmstone

Zeus Project Services Pty Ltd.

Re: RFR: 8267485: Remove the dependency on SecurityManager in JceSecurityManager.java [v3]

[Bradford Wetmore]

> The JceSecurityManager is currently a subclass of 
> java.security.SecurityManager.  Now that JEP 411 has been integrated, this 
> class should be updated to no longer subclass SecurityManager.
> 
> The only reason for using SecurityManager to easily get the Class Context 
> (call stack), but we can achieve the same effect by using the JDK 9 API 
> java.lang.StackWalkeer.  None of the other SecurityManager API are used.
> 
> I have run mach5 tier1/tier2 plus --test 
> jck:api/java_security,jck:api/javax_crypto,jck:api/javax_net,jck:api/javax_security,jck:api/org_ietf,jck:api/javax_xml/crypto
>  with all green.

Bradford Wetmore has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains eight commits:

 - Address codereview comments
 - Merge branch 'master' into JDK-8267485
 - Merge branch 'master' into JDK-8267485
 - Merge branch 'master' into JDK-8267485
 - Replace missing annotation
 - Merge branch 'master' into JDK-8267485
 - Updated copyright date.
 - 8267485: Remove the dependency on SecurityManager in JceSecurityManager.java

-

Changes: https://git.openjdk.java.net/jdk/pull/4150/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4150&range=02
  Stats: 24 lines in 1 file changed: 14 ins; 2 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4150.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4150/head:pull/4150

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

Re: RFR: 8267485: Remove the dependency on SecurityManager in JceSecurityManager.java [v2]

[Bradford Wetmore]

On Wed, 2 Jun 2021 18:18:46 GMT, Daniel Fuchs  wrote:

>> Bradford Wetmore has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains six commits:
>> 
>>  - Merge branch 'master' into JDK-8267485
>>  - Merge branch 'master' into JDK-8267485
>>  - Replace missing annotation
>>  - Merge branch 'master' into JDK-8267485
>>  - Updated copyright date.
>>  - 8267485: Remove the dependency on SecurityManager in 
>> JceSecurityManager.java
>
> src/java.base/share/classes/javax/crypto/JceSecurityManager.java line 107:
> 
>> 105: List stack = StackWalker.getInstance(
>> 106: StackWalker.Option.RETAIN_CLASS_REFERENCE)
>> 107: .walk((s) -> s.collect(Collectors.toList()));
> 
> StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE) will 
> require a permission check.
> As long as the SecurityManager is still functional, doesn't this mean that 
> creating the StackWalker should be performed in a doPrivileged? If so maybe 
> it should be done in a (possibly static) initializer. Or is it intentional to 
> check that the caller (and the whole calling stack) posses the 
> `RuntimePermission("getStackWalkerWithClassReference")`?

Good catch, thanks.

-

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

Re: RFR: 8267485: Remove the dependency on SecurityManager in JceSecurityManager.java [v2]

[Bradford Wetmore]

> The JceSecurityManager is currently a subclass of 
> java.security.SecurityManager.  Now that JEP 411 has been integrated, this 
> class should be updated to no longer subclass SecurityManager.
> 
> The only reason for using SecurityManager to easily get the Class Context 
> (call stack), but we can achieve the same effect by using the JDK 9 API 
> java.lang.StackWalkeer.  None of the other SecurityManager API are used.
> 
> I have run mach5 tier1/tier2 plus --test 
> jck:api/java_security,jck:api/javax_crypto,jck:api/javax_net,jck:api/javax_security,jck:api/org_ietf,jck:api/javax_xml/crypto
>  with all green.

Bradford Wetmore has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains six commits:

 - Merge branch 'master' into JDK-8267485
 - Merge branch 'master' into JDK-8267485
 - Replace missing annotation
 - Merge branch 'master' into JDK-8267485
 - Updated copyright date.
 - 8267485: Remove the dependency on SecurityManager in JceSecurityManager.java

-

Changes: https://git.openjdk.java.net/jdk/pull/4150/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4150&range=01
  Stats: 20 lines in 1 file changed: 10 ins; 2 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4150.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4150/head:pull/4150

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

Integrated: 8248268: Support KWP in addition to KW

[Valerie Peng]

On Thu, 4 Feb 2021 10:51:12 GMT, Valerie Peng  wrote:

> This change updates SunJCE provider as below:
> - updated existing AESWrap support with AES/KW/NoPadding cipher 
> transformation. 
> - added support for AES/KWP/NoPadding and AES/KW/PKCS5Padding.
> 
> Existing AESWrap impl, i.e. AESWrapCipher class, is re-factored and renamed 
> to KeyWrapCipher class. The W and W_inverse functions are moved to KWUtil 
> class. The KW and KWP support are in the new AESKeyWrap and AESKeyWrapPadded 
> classes which extend FeedbackCipher and used in KeyWrapCipher class. To 
> minimize data copying, AESKeyWrap and AESKeyWrapPadded will do the crypto 
> operation over the same input buffer which is allocated and managed by 
> KeyWrapCipher class. 
> 
> Also note that existing AESWrap impl does not take IV. However, the 
> corresponding PKCS#11 mechanisms do, so I added support for accepting IVs to 
> both KW and KWP.
> 
> Thanks,
> Valerie

This pull request has now been integrated.

Changeset: 136badb1
Author:Valerie Peng 
URL:   
https://git.openjdk.java.net/jdk/commit/136badb1f7b0ba1d16fcf0deca5899e0d0186fc0
Stats: 2780 lines in 18 files changed: 2105 ins; 557 del; 118 mod

8248268: Support KWP in addition to KW

Reviewed-by: xuelei

-

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

Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v4]

[Maurizio Cimadamore]

> This patch overhauls the library loading mechanism used by the Foreign Linker 
> API. We realized that, while handy, the *default* lookup abstraction 
> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.
> 
> This patch replaces `LibraryLookup` with a simpler `SymbolLookup` 
> abstraction, a functional interface. Crucially, `SymbolLookup` does not 
> concern with library loading, only symbol lookup. For this reason, two 
> factories are added:
> 
> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to 
> lookup symbols in libraries loaded by current loader
> * `CLinker::systemLookup` - a more stable replacement for the *default* 
> lookup, which looks for symbols in libc.so (or its equivalent in other 
> platforms). The contents of this lookup are unspecified.
> 
> Both factories are *restricted*, so they can only be called when 
> `--enable-native-access` is set.

Maurizio Cimadamore has updated the pull request incrementally with one 
additional commit since the last revision:

  Address review comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4316/files
  - new: https://git.openjdk.java.net/jdk/pull/4316/files/41ab74b6..7be87eae

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

  Stats: 42 lines in 13 files changed: 6 ins; 6 del; 30 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4316.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4316/head:pull/4316

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

Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v3]

[Maurizio Cimadamore]

> This patch overhauls the library loading mechanism used by the Foreign Linker 
> API. We realized that, while handy, the *default* lookup abstraction 
> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.
> 
> This patch replaces `LibraryLookup` with a simpler `SymbolLookup` 
> abstraction, a functional interface. Crucially, `SymbolLookup` does not 
> concern with library loading, only symbol lookup. For this reason, two 
> factories are added:
> 
> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to 
> lookup symbols in libraries loaded by current loader
> * `CLinker::systemLookup` - a more stable replacement for the *default* 
> lookup, which looks for symbols in libc.so (or its equivalent in other 
> platforms). The contents of this lookup are unspecified.
> 
> Both factories are *restricted*, so they can only be called when 
> `--enable-native-access` is set.

Maurizio Cimadamore has updated the pull request incrementally with 10 
additional commits since the last revision:

 - Update test/jdk/java/foreign/TestIntrinsics.java
   
   Co-authored-by: Paul Sandoz 
 - Update test/jdk/java/foreign/valist/VaListTest.java
   
   Co-authored-by: Paul Sandoz 
 - Update test/jdk/java/foreign/TestVarArgs.java
   
   Co-authored-by: Paul Sandoz 
 - Update test/jdk/java/foreign/TestUpcall.java
   
   Co-authored-by: Paul Sandoz 
 - Update test/jdk/java/foreign/TestIllegalLink.java
   
   Co-authored-by: Paul Sandoz 
 - Update test/jdk/java/foreign/TestSymbolLookup.java
   
   Co-authored-by: Paul Sandoz 
 - Update test/jdk/java/foreign/TestDowncall.java
   
   Co-authored-by: Paul Sandoz 
 - Update test/jdk/java/foreign/StdLibTest.java
   
   Co-authored-by: Paul Sandoz 
 - Update test/jdk/java/foreign/SafeFunctionAccessTest.java
   
   Co-authored-by: Paul Sandoz 
 - Update 
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java
   
   Co-authored-by: Paul Sandoz 

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4316/files
  - new: https://git.openjdk.java.net/jdk/pull/4316/files/2b668f7c..41ab74b6

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

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

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

Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v3]

[Maurizio Cimadamore]

On Wed, 2 Jun 2021 18:40:48 GMT, Paul Sandoz  wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with 10 
>> additional commits since the last revision:
>> 
>>  - Update test/jdk/java/foreign/TestIntrinsics.java
>>
>>Co-authored-by: Paul Sandoz 
>>  - Update test/jdk/java/foreign/valist/VaListTest.java
>>
>>Co-authored-by: Paul Sandoz 
>>  - Update test/jdk/java/foreign/TestVarArgs.java
>>
>>Co-authored-by: Paul Sandoz 
>>  - Update test/jdk/java/foreign/TestUpcall.java
>>
>>Co-authored-by: Paul Sandoz 
>>  - Update test/jdk/java/foreign/TestIllegalLink.java
>>
>>Co-authored-by: Paul Sandoz 
>>  - Update test/jdk/java/foreign/TestSymbolLookup.java
>>
>>Co-authored-by: Paul Sandoz 
>>  - Update test/jdk/java/foreign/TestDowncall.java
>>
>>Co-authored-by: Paul Sandoz 
>>  - Update test/jdk/java/foreign/StdLibTest.java
>>
>>Co-authored-by: Paul Sandoz 
>>  - Update test/jdk/java/foreign/SafeFunctionAccessTest.java
>>
>>Co-authored-by: Paul Sandoz 
>>  - Update 
>> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java
>>
>>Co-authored-by: Paul Sandoz 
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java 
> line 138:
> 
>> 136:  * 
>> 137:  * This method is > href="package-summary.html#restricted">restricted.
>> 138:  * Restricted method are unsafe, and, if used incorrectly, their 
>> use might crash
> 
> Suggestion:
> 
>  * Restricted methods are unsafe, and, if used incorrectly, their use 
> might crash

Ouch - this seems like an issue with _all_ restricted methods.

> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java 
> line 160:
> 
>> 158:  * to allocate structs returned by-value.
>> 159:  *
>> 160:  * @see SymbolLookup
> 
> For JDK code it is more common for `@See` to occur after the 
> parameters/return/throws, and before any `@Since`.

I'll fix

-

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

Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v2]

[Paul Sandoz]

On Wed, 2 Jun 2021 20:17:20 GMT, Maurizio Cimadamore  
wrote:

>> This patch overhauls the library loading mechanism used by the Foreign 
>> Linker API. We realized that, while handy, the *default* lookup abstraction 
>> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.
>> 
>> This patch replaces `LibraryLookup` with a simpler `SymbolLookup` 
>> abstraction, a functional interface. Crucially, `SymbolLookup` does not 
>> concern with library loading, only symbol lookup. For this reason, two 
>> factories are added:
>> 
>> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to 
>> lookup symbols in libraries loaded by current loader
>> * `CLinker::systemLookup` - a more stable replacement for the *default* 
>> lookup, which looks for symbols in libc.so (or its equivalent in other 
>> platforms). The contents of this lookup are unspecified.
>> 
>> Both factories are *restricted*, so they can only be called when 
>> `--enable-native-access` is set.
>
> Maurizio Cimadamore has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update 
> test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/PanamaPoint.java
>   
>   Co-authored-by: Jorn Vernee 

Looks good. Just minor comments to accept/reject.

The shim library is an interesting approach. I did wonder if the libvm library 
could be used, but it might have some weird side-effects or bring in more 
symbols than necessary.

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 
138:

> 136:  * 
> 137:  * This method is  href="package-summary.html#restricted">restricted.
> 138:  * Restricted method are unsafe, and, if used incorrectly, their use 
> might crash

Suggestion:

 * Restricted methods are unsafe, and, if used incorrectly, their use might 
crash

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line 
160:

> 158:  * to allocate structs returned by-value.
> 159:  *
> 160:  * @see SymbolLookup

For JDK code it is more common for `@See` to occur after the 
parameters/return/throws, and before any `@Since`.

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/abi/SharedUtils.java
 line 418:

> 416: private static class AllocHolder {
> 417: 
> 418: private static final CLinker linker = getSystemLinker();

Suggestion:

private static final CLinker SYS_LINKER = getSystemLinker();

test/jdk/java/foreign/SafeFunctionAccessTest.java line 53:

> 51: );
> 52: 
> 53: static SymbolLookup lookup = SymbolLookup.loaderLookup();

Suggestion:

static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();

test/jdk/java/foreign/StdLibTest.java line 158:

> 156: static class StdLibHelper {
> 157: 
> 158: final static SymbolLookup LOOKUP;

Suggestion:

static final SymbolLookup LOOKUP;

test/jdk/java/foreign/TestDowncall.java line 60:

> 58: }
> 59: 
> 60: static SymbolLookup lookup = SymbolLookup.loaderLookup();

Suggestion:

static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();

test/jdk/java/foreign/TestIllegalLink.java line 48:

> 46: public class TestIllegalLink {
> 47: 
> 48: private static final MemoryAddress dummyTarget = 
> MemoryAddress.ofLong(1);

Suggestion:

private static final MemoryAddress DUMMY_TARGET = MemoryAddress.ofLong(1);

test/jdk/java/foreign/TestIntrinsics.java line 60:

> 58: }
> 59: 
> 60: static SymbolLookup lookup = SymbolLookup.loaderLookup();

Suggestion:

static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();

test/jdk/java/foreign/TestSymbolLookup.java line 50:

> 48: }
> 49: 
> 50: static SymbolLookup lookup = SymbolLookup.loaderLookup();

Suggestion:

static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();

test/jdk/java/foreign/TestUpcall.java line 69:

> 67: static CLinker abi = CLinker.getInstance();
> 68: 
> 69: static SymbolLookup lookup = SymbolLookup.loaderLookup();

Suggestion:

static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();

test/jdk/java/foreign/TestVarArgs.java line 68:

> 66: }
> 67: 
> 68: static final MemoryAddress varargsAddr =

Suggestion:

static final MemoryAddress VARARGS_ADDR =

test/jdk/java/foreign/valist/VaListTest.java line 74:

> 72: }
> 73: 
> 74: static SymbolLookup lookup = SymbolLookup.loaderLookup();

Suggestion:

static final SymbolLookup LOOKUP = SymbolLookup.loaderLookup();

-

Marked as reviewed by psandoz (Reviewer).

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

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

[Anthony Scarpino]

On Mon, 24 May 2021 20:33:33 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/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?

I just decided it was better to have the declaration that is used more than 
once at the top.

-

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

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

[Anthony Scarpino]

On Fri, 21 May 2021 01:19:44 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix perf problem by reorganizing doLastBlock()
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1610:
> 
>> 1608: // update the input parameters for what was taken 
>> out of 'in'
>> 1609: inOfs += inUsed;
>> 1610: inLen -= inUsed;
> 
> This merge block code won't be needed if inLen == 0, i.e. can just assign in 
> to be buffer, inOfs to 0, and inLen to bufRemaining.

You are correct, but it's not that simple to handle this case without adding 
more if()'s which I've found can slow down overall performance.  I'm hesitant 
change this code for this case

-

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

Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries

[Maurizio Cimadamore]

On Wed, 2 Jun 2021 17:19:06 GMT, Maurizio Cimadamore  
wrote:

> This patch overhauls the library loading mechanism used by the Foreign Linker 
> API. We realized that, while handy, the *default* lookup abstraction 
> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.
> 
> This patch replaces `LibraryLookup` with a simpler `SymbolLookup` 
> abstraction, a functional interface. Crucially, `SymbolLookup` does not 
> concern with library loading, only symbol lookup. For this reason, two 
> factories are added:
> 
> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to 
> lookup symbols in libraries loaded by current loader
> * `CLinker::systemLookup` - a more stable replacement for the *default* 
> lookup, which looks for symbols in libc.so (or its equivalent in other 
> platforms). The contents of this lookup are unspecified.
> 
> Both factories are *restricted*, so they can only be called when 
> `--enable-native-access` is set.

> _Mailing list message from [Chapman Flack](mailto:c...@anastigmatix.net) on 
> [security-dev](mailto:security-...@mail.openjdk.java.net):_
> 
> On 06/02/21 13:30, Maurizio Cimadamore wrote:
> 
> > This patch replaces `LibraryLookup` with a simpler `SymbolLookup`
> > abstraction, a functional interface. Crucially, `SymbolLookup` does not
> > concern with library loading, only symbol lookup. For this reason, two
> > factories are added:
> 
> Hi,
> 
> While I am thinking about this, what will be the behavior when the JVM
> itself has been dynamically loaded into an embedding application, and
> launched with the JNI invocation API?
> 
> Will there be a *Lookup flavor that is able to find any exported symbol
> known in the embedding environment the JVM was loaded into? (This is the
> sort of condition the Mac OS linker checks when given the -bundle_loader
> option.)
> 
> Regards,
> Chapman Flack (maintainer of a project that happens to work that way)

Hi,
at the moment we don't have plans to add such a lookup, but I believe it should 
be possible to build such a lookup using `dlopen` and the linker API. I have 
provided an example elsewhere of how easy it easy to build a wrapper lookup 
around dlopen/dlsym:

https://gist.github.com/mcimadamore/0883ea6f4836ae0c1d2a31c48197da1a

Perhaps something like that could be useful in the use case you mention?

-

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

Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v2]

[Maurizio Cimadamore]

> This patch overhauls the library loading mechanism used by the Foreign Linker 
> API. We realized that, while handy, the *default* lookup abstraction 
> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.
> 
> This patch replaces `LibraryLookup` with a simpler `SymbolLookup` 
> abstraction, a functional interface. Crucially, `SymbolLookup` does not 
> concern with library loading, only symbol lookup. For this reason, two 
> factories are added:
> 
> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to 
> lookup symbols in libraries loaded by current loader
> * `CLinker::systemLookup` - a more stable replacement for the *default* 
> lookup, which looks for symbols in libc.so (or its equivalent in other 
> platforms). The contents of this lookup are unspecified.
> 
> Both factories are *restricted*, so they can only be called when 
> `--enable-native-access` is set.

Maurizio Cimadamore has updated the pull request incrementally with one 
additional commit since the last revision:

  Update 
test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/PanamaPoint.java
  
  Co-authored-by: Jorn Vernee 

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4316/files
  - new: https://git.openjdk.java.net/jdk/pull/4316/files/01d9c198..2b668f7c

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

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

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

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

[Valerie Peng]

On Wed, 2 Jun 2021 03:26:03 GMT, Anthony Scarpino  wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 975:
>> 
>>> 973: doUpdate(in, inOff, inLen, output, 0);
>>> 974: } catch (ShortBufferException e) {
>>> 975: // update decryption has no output
>> 
>> The comment does not seems to make sense? This is GCMEncrypt class. The 
>> right reason should be that the output array is allocated by the provider 
>> and should have the right size. However, it seems safer to throw 
>> AssertionException() instead of swallowing the SBE...
>
> Yeah the comment isn't right.  I think it's excessive to throw an exception 
> that should never happen, but I'll add a ProviderException.

Sure.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 1030:
>> 
>>> 1028: inOfs += inLenUsed;
>>> 1029: inLen -= inLenUsed;
>>> 1030: outOfs += blockSize;
>> 
>> 'blockSize' should be 'len'?
>
> Either is fine because len and blockSize will be the same.

Hmm, re-reading the code, I see why you chose to use blockSize since line 1055 
uses "len += ..." so whether len==blockSize depends on the code in line 
1022-1054.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 1055:
>> 
>>> 1053: // remainder offset is based on original buffer length
>>> 1054: ibuffer.write(in, inOfs + inLen, remainder);
>>> 1055: }
>> 
>> I wonder if these update(byte[], int, int, byte[], int) calls (such as the 
>> one on line 1045) should take ibuffer and stores the unprocessed bytes into 
>> it. This way seems more robust and you won't need separate logic. Same 
>> comment for the doUpdate() taking ByteBuffer arguments.
>
> Do you mean having all the GCM interface implementations have an argument 
> that takes ibuffer and adds any unprocessed data into?  Maybe it would save a 
> copy of the code, but I'm not sure I like GCTR or GHASH adding data into the 
> ibuffer.

Maybe there are other alternatives than making GCTR/GHASH to write into 
ibuffer. Or, perhaps we can figure out the right input length when passing them 
to GCTR or GHASH? This is again more for readability and robustness. What you 
have works, just need to be very careful and require detailed 
tracking/knowledge on GCTR/GHASH level since all inputs are passed down, but 
not all are used.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 1163:
>> 
>>> 1161: r = doUpdate(buffer, 0, bufLen, out, outOfs);
>>> 1162: bufLen -= r;
>>> 1163: inOfs += r;
>> 
>> Would bufLen >= blockSize? Regardless, 'inOfs' should not be touched as 'in' 
>> is not used in the above doUpdate() call.
>
> removing it

Ok.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 1174:
>> 
>>> 1172: inLen -= r;
>>> 1173: r = gctrghash.update(block, 0, blockSize, out,
>>> 1174: outOfs + resultLen);
>> 
>> I don't follow why you don't update the 'outOfs' after the line 1161 
>> doUpdate() call and then add the resultLen when calling gctrhash.update(...) 
>> here. Seems fragile and difficult to maintain?
>
> i cleaned it up.. all the += or -+ are annoying, but not there is much i can 
> do

Ok, will wait for the commit.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 1213:
>> 
>>> 1211: 
>>> 1212: // copy the tag to the end of the buffer
>>> 1213: System.arraycopy(block, 0, out, resultLen + outOfs, 
>>> tagLenBytes);
>> 
>> Now that the tag is copied to the output, why not increment resultLen w/ 
>> tagLenBytes? This way, you don't have to keep repeating the (resultLen + 
>> tagLenBytes) for another two times?
>
> yeah, that got changed after this comment

Ok.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 1317:
>> 
>>> 1315:  * If tagOfs = 0, 'in' contains only the tag
>>> 1316:  * if tagOfs = blockSize, there is no data in 'in' and all 
>>> the tag
>>> 1317:  *   is in ibuffer
>> 
>> Is this correct? mergeBlock() returns the number of used bytes from 'in', if 
>> no data is in 'in' and all the tag is from 'ibuffer', tagOfs should equal to 
>> -tagLenBytes. The next line should be moved up as the tag position gradually 
>> shifts from 'in' toward 'ibuffer'. The tagOfs for the next line should be 
>> -tagLenBytes < tagOfs < 0?
>
> Yeah, I reworkded it

Ok, will wait for the commit.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 1401:
>> 
>>> 1399: ShortBufferException {
>>> 1400: GHASH save = null;
>>> 1401: 
>> 
>> Should do ArrayUtil.nullAndBoundsCheck() on 'in'?
>
> that was done in engineDoF

Re: RFR: 8262409: sun/security/ssl/SSLSocketImpl/SSLSocketImplThrowsWrongExceptions. SSL test failures caused by java failed with "Server reported the wrong exception"

[Clive Verghese]

On Wed, 2 Jun 2021 14:03:57 GMT, Fernando Guallini  
wrote:

> The test `SSLSocketImplThrowsWrongExceptions` is failing intermittently after 
> the change: [JDK-8259662: Don't wrap SocketExceptions into SSLExceptions in 
> SSLSocketImpl](https://bugs.openjdk.java.net/browse/JDK-8259662)
> 
> Since SocketExceptions are not wrapped into SSLException, also need to be 
> caught. Other tests that were expecting a SSLException to be thrown under 
> certain condition were updated with a similar fix in the change previously 
> mentioned.
> 
> In addition, thread.sleep was replaced with a CountDownLatch for 
> client/server synchronization

Thank you for verifying.

-

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

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

[Anthony Scarpino]

On Fri, 21 May 2021 00:03:40 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix perf problem by reorganizing doLastBlock()
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1437:
> 
>> 1435: } catch (ArrayIndexOutOfBoundsException aiobe) {
>> 1436: throw new ShortBufferException("Output buffer 
>> invalid");
>> 1437: }
> 
> I think this should be moved to the very beginning before all the processing 
> and if the output capacity is less than 'len-tagLenBytes' value, then no need 
> to proceed? IIRC, the save/restore is more for algorithms which use padding, 
> may not be needed for GCM?

I had this down here because it's not needed until gctr ops are done and ghash 
doesn't use an output, but I can move it up.
I remember Sean C having to do save/restore work for GCM.. The tag can create 
the similar padding issues.  It felt safe to keep it.

-

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

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

[Valerie Peng]

On Wed, 2 Jun 2021 01:53:51 GMT, Anthony Scarpino  wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 741:
>> 
>>> 739: } else {
>>> 740: // If the remaining in buffer + data does 
>>> not fill a
>>> 741: // block, complete the gctr operation
>> 
>> This comment seems more suited for the else block below at line 753. In 
>> addition, the criteria for completing the gctr operation should be whether 
>> src still has remaining bytes left. It's possible that the (src.remaining() 
>> == blockSize - over) and happen to fill up the block, right? The current 
>> flow for this particular case would be an op.update(...) then continue down 
>> to line 770-778, maybe you can adjust the if-check on line748 so this would 
>> go through line 754-759 and return, i.e. the else block?
>
> I changed the comment, but I also changed the code which will be in the next 
> update

Ok, will wait for the update then.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 752:
>> 
>>> 750: if (dst != null) {
>>> 751: dst.put(block, 0, blockSize);
>>> 752: }
>> 
>> not counting this blockSize value into "processed"?
>
> code is now changed

Ok, also in the next update?

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 805:
>> 
>>> 803: /**
>>> 804:  * This segments large data into smaller chunks so hotspot 
>>> will start
>>> 805:  * using CTR and GHASH intrinsics sooner.  This is a problem 
>>> for app
>> 
>> nit: typo: CTR->GCTR? This is to improve performance rather than a problem, 
>> no? Same comment applies to the other throttleData() method above.
>
> Yes, this is for apps or perf tests that send large input data and cause the 
> hotspot compiler to be slower to start using the intrinsics.. This is just a 
> new version of what was in the old code in doLastBlock() around line 400.

Sure.

-

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

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

[Anthony Scarpino]

On Fri, 21 May 2021 00:05:17 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix perf problem by reorganizing doLastBlock()
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1442:
> 
>> 1440: throw new ShortBufferException("Output buffer too 
>> small, must" +
>> 1441: "be at least " + (len - tagLenBytes) + " bytes 
>> long");
>> 1442: }
> 
> This looks like a half-baked save/restore functionality?

This one forgot to restore the saved object

> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1475:
> 
>> 1473: 
>> 1474: // Length of the input
>> 1475: ByteBuffer tag;
> 
> Is the comment obsolete? I don't quite follow how it's related to 'tag'.

sounds obsolete

-

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

Re: RFR: 8262409: sun/security/ssl/SSLSocketImpl/SSLSocketImplThrowsWrongExceptions. SSL test failures caused by java failed with "Server reported the wrong exception"

[Fernando Guallini]

On Wed, 2 Jun 2021 17:29:51 GMT, Clive Verghese  wrote:

> Thank you @fguallini for fixing this test.
> 
> Should the SocketException be expected by the client logic as well?
> https://github.com/openjdk/jdk/blob/3631a39a4797919acbc7862cb94fa7f7be7f4f8a/test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketImplThrowsWrongExceptions.java#L135

Thanks, I did look at this but the SocketException was not thrown in the client 
side in any of the thousands of times I executed the test with the current PR 
fix, it does not seem to be necessary to expect the exception there

-

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

Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries

[Jorn Vernee]

On Wed, 2 Jun 2021 17:19:06 GMT, Maurizio Cimadamore  
wrote:

> This patch overhauls the library loading mechanism used by the Foreign Linker 
> API. We realized that, while handy, the *default* lookup abstraction 
> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.
> 
> This patch replaces `LibraryLookup` with a simpler `SymbolLookup` 
> abstraction, a functional interface. Crucially, `SymbolLookup` does not 
> concern with library loading, only symbol lookup. For this reason, two 
> factories are added:
> 
> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to 
> lookup symbols in libraries loaded by current loader
> * `CLinker::systemLookup` - a more stable replacement for the *default* 
> lookup, which looks for symbols in libc.so (or its equivalent in other 
> platforms). The contents of this lookup are unspecified.
> 
> Both factories are *restricted*, so they can only be called when 
> `--enable-native-access` is set.

LGTM, minor nit inline

test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/PanamaPoint.java
 line 62:

> 60: );
> 61: MH_distance_ptrs = abi.downcallHandle(
> 62: lookup.lookup("distance_ptrs").get(),

Spurious white space
Suggestion:

lookup.lookup("distance_ptrs").get(),

-

Marked as reviewed by jvernee (Reviewer).

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

Re: RFR: 8267485: Remove the dependency on SecurityManager in JceSecurityManager.java

[Daniel Fuchs]

On Sat, 22 May 2021 00:20:11 GMT, Bradford Wetmore  wrote:

> The JceSecurityManager is currently a subclass of 
> java.security.SecurityManager.  Now that JEP 411 has been integrated, this 
> class should be updated to no longer subclass SecurityManager.
> 
> The only reason for using SecurityManager to easily get the Class Context 
> (call stack), but we can achieve the same effect by using the JDK 9 API 
> java.lang.StackWalkeer.  None of the other SecurityManager API are used.
> 
> I have run mach5 tier1/tier2 plus --test 
> jck:api/java_security,jck:api/javax_crypto,jck:api/javax_net,jck:api/javax_security,jck:api/org_ietf,jck:api/javax_xml/crypto
>  with all green.

src/java.base/share/classes/javax/crypto/JceSecurityManager.java line 107:

> 105: List stack = StackWalker.getInstance(
> 106: StackWalker.Option.RETAIN_CLASS_REFERENCE)
> 107: .walk((s) -> s.collect(Collectors.toList()));

StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE) will require 
a permission check.
As long as the SecurityManager is still functional, doesn't this mean that 
creating the StackWalker should be performed in a doPrivileged? If so maybe it 
should be done in a (possibly static) initializer. Or is it intentional to 
check that the caller (and the whole calling stack) posses the 
`RuntimePermission("getStackWalkerWithClassReference")`?

-

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

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

[Anthony Scarpino]

On Thu, 20 May 2021 22:59:18 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix perf problem by reorganizing doLastBlock()
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1412:
> 
>> 1410: }
>> 1411: 
>> 1412: checkDataLength(len - tagLenBytes);
> 
> The impl of checkDataLength(...) is based on the "processed" variable which 
> is always 0 for doUpdate() calls. This suits encryption better, but does not 
> suit decryption? It seems that the doUpdate() calls would just keep buffering 
> the data into 'ibuffer' without checking the size. It seems to me that this 
> checkDataLength() call on line 1412 would always pass.

checkDataLength is subtracting the length's from the max.  The check at 1422 
would fail because the max would be negative and the processed would be 0.  I 
don't see it always passing

-

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

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

[Valerie Peng]

On Wed, 2 Jun 2021 17:52:07 GMT, Valerie Peng  wrote:

>> This change updates SunJCE provider as below:
>> - updated existing AESWrap support with AES/KW/NoPadding cipher 
>> transformation. 
>> - added support for AES/KWP/NoPadding and AES/KW/PKCS5Padding.
>> 
>> Existing AESWrap impl, i.e. AESWrapCipher class, is re-factored and renamed 
>> to KeyWrapCipher class. The W and W_inverse functions are moved to KWUtil 
>> class. The KW and KWP support are in the new AESKeyWrap and AESKeyWrapPadded 
>> classes which extend FeedbackCipher and used in KeyWrapCipher class. To 
>> minimize data copying, AESKeyWrap and AESKeyWrapPadded will do the crypto 
>> operation over the same input buffer which is allocated and managed by 
>> KeyWrapCipher class. 
>> 
>> Also note that existing AESWrap impl does not take IV. However, the 
>> corresponding PKCS#11 mechanisms do, so I added support for accepting IVs to 
>> both KW and KWP.
>> 
>> Thanks,
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add AESWrapPad alias for AES with KWP mode for better interoperability and 
> updated tests accordingly.

Hi Mike,
Lastly, regarding the naming, I am not too inclined toward the 
AES/KW/KWPPadding suggestion since the general concept of padding is that it's 
general, not mode-specific and are applied before data is processed by crypto 
operations. The mode can just process data without knowing whether padding is 
applied as long as the data has the right length. Thus, I'd keep KWP as a mode 
which does internal padding instead of a generic padding scheme. The 
AutoPadding suggestion is interesting and can be easily built on top of KW and 
KWP modes if desired. 

Thanks for the feedbacks,
Valerie

-

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

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

[Anthony Scarpino]

On Thu, 20 May 2021 23:27:51 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix perf problem by reorganizing doLastBlock()
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1317:
> 
>> 1315:  * If tagOfs = 0, 'in' contains only the tag
>> 1316:  * if tagOfs = blockSize, there is no data in 'in' and all the 
>> tag
>> 1317:  *   is in ibuffer
> 
> Is this correct? mergeBlock() returns the number of used bytes from 'in', if 
> no data is in 'in' and all the tag is from 'ibuffer', tagOfs should equal to 
> -tagLenBytes. The next line should be moved up as the tag position gradually 
> shifts from 'in' toward 'ibuffer'. The tagOfs for the next line should be 
> -tagLenBytes < tagOfs < 0?

Yeah, I reworkded it

> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1401:
> 
>> 1399: ShortBufferException {
>> 1400: GHASH save = null;
>> 1401: 
> 
> Should do ArrayUtil.nullAndBoundsCheck() on 'in'?

that was done in engineDoFinal

-

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

Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries

[Chapman Flack]

On 06/02/21 13:30, Maurizio Cimadamore wrote:
> This patch replaces `LibraryLookup` with a simpler `SymbolLookup`
> abstraction, a functional interface. Crucially, `SymbolLookup` does not
> concern with library loading, only symbol lookup. For this reason, two
> factories are added:

Hi,

While I am thinking about this, what will be the behavior when the JVM
itself has been dynamically loaded into an embedding application, and
launched with the JNI invocation API?

Will there be a *Lookup flavor that is able to find any exported symbol
known in the embedding environment the JVM was loaded into? (This is the
sort of condition the Mac OS linker checks when given the -bundle_loader
option.)

Regards,
Chapman Flack (maintainer of a project that happens to work that way)

RFR: 8267485: Remove the dependency on SecurityManager in JceSecurityManager.java

[Bradford Wetmore]

The JceSecurityManager is currently a subclass of 
java.security.SecurityManager.  Now that JEP 411 has been integrated, this 
class should be updated to no longer subclass SecurityManager.

The only reason for using SecurityManager to easily get the Class Context (call 
stack), but we can achieve the same effect by using the JDK 9 API 
java.lang.StackWalkeer.  None of the other SecurityManager API are used.

I have run mach5 tier1/tier2 plus --test 
jck:api/java_security,jck:api/javax_crypto,jck:api/javax_net,jck:api/javax_security,jck:api/org_ietf,jck:api/javax_xml/crypto
 with all green.

-

Commit messages:
 - Merge branch 'master' into JDK-8267485
 - Updated copyright date.
 - 8267485: Remove the dependency on SecurityManager in JceSecurityManager.java

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

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

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

[Anthony Scarpino]

On Thu, 20 May 2021 20:00:07 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix perf problem by reorganizing doLastBlock()
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1174:
> 
>> 1172: inLen -= r;
>> 1173: r = gctrghash.update(block, 0, blockSize, out,
>> 1174: outOfs + resultLen);
> 
> I don't follow why you don't update the 'outOfs' after the line 1161 
> doUpdate() call and then add the resultLen when calling gctrhash.update(...) 
> here. Seems fragile and difficult to maintain?

i cleaned it up.. all the += or -+ are annoying, but not there is much i can do

> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1213:
> 
>> 1211: 
>> 1212: // copy the tag to the end of the buffer
>> 1213: System.arraycopy(block, 0, out, resultLen + outOfs, 
>> tagLenBytes);
> 
> Now that the tag is copied to the output, why not increment resultLen w/ 
> tagLenBytes? This way, you don't have to keep repeating the (resultLen + 
> tagLenBytes) for another two times?

yeah, that got changed after this comment

-

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

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

[Valerie Peng]

> This change updates SunJCE provider as below:
> - updated existing AESWrap support with AES/KW/NoPadding cipher 
> transformation. 
> - added support for AES/KWP/NoPadding and AES/KW/PKCS5Padding.
> 
> Existing AESWrap impl, i.e. AESWrapCipher class, is re-factored and renamed 
> to KeyWrapCipher class. The W and W_inverse functions are moved to KWUtil 
> class. The KW and KWP support are in the new AESKeyWrap and AESKeyWrapPadded 
> classes which extend FeedbackCipher and used in KeyWrapCipher class. To 
> minimize data copying, AESKeyWrap and AESKeyWrapPadded will do the crypto 
> operation over the same input buffer which is allocated and managed by 
> KeyWrapCipher class. 
> 
> Also note that existing AESWrap impl does not take IV. However, the 
> corresponding PKCS#11 mechanisms do, so I added support for accepting IVs to 
> both KW and KWP.
> 
> Thanks,
> Valerie

Valerie Peng has updated the pull request incrementally with one additional 
commit since the last revision:

  Add AESWrapPad alias for AES with KWP mode for better interoperability and 
updated tests accordingly.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2404/files
  - new: https://git.openjdk.java.net/jdk/pull/2404/files/cfe66d54..e24282c0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2404&range=09
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2404&range=08-09

  Stats: 138 lines in 6 files changed: 130 ins; 1 del; 7 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2404.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2404/head:pull/2404

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

Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries

[Maurizio Cimadamore]

On Wed, 2 Jun 2021 17:19:06 GMT, Maurizio Cimadamore  
wrote:

> This patch overhauls the library loading mechanism used by the Foreign Linker 
> API. We realized that, while handy, the *default* lookup abstraction 
> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.
> 
> This patch replaces `LibraryLookup` with a simpler `SymbolLookup` 
> abstraction, a functional interface. Crucially, `SymbolLookup` does not 
> concern with library loading, only symbol lookup. For this reason, two 
> factories are added:
> 
> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to 
> lookup symbols in libraries loaded by current loader
> * `CLinker::systemLookup` - a more stable replacement for the *default* 
> lookup, which looks for symbols in libc.so (or its equivalent in other 
> platforms). The contents of this lookup are unspecified.
> 
> Both factories are *restricted*, so they can only be called when 
> `--enable-native-access` is set.

Some notes on how the *system* lookup is implemented (see class SystemLookup):

* On Linux and MacOS, we generate, at build time, an empty shim library. Since 
this library depends on the C standard libraries, we can, at runtime, load this 
shim library, and use `dlsym` to lookup symbols in it (since `dlsym` will also 
look at the dependencies).

* Since Windows does not allow for library symbols in dependent libraries to be 
re-exported, Windows uses a different approach - which loads either 
`ucrtbase.dll` or `msvcrt.dll` (the former is preferred, if available), and 
returns a lookup object based on that.

In both cases, the required libraries are loaded in a classloader-independent 
fashion, by taking advantage of the support available in NativeLibraries.

Class loader lookups are built on top of ClassLoader::findNative (which is also 
the method used by JNI to find JNI methods).

This patch removes all the native code which was required to support the 
default lookup abstraction. The implementation changes are relatively 
straightforward - but several test and microbenchmark updates were required.

-

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

Re: RFR: 8262409: sun/security/ssl/SSLSocketImpl/SSLSocketImplThrowsWrongExceptions. SSL test failures caused by java failed with "Server reported the wrong exception"

[Clive Verghese]

On Wed, 2 Jun 2021 14:03:57 GMT, Fernando Guallini  
wrote:

> The test `SSLSocketImplThrowsWrongExceptions` is failing intermittently after 
> the change: [JDK-8259662: Don't wrap SocketExceptions into SSLExceptions in 
> SSLSocketImpl](https://bugs.openjdk.java.net/browse/JDK-8259662)
> 
> Since SocketExceptions are not wrapped into SSLException, also need to be 
> caught. Other tests that were expecting a SSLException to be thrown under 
> certain condition were updated with a similar fix in the change previously 
> mentioned.
> 
> In addition, thread.sleep was replaced with a CountDownLatch for 
> client/server synchronization

Thank you @fguallini for fixing this test. 

Should the SocketException be expected by the client logic as well?
https://github.com/openjdk/jdk/blob/3631a39a4797919acbc7862cb94fa7f7be7f4f8a/test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketImplThrowsWrongExceptions.java#L135

-

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

RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries

[Maurizio Cimadamore]

This patch overhauls the library loading mechanism used by the Foreign Linker 
API. We realized that, while handy, the *default* lookup abstraction 
(`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.

This patch replaces `LibraryLookup` with a simpler `SymbolLookup` abstraction, 
a functional interface. Crucially, `SymbolLookup` does not concern with library 
loading, only symbol lookup. For this reason, two factories are added:

* `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to 
lookup symbols in libraries loaded by current loader
* `CLinker::systemLookup` - a more stable replacement for the *default* lookup, 
which looks for symbols in libc.so (or its equivalent in other platforms). The 
contents of this lookup are unspecified.

Both factories are *restricted*, so they can only be called when 
`--enable-native-access` is set.

-

Commit messages:
 - Improve javadoc
 - Initial push

Changes: https://git.openjdk.java.net/jdk/pull/4316/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4316&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8268129
  Stats: 1278 lines in 44 files changed: 569 ins; 617 del; 92 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4316.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4316/head:pull/4316

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

Re: RFR: 8240256: Better resource cleaning for SunPKCS11 Provider [v3]

[Valerie Peng]

On Fri, 28 May 2021 14:31:25 GMT, Sean Coffey  wrote:

>> Added capability to allow the PKCS11 Token to be destroyed once a session is 
>> logged out from. New configuration properties via pkcs11 config file. 
>> Cleaned up the native resource poller also.
>> 
>> New unit test case to test behaviour. Some PKCS11 tests refactored to allow 
>> pkcs11 provider to be configured (and tested) with a config file of choice.
>> 
>> Reviewer request @valeriepeng
>
> Sean Coffey has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains five commits:
> 
>  - whitespace
>  - Further 8240256 test clean up
>  - Merge branch 'master' into JDK-8240256-pkcs11
>  - Initial corrections from RFR
>  - 8240256: Better resource cleaning for SunPKCS11 Provider

Changes look fine. Thanks, Valerie

-

Marked as reviewed by valeriep (Reviewer).

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

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

[Anthony Scarpino]

On Thu, 20 May 2021 19:46:51 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix perf problem by reorganizing doLastBlock()
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1163:
> 
>> 1161: r = doUpdate(buffer, 0, bufLen, out, outOfs);
>> 1162: bufLen -= r;
>> 1163: inOfs += r;
> 
> Would bufLen >= blockSize? Regardless, 'inOfs' should not be touched as 'in' 
> is not used in the above doUpdate() call.

removing it

-

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

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

[Anthony Scarpino]

On Thu, 20 May 2021 19:06:31 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix perf problem by reorganizing doLastBlock()
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1055:
> 
>> 1053: // remainder offset is based on original buffer length
>> 1054: ibuffer.write(in, inOfs + inLen, remainder);
>> 1055: }
> 
> I wonder if these update(byte[], int, int, byte[], int) calls (such as the 
> one on line 1045) should take ibuffer and stores the unprocessed bytes into 
> it. This way seems more robust and you won't need separate logic. Same 
> comment for the doUpdate() taking ByteBuffer arguments.

Do you mean having all the GCM interface implementations have an argument that 
takes ibuffer and adds any unprocessed data into?  Maybe it would save a copy 
of the code, but I'm not sure I like GCTR or GHASH adding data into the ibuffer.

-

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

Re: RFR: 8262409: sun/security/ssl/SSLSocketImpl/SSLSocketImplThrowsWrongExceptions. SSL test failures caused by java failed with "Server reported the wrong exception"

[Xue-Lei Andrew Fan]

On Wed, 2 Jun 2021 14:03:57 GMT, Fernando Guallini  
wrote:

> The test `SSLSocketImplThrowsWrongExceptions` is failing intermittently after 
> the change: [JDK-8259662: Don't wrap SocketExceptions into SSLExceptions in 
> SSLSocketImpl](https://bugs.openjdk.java.net/browse/JDK-8259662)
> 
> Since SocketExceptions are not wrapped into SSLException, also need to be 
> caught. Other tests that were expecting a SSLException to be thrown under 
> certain condition were updated with a similar fix in the change previously 
> mentioned.
> 
> In addition, thread.sleep was replaced with a CountDownLatch for 
> client/server synchronization

Marked as reviewed by xuelei (Reviewer).

-

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

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

[Anthony Scarpino]

On Thu, 20 May 2021 18:38:35 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix perf problem by reorganizing doLastBlock()
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1030:
> 
>> 1028: inOfs += inLenUsed;
>> 1029: inLen -= inLenUsed;
>> 1030: outOfs += blockSize;
> 
> 'blockSize' should be 'len'?

Either is fine because len and blockSize will be the same.

> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 1033:
> 
>> 1031: ibuffer.reset();
>> 1032: // Code below will write the remainder from 'in' 
>> to ibuffer
>> 1033: } else if (remainder > 0) {
> 
> If bLen == 0, there is no need to put the rest of 'buffer' into 'ibuffer'.
> It looks strange that the remaining buffer data is stored back into 
> 'ibuffer', then the code proceeds to encrypt 'in' from line 1043-1046. This 
> looks incorrect as all prior buffered input should be processed before 
> process current input.

code has changed. not applicable anymore

-

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

Integrated: 8267543: Post JEP 411 refactoring: security

[Weijun Wang]

On Mon, 24 May 2021 20:23:27 GMT, Weijun Wang  wrote:

> 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.
> 
> I'll add a copyright year update commit before integration.

This pull request has now been integrated.

Changeset: 40d23a0c
Author:Weijun Wang 
URL:   
https://git.openjdk.java.net/jdk/commit/40d23a0c0b955ae4636800be183da7a71665f79f
Stats: 116 lines in 19 files changed: 37 ins; 36 del; 43 mod

8267543: Post JEP 411 refactoring: security

Reviewed-by: mullan

-

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

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

[Weijun Wang]

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

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

This pull request has now been integrated.

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

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

Reviewed-by: dfuchs, prr

-

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

Re: RFR: 8267543: Post JEP 411 refactoring: security [v3]

[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.
> 
> I'll add a copyright year update commit before integration.

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

  8267543: Post JEP 411 refactoring: security

-

Changes: https://git.openjdk.java.net/jdk/pull/4172/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4172&range=02
  Stats: 116 lines in 19 files changed: 37 ins; 36 del; 43 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: 8267521: Post JEP 411 refactoring: maximum covering > 50K [v4]

[Weijun Wang]

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

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

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

-

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

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

Re: RFR: 8262409: sun/security/ssl/SSLSocketImpl/SSLSocketImplThrowsWrongExceptions. SSL test failures caused by java failed with "Server reported the wrong exception"

[Rajan Halade]

On Wed, 2 Jun 2021 14:03:57 GMT, Fernando Guallini  
wrote:

> The test `SSLSocketImplThrowsWrongExceptions` is failing intermittently after 
> the change: [JDK-8259662: Don't wrap SocketExceptions into SSLExceptions in 
> SSLSocketImpl](https://bugs.openjdk.java.net/browse/JDK-8259662)
> 
> Since SocketExceptions are not wrapped into SSLException, also need to be 
> caught. Other tests that were expecting a SSLException to be thrown under 
> certain condition were updated with a similar fix in the change previously 
> mentioned.
> 
> In addition, thread.sleep was replaced with a CountDownLatch for 
> client/server synchronization

Marked as reviewed by rhalade (Reviewer).

-

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

RFR: 8262409: sun/security/ssl/SSLSocketImpl/SSLSocketImplThrowsWrongExceptions. SSL test failures caused by java failed with "Server reported the wrong exception"

[Fernando Guallini]

The test `SSLSocketImplThrowsWrongExceptions` is failing intermittently after 
the change: [JDK-8259662: Don't wrap SocketExceptions into SSLExceptions in 
SSLSocketImpl](https://bugs.openjdk.java.net/browse/JDK-8259662)

Since SocketExceptions are not wrapped into SSLException, also need to be 
caught. Other tests that were expecting a SSLException to be thrown under 
certain condition were updated with a similar fix in the change previously 
mentioned.

In addition, thread.sleep was replaced with a CountDownLatch for client/server 
synchronization

-

Commit messages:
 - replace thread.sleep with countdownlatch
 - Merge branch 'master' into 8262409
 - SocketExceptions is not wrapped into SSLExceptions

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

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

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

[openjdk-notifier [bot]]

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 if not void. The local variable will be called `tmp` 
>> if later reassigned or `dummy` if it will be discarded.
>> 3. Put declaration and assignment into a single statement if possible.
>> 4. Rewrite code to achieve #3 above.
>> 
>> I'll add a copyright year update commit before integration.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update FtpClient.java

The dependent pull request has now been integrated, and the target branch of 
this pull request has been updated. This means that changes from the dependent 
pull request can start to show up as belonging to this pull request, which may 
be confusing for reviewers. To remedy this situation, simply merge the latest 
changes from the new target branch into this pull request by running commands 
similar to these in the local repository for your personal fork:


git checkout 8266459
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

-

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

Re: RFR: 8267543: Post JEP 411 refactoring: security [v2]

[openjdk-notifier [bot]]

On Tue, 25 May 2021 16:20:35 GMT, Weijun Wang  wrote:

>> 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.
>> 
>> I'll add a copyright year update commit before integration.
>
> Weijun Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   one more change

The dependent pull request has now been integrated, and the target branch of 
this pull request has been updated. This means that changes from the dependent 
pull request can start to show up as belonging to this pull request, which may 
be confusing for reviewers. To remedy this situation, simply merge the latest 
changes from the new target branch into this pull request by running commands 
similar to these in the local repository for your personal fork:


git checkout 8266459
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

-

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

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

[Weijun Wang]

On Mon, 17 May 2021 18:23:41 GMT, Weijun Wang  wrote:

> Please review this implementation of [JEP 
> 411](https://openjdk.java.net/jeps/411).
> 
> The code change is divided into 3 commits. Please review them one by one.
> 
> 1. 
> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>  The essential change for this JEP, including the `@Deprecate` annotations 
> and spec change. It also update the default value of the 
> `java.security.manager` system property to "disallow", and necessary test 
> change following this update.
> 2. 
> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>  Manual changes to several files so that the next commit can be generated 
> programatically.
> 3. 
> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>  Automatic changes to other source files to avoid javac warnings on 
> deprecation for removal
> 
> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
> generated programmatically, see the comment below for more details. If you 
> are only interested in a portion of the 3rd commit and would like to review 
> it as a separate file, please comment here and I'll generate an individual 
> webrev.
> 
> Due to the size of this PR, no attempt is made to update copyright years for 
> any file to minimize unnecessary merge conflict.
> 
> Furthermore, since the default value of `java.security.manager` system 
> property is now "disallow", most of the tests calling 
> `System.setSecurityManager()` need to launched with 
> `-Djava.security.manager=allow`. This is covered in a different PR at 
> https://github.com/openjdk/jdk/pull/4071.
> 
> Update: the deprecation annotations and javadoc tags, build, compiler, 
> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
> reviewed. Rest are 2d, awt, beans, sound, and swing.

This pull request has now been integrated.

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

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

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

-

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

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

[Weijun Wang]

> Please review this implementation of [JEP 
> 411](https://openjdk.java.net/jeps/411).
> 
> The code change is divided into 3 commits. Please review them one by one.
> 
> 1. 
> https://github.com/openjdk/jdk/commit/576161d15423f58281e384174d28c9f9be7941a1
>  The essential change for this JEP, including the `@Deprecate` annotations 
> and spec change. It also update the default value of the 
> `java.security.manager` system property to "disallow", and necessary test 
> change following this update.
> 2. 
> https://github.com/openjdk/jdk/commit/26a54a835e9f84aa528740a7c5c35d07355a8a66
>  Manual changes to several files so that the next commit can be generated 
> programatically.
> 3. 
> https://github.com/openjdk/jdk/commit/eb6c566ff9207974a03a53335e0e697cffcf0950
>  Automatic changes to other source files to avoid javac warnings on 
> deprecation for removal
> 
> The 1st and 2nd commits should be reviewed carefully. The 3rd one is 
> generated programmatically, see the comment below for more details. If you 
> are only interested in a portion of the 3rd commit and would like to review 
> it as a separate file, please comment here and I'll generate an individual 
> webrev.
> 
> Due to the size of this PR, no attempt is made to update copyright years for 
> any file to minimize unnecessary merge conflict.
> 
> Furthermore, since the default value of `java.security.manager` system 
> property is now "disallow", most of the tests calling 
> `System.setSecurityManager()` need to launched with 
> `-Djava.security.manager=allow`. This is covered in a different PR at 
> https://github.com/openjdk/jdk/pull/4071.
> 
> Update: the deprecation annotations and javadoc tags, build, compiler, 
> core-libs, hotspot, i18n, jmx, net, nio, security, and serviceability are 
> reviewed. Rest are 2d, awt, beans, sound, and swing.

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

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

-

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

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

Integrated: 8264774: Implementation of Foreign Function and Memory API (Incubator)

[Maurizio Cimadamore]

On Mon, 26 Apr 2021 17:10:13 GMT, Maurizio Cimadamore  
wrote:

> This PR contains the API and implementation changes for JEP-412 [1]. A more 
> detailed description of such changes, to avoid repetitions during the review 
> process, is included as a separate comment.
> 
> [1] - https://openjdk.java.net/jeps/412

This pull request has now been integrated.

Changeset: a223189b
Author:Maurizio Cimadamore 
URL:   
https://git.openjdk.java.net/jdk/commit/a223189b069a7cfe49511d49b5b09e7107cb3cab
Stats: 14500 lines in 219 files changed: 8847 ins; 3642 del; 2011 mod

8264774: Implementation of Foreign Function and Memory API (Incubator)

Co-authored-by: Paul Sandoz 
Co-authored-by: Jorn Vernee 
Co-authored-by: Vladimir Ivanov 
Co-authored-by: Athijegannathan Sundararajan 
Co-authored-by: Chris Hegarty 
Reviewed-by: psandoz, chegar, mchung, vlivanov

-

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

Re: JEP 411 - Secure Java Distribution

[Peter Firmstone]

Thanks Andrew,

I've been thinking about how to do this in a compatible manner.

The Guard.check call checks whether SecurityManager is enabled.

All permission checks in the JDK could be changed to call the 
Guard.check method.  Unfortunately other permission checks in user code 
will be broken, if they don't utilise this method, but often the most 
important permissions are those defined in JDK code.


If we modify this method to look for a provider that's outside the 
java.* namespace.


Then we have a point in the code where we can do something like 
SecurityManager, without requiring SecurityManager.


Next step will be to replace all AccessController and 
AccessControlContext uses in JDK code with another class outside the 
java.* namespace.


In the first instance, we allow SecurityManager, AccessController and 
AccessControlContext to behave in their degraded state, passing all JCK 
tests.


But then we can create providers to replace their functionality.

This is my current line of thought, as other classes will remain.

Subject.doAs could be an issue, so would need to consider how to manage 
that.


This seems the easiest way.

Then for backward compatibility, we make a tool that rewrites java 
bytecode, to replace the calls to AccessController and 
AccessControlContext with compatible equivalents outside the java.* 
namespace in client code.  Then there's the case of removing any 
references to SecurityManager and looking for SecurityManager permission 
checks and replacing them with Guard.check.


Then we could potentially continue to support this functionality on 
later versions of Java without detonation.


Cheers,

Peter.


On 2/06/2021 7:35 pm, Andrew Haley wrote:

On 6/1/21 10:06 AM, Peter Firmstone wrote:

If a vendor were to continue supporting SecurityManager and was
backporting from OpenJDK, if it passes the JCK with SecurityManager
disabled, that's still acceptable right?

Look at the licence agreement in conjunction with the JCK users' guide.
See the definition of “Compatible Licensee Implementation”.

Re: JEP 411 - Secure Java Distribution

[Andrew Haley]

On 6/1/21 10:06 AM, Peter Firmstone wrote:
> If a vendor were to continue supporting SecurityManager and was 
> backporting from OpenJDK, if it passes the JCK with SecurityManager 
> disabled, that's still acceptable right?

Look at the licence agreement in conjunction with the JCK users' guide.
See the definition of “Compatible Licensee Implementation”.

-- 
Andrew Haley  (he/him)
Java Platform Lead Engineer
Red Hat UK Ltd. 
https://keybase.io/andrewhaley
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671