Re: RFR: 8282077: PKCS11 provider C_sign() impl should handle CKR_BUFFER_TOO_SMALL error [v2]

[Mikael Vidstedt]

On Sat, 19 Feb 2022 00:07:34 GMT, Valerie Peng  wrote:

>> Could someone please help review this trivial change? This is to add an 
>> error handling for the potential CKR_BUFFER_TOO_SMALL error when calling 
>> C_Sign(). Since none of the supported signature algorithms trigger this 
>> error as the default buffer size is large enough, this is more for 
>> consistency sake. No new regression test for this and thus the @noreg-hard 
>> label.
>> 
>> Thanks,
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   free allocated 'ckpData' upon OOM.

Marked as reviewed by mikael (Reviewer).

-

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

Re: RFR: 8282077: PKCS11 provider C_sign() impl should handle CKR_BUFFER_TOO_SMALL error

[Mikael Vidstedt]

On Fri, 18 Feb 2022 21:52:59 GMT, Valerie Peng  wrote:

> Could someone please help review this trivial change? This is to add an error 
> handling for the potential CKR_BUFFER_TOO_SMALL error when calling C_Sign(). 
> Since none of the supported signature algorithms trigger this error as the 
> default buffer size is large enough, this is more for consistency sake. No 
> new regression test for this and thus the @noreg-hard label.
> 
> Thanks,
> Valerie

src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_sign.c line 151:

> 149: if (bufP == NULL) {
> 150: throwOutOfMemoryError(env, 0);
> 151: return NULL;

Does ckpData need to be freed here?

-

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Mikael Vidstedt]

On Wed, 3 Feb 2021 20:08:28 GMT, Mikael Vidstedt  wrote:

>>> I wonder if this is the right choice
>>> ...
>>> ```
>>> OopStorageParIterPerf::~OopStorageParIterPerf() {
>>> ...
>>> ```
>>> 
>> 
>> The transition in OopStorageParIterPerf was made for gtest setup to execute 
>> in WXWrite context. For tests themselves, defining macro set WXWrite.
>> 
>> I've simplified the scheme and now we switch to WXWrite once at the gtest 
>> launcher. So this transition was dropped.
>> 
>> I've also refreshed my memory and tried to switch to WXWrite as close as 
>> possible to each place where we'll be writing executable memory. There are a 
>> lot of such places! As you correctly noted, code cache contains objects, not 
>> plain data. For example, CodeCache memory management structures, 
>> CompiledMethod, ...  are there, so we need more WXWrite switches than we 
>> have in the current approach. I had a comparable amount of them just to run 
>> -version, but certainly not enough to run tier1 tests.
>> 
>> Following your advice, I don't require a known "from" state anymore. So a 
>> few W^X transitions were dropped, e.g. when the JVM code calls a JNI entry 
>> function, which expects to be called from the native code. I had to switch 
>> to WXExec just only to satisfy the expectations. After the update, we don't 
>> need this anymore.
>> 
>> W^X switches are mostly hidden by VM_ENTRY and similar macros. Some JVM 
>> functions are not marked as entries for some reason, although they are 
>> called directly from e.g. interpreter. I added W^X management to such 
>> functions.
>> 
>> Thank you!
>
> Out of curiosity, have you looked at the performance of the W^X state 
> transition? In particular I'm wondering if the cost is effectively negligible 
> so doing it unconditionally on JVM entry is a no-brainer and just 
> easier/cleaner than the alternatives, or if there are reasons to look at only 
> doing the transition if/when needed (perhaps do it lazily and revert back to 
> X when leaving the JVM?).

You read my mind, Andrew. Unless, of course, it's optimized to leverage the 
fact that it's thread-specific..

-

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

Re: RFR: 8253795: Implementation of JEP 391: macOS/AArch64 Port [v9]

[Mikael Vidstedt]

On Wed, 3 Feb 2021 20:05:29 GMT, Anton Kozlov  wrote:

>> Thank you all for your comments regarding W^X implementation. I've made a 
>> change that reduces the footprint of the implementation, also addressing 
>> most of the comments. I'll revisit them individually to make sure nothing is 
>> forgotten.
>> 
>> The basic principle has not changed: when we execute JVM code (owned by 
>> libjvm.so, starting from JVM entry function), we switch to Write state. When 
>> we leave JVM to execute generated or JNI code, we switch to Executable 
>> state. I would like to highlight that JVM code does not mean the VM state of 
>> the java thread. After @stefank's suggestion, I could also drop a few W^X 
>> state switches, so now it should be more clear that switches are tied to JVM 
>> entry functions.
>
>> I wonder if this is the right choice
>> ...
>> ```
>> OopStorageParIterPerf::~OopStorageParIterPerf() {
>> ...
>> ```
>> 
> 
> The transition in OopStorageParIterPerf was made for gtest setup to execute 
> in WXWrite context. For tests themselves, defining macro set WXWrite.
> 
> I've simplified the scheme and now we switch to WXWrite once at the gtest 
> launcher. So this transition was dropped.
> 
> I've also refreshed my memory and tried to switch to WXWrite as close as 
> possible to each place where we'll be writing executable memory. There are a 
> lot of such places! As you correctly noted, code cache contains objects, not 
> plain data. For example, CodeCache memory management structures, 
> CompiledMethod, ...  are there, so we need more WXWrite switches than we have 
> in the current approach. I had a comparable amount of them just to run 
> -version, but certainly not enough to run tier1 tests.
> 
> Following your advice, I don't require a known "from" state anymore. So a few 
> W^X transitions were dropped, e.g. when the JVM code calls a JNI entry 
> function, which expects to be called from the native code. I had to switch to 
> WXExec just only to satisfy the expectations. After the update, we don't need 
> this anymore.
> 
> W^X switches are mostly hidden by VM_ENTRY and similar macros. Some JVM 
> functions are not marked as entries for some reason, although they are called 
> directly from e.g. interpreter. I added W^X management to such functions.

Out of curiosity, have you looked at the performance of the W^X state 
transition? In particular I'm wondering if the cost is effectively negligible 
so doing it unconditionally on JVM entry is a no-brainer and just 
easier/cleaner than the alternatives, or if there are reasons to look at only 
doing the transition if/when needed (perhaps do it lazily and revert back to X 
when leaving the JVM?).

-

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

RFR(T): 8248044: Backout ProblemList-ed tests introduced by JDK-8247876

[Mikael Vidstedt]

Please review this trivial change which reenables the tests which were problem 
listed because of a linux-aarch64 machine configuration issue which has since 
been fixed:

Note: I intend to push this to jdk15.

JBS: https://bugs.openjdk.java.net/browse/JDK-8248044
webrev: 
https://cr.openjdk.java.net/~mikael/webrevs/8248044/webrev.00/open/webrev

Testing:

I ran a few jobs and verified that the four tests passed (they did) and 
completed in reasonable times (they did).

Cheers,
Mikael

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

[Mikael Vidstedt]

New webrev fixing Basic.policy based on Valeries’s comment:

webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.03/security/open/webrev/
incremental: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.03/security.incr/open/webrev/

Cheers,
Mikael

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

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

[Mikael Vidstedt]

New webrev:

webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.02/security/open/webrev/
incremental: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.02/security.incr/open/webrev/

Items to be resolved:

* File follow-up to drop $ISA support in 
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java

Cheers,
Mikael

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

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

[Mikael Vidstedt]

> On May 7, 2020, at 4:08 PM, Valerie Peng  wrote:
> 
> It should be ok to remove the if (secret == null) block of 
> test/jdk/sun/security/pkcs11/tls/TestPRF.java.
> 
> I verified that the test runs fine on Linux without that block, so the 
> comment is legit.

Thank you for verifying!

I’ll revert the removal of the test/jdk/sun/security/pkcs11/Config/ files and 
send out a new webrev in a minute!

Cheers,
Mikael

> 
> Thanks,
> Valerie
> On 5/5/2020 4:17 PM, Mikael Vidstedt wrote:
>> John,
>> 
>> Thanks for the review! Comments/questions inline..
>> 
>>> On May 4, 2020, at 4:02 PM, sha.ji...@oracle.com wrote:
>>> 
>>> Hi,
>>> Generally, this patch doesn't take care of the solaris/sunos/ucrypto-related
>>> words in test summaries, code comments, configs and READMEs.
>>> E.g.
>>> test/jdk/javax/net/ssl/templates/SSLEngineTemplate.java
>>> test/jdk/sun/security/provider/MessageDigest/TestSHAClone.java
>>> test/jdk/sun/security/util/Resources/Format.config
>>> test/jdk/sun/security/pkcs11/KeyStore/BasicData/README
>>> Would we need some follow-up issues to double-check this point?
>> I’ve deliberately avoided changing comments I didn’t feel comfortable 
>> modifying. I’d prefer to file follow-ups as needed, but if you have specific 
>> suggestions (or patches!) I should include in the already huge patch do let 
>> me know.
>> 
>>> test/jdk/sun/security/tools/keytool/KeyToolTest.java
>>> 39   * Testing Solaris Cryptography Framework PKCS11 keystores:
>>> 40   *   # make sure you've already run pktool and set test12 as pin
>>> 41   *   echo | java -Dsolaris KeyToolTest
>>> ...
>>> 1863  if (System.getProperty("solaris") != null) {
>>> 1864  // For Solaris Cryptography Framework
>>> 1865  t.srcP11Arg = SUN_SRC_P11_ARG;
>>> 1866  t.p11Arg = SUN_P11_ARG;
>>> 1867  t.testPKCS11();
>>> 1868  t.testPKCS11ImportKeyStore();
>>> 1869  t.i18nPKCS11Test();
>>> 1870  }
>>> It may be necessary to remove the above lines.
>> I was staring at this one for quite a while. AFAICT there’s nothing Solaris 
>> specific about that block or the methods it’s calling - to me it looks like 
>> the property just happens to be called “solaris”, but it could equally well 
>> be called “pkcs11” or “standard” or “foobar”. That said, I don’t know enough 
>> about PKCS11 & friends to say for sure. Do let me know if it is indeed dead 
>> code and should be removed..
>> 
>>> test/jdk/sun/security/provider/PolicyParser/PrincipalExpansionError.java
>>> test/jdk/sun/security/provider/PolicyParser/PrincipalExpansionError.policy
>>> It looks this manual test and the associated policy are solaris-related 
>>> only.
>>> Could these files be removed as well?
>>> In fact, the solaris-related com.sun.security.auth classes, including
>>> SolarisPrincipal, are already removed.
>> Ah, that indeed seems to be the case. Made me wonder why the test doesn’t 
>> fail to compile altogether, but then I noticed that it’s ProblemListed on 
>> all platforms..
>> 
>> How about a follow-up since this isn’t strictly related to the Solaris/SPARC 
>> removal itself - perhaps https://bugs.openjdk.java.net/browse/JDK-8039280 
>> covers it?
>> 
>>> test/jdk/sun/security/pkcs11/tls/TestPRF.java
>>> 114  if (secret == null) {
>>> 115  // This fails on Solaris, but since we 
>>> never call this
>>> 116  // API for this case in JSSE, ignore the 
>>> failure.
>>> 117  // (SunJSSE uses the 
>>> CKM_TLS_KEY_AND_MAC_DERIVE
>>> 118  // mechanism)
>>> 119  System.out.print("X");
>>> 120  continue;
>>> 121  }
>>> Could remove this block?
>> Good question - the comment certainly makes it sound Solaris specific, but 
>> could be a stale comment..
>> 
>> Cheers,
>> Mikael
>> 
>>> On 2020/5/4 13:12, Mikael Vidstedt wrote:
>>>> Please review this change which implements part of JEP 381:
>>>> 
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>>>> webrev: 
>>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/security/open/webrev/
&

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

[Mikael Vidstedt]

Will generate a new webrev shortly, meanwhile some comments inline..

> On May 7, 2020, at 1:02 AM, Weijun Wang  wrote:
> 
> 
> 
>> On May 7, 2020, at 1:05 PM, Mikael Vidstedt  
>> wrote:
>> 
>> 
>> New webrev here:
>> 
>> webrev: 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/security/open/webrev/
>> incremental: 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/security.incr/open/webrev/
>> 
>> Items yet to be resolved:
>> 
>> * File follow-up to drop $ISA support in 
>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java
> 
> Maybe keep it now. I remember on Ubuntu the libraries are also in different 
> directories for x64 and x86.

I’ll leave it the way it is in webrev.01. Just to make sure we’re on the same 
page: the only remaining effect it will have is that the "$ISA/“ substring will 
be removed if it’s found. A possible removal can perhaps be handled as a 
follow-up instead?

> 
>> 
>> * Get confirmation on the “solaris” property in 
>> test/jdk/sun/security/tools/keytool/KeyToolTest.java
> 
> 1. remove all lines except for the 1st in the test's @summary
> 2. remove the whole "if (System.getProperty("solaris") != null)" block in 
> main()
> 3. remove the static final String fields SUN_P11_ARG and SUN_SRC_P11_ARG

Thanks for the off-thread education on why this code is indeed Solaris 
specific: all other platforms need the -addprovider option to enable PKCS11, 
Solaris comes with enabled by default.

> 
>> 
>> * Get confirmation if https://bugs.openjdk.java.net/browse/JDK-8039280 is 
>> enough to cover 
>> test/jdk/sun/security/provider/PolicyParser/PrincipalExpansionError.java
> 
> The test has been neglected for a long time, but I think in theory the 
> SolarisPrincipal can be substituted into UnixPrincipal. Let's keep it.

I changed SolarisPrincipal to UnixPrincipal in the .java and .policy files, but 
did *not* verify that the test works as expected with that change.

> 
>> 
>> * Get confirmation on what to do about the "(secret == null)” block in 
>> test/jdk/sun/security/pkcs11/tls/TestPRF.java
> 
> I don't quite understand this test, but I think we can simply remove the 
> block. The comment seems to suggest that this will not fail on Solaris. Let's 
> see what would happen on other platforms.

Consider it gone!

Cheers,
Mikael

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

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

[Mikael Vidstedt]

New webrev here:

webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/security/open/webrev/
incremental: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.01/security.incr/open/webrev/

Items yet to be resolved:

* File follow-up to drop $ISA support in 
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java

* Get confirmation on the “solaris” property in 
test/jdk/sun/security/tools/keytool/KeyToolTest.java

* Get confirmation if https://bugs.openjdk.java.net/browse/JDK-8039280 is 
enough to cover 
test/jdk/sun/security/provider/PolicyParser/PrincipalExpansionError.java

* Get confirmation on what to do about the "(secret == null)” block in 
test/jdk/sun/security/pkcs11/tls/TestPRF.java

Cheers,
Mikael

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

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

[Mikael Vidstedt]

> On May 5, 2020, at 7:29 PM, Weijun Wang  wrote:
> 
> 
> 
>> On May 6, 2020, at 6:51 AM, Mikael Vidstedt  
>> wrote:
>> 
>> 
>> Max,
>> 
>> Thank you so much for the thorough review! I’m working on an incremental 
>> webrev but could use some help - comments/questions inline..
>> 
>>> On May 4, 2020, at 6:58 AM, Weijun Wang  wrote:
>>> 
>>> I've gone through all files. Many of them are PKCS11-related, it will be 
>>> nice if Valerie can confirm my findings.
>>> 
>>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java:
>>> 
>>> Maybe we should not support $ISA at all?
>> 
>> It does seem like it could go, but I’m not comfortable making that change 
>> myself. How about a follow-up issue?
> 
> Sure.
> 
>> 
>>> test/jdk/com/sun/security/auth/login/ConfigFile/InconsistentError.java:
>>> 
>>> Please also remove the whole else block from line 61.
>> 
>> Fixed.
>> 
>>> test/jdk/java/security/KeyPairGenerator/SolarisShortDSA.java:
>>> 
>>> It's not worth keeping this test. Error has never occurred on other 
>>> platforms.
>> 
>> Fixed.
>> 
>>> test/jdk/java/security/KeyStore/TestKeyStoreBasic.java:
>>> 
>>> 128-135: There is no need to use a try-catch block.
>> 
>> Fixed.
>> 
>>> test/jdk/java/security/KeyStore/TestKeyStoreEntry.java:
>>> 
>>> 54-60: No need
>> 
>> Fixed.
>> 
>>> 81-97: No need. Just a single run() is OK. 
>>> 101: Remove the ", p” argument
>> 
>> I’m not sure I understand what to do here. Help? :)
> 
> Since there is only one provider having "jceks" you can 
> 
> 1 remove run() method
> 2 rename runTest(p) to run()
> 3 call 'KeyStore.getInstance("jceks")' without the "p" argument.

Thank you! I did exactly that.

> 
>> 
>>> test/jdk/java/security/MessageDigest/TestDigestIOStream.java:
>>> test/jdk/java/security/MessageDigest/TestSameLength.java:
>>> test/jdk/java/security/MessageDigest/TestSameValue.java:
>>> 
>>> No need for isSHA3Supported(). The SUN provider should always be there.
>> 
>> Fixed.
>> 
>>> Inside the test where NoSuchAlgorithmException is caught, the catch block 
>>> can be removed and no need to try
>> 
>> Fixed.
>> 
>>> test/jdk/java/security/MessageDigest/UnsupportedProvider.java:
>>> 
>>> Not worth keeping this test.
>> 
>> Fixed.
>> 
>>> test/jdk/sun/security/jca/PreferredProviderTest.java:
>>> 
>>> 53: remove "for other platform”
>> 
>> Fixed.
>> 
>>> test/jdk/sun/security/pkcs/pkcs8/PKCS8Test.java
>>> 
>>> Remove the comments on lines 37-40
>> 
>> Fixed.
>> 
>>> test/jdk/sun/security/pkcs11/Config/ReadConfInUTF16Env.java:
>>> 
>>> Not worth keeping the tests in this directory.
>> 
>> Fixed.
>> 
>>> test/jdk/sun/security/pkcs11/Mac/MacSameTest.java:
>>> 
>>> No need for try
>> 
>> Fixed.
>> 
>>> test/jdk/sun/security/pkcs11/Provider/ConfigShortPath.java:
>>> 
>>> Will this test ever run? There is no out-of-box SunPKCS11 provider. Even if 
>>> one is configured manually, the name is likely to be SunPKCS11-XYZ,
>> 
>> You tell me? :)
> 
> This is why I included Valerie, I'll ping her.

Sounds good, I’ll wait for Valerie’s feedback.

Cheers,
Mikael

> 
> Thanks,
> Max
> 
>> 
>>> test/jdk/sun/security/pkcs11/tls/TestKeyMaterial.java:
>>> 
>>> No need for this catch block
>> 
>> Fixed (removed the InvalidAlgorithmParameterException catch).
>> 
>>> test/jdk/sun/security/tools/keytool/fakegen/PSS.java:
>>> 
>>> Please also remove the comment on lines 35-37.
>> 
>> Fixed.
>> 
>>> test/jdk/sun/security/tools/keytool/fakegen/DefaultSignatureAlgorithm.java:
>>> 
>>> Please also remove the comment on lines 36-38.
>> 
>> Fixed.
>> 
>> Cheers,
>> Mikael
>> 
>>> 
>>>> On May 4, 2020, at 1:12 PM, Mikael Vidstedt  
>>>> wrote:
>>>> 
>>>> 
>>>> Please review this change which implements part of JEP 381:
>>>> 
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>>>> webrev: 
>>>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/security/open/webr

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

[Mikael Vidstedt]

John,

Thanks for the review! Comments/questions inline..

> On May 4, 2020, at 4:02 PM, sha.ji...@oracle.com wrote:
> 
> Hi,
> Generally, this patch doesn't take care of the solaris/sunos/ucrypto-related
> words in test summaries, code comments, configs and READMEs.
> E.g.
> test/jdk/javax/net/ssl/templates/SSLEngineTemplate.java
> test/jdk/sun/security/provider/MessageDigest/TestSHAClone.java
> test/jdk/sun/security/util/Resources/Format.config
> test/jdk/sun/security/pkcs11/KeyStore/BasicData/README
> Would we need some follow-up issues to double-check this point?

I’ve deliberately avoided changing comments I didn’t feel comfortable 
modifying. I’d prefer to file follow-ups as needed, but if you have specific 
suggestions (or patches!) I should include in the already huge patch do let me 
know.

> test/jdk/sun/security/tools/keytool/KeyToolTest.java
> 39   * Testing Solaris Cryptography Framework PKCS11 keystores:
> 40   *   # make sure you've already run pktool and set test12 as pin
> 41   *   echo | java -Dsolaris KeyToolTest
> ...
> 1863  if (System.getProperty("solaris") != null) {
> 1864  // For Solaris Cryptography Framework
> 1865  t.srcP11Arg = SUN_SRC_P11_ARG;
> 1866  t.p11Arg = SUN_P11_ARG;
> 1867  t.testPKCS11();
> 1868  t.testPKCS11ImportKeyStore();
> 1869  t.i18nPKCS11Test();
> 1870  }
> It may be necessary to remove the above lines.

I was staring at this one for quite a while. AFAICT there’s nothing Solaris 
specific about that block or the methods it’s calling - to me it looks like the 
property just happens to be called “solaris”, but it could equally well be 
called “pkcs11” or “standard” or “foobar”. That said, I don’t know enough about 
PKCS11 & friends to say for sure. Do let me know if it is indeed dead code and 
should be removed..

> test/jdk/sun/security/provider/PolicyParser/PrincipalExpansionError.java
> test/jdk/sun/security/provider/PolicyParser/PrincipalExpansionError.policy
> It looks this manual test and the associated policy are solaris-related only.
> Could these files be removed as well?
> In fact, the solaris-related com.sun.security.auth classes, including
> SolarisPrincipal, are already removed.

Ah, that indeed seems to be the case. Made me wonder why the test doesn’t fail 
to compile altogether, but then I noticed that it’s ProblemListed on all 
platforms..

How about a follow-up since this isn’t strictly related to the Solaris/SPARC 
removal itself - perhaps https://bugs.openjdk.java.net/browse/JDK-8039280 
covers it?

> test/jdk/sun/security/pkcs11/tls/TestPRF.java
> 114  if (secret == null) {
> 115  // This fails on Solaris, but since we never 
> call this
> 116  // API for this case in JSSE, ignore the 
> failure.
> 117  // (SunJSSE uses the 
> CKM_TLS_KEY_AND_MAC_DERIVE
> 118  // mechanism)
> 119  System.out.print("X");
> 120  continue;
> 121  }
> Could remove this block?

Good question - the comment certainly makes it sound Solaris specific, but 
could be a stale comment..

Cheers,
Mikael

> 
> On 2020/5/4 13:12, Mikael Vidstedt wrote:
>> Please review this change which implements part of JEP 381:
>> 
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
>> webrev: 
>> http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/security/open/webrev/
>> JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
>> 
>> 
>> Note: When reviewing this, please be aware that this exercise was 
>> *extremely* mind-numbing, so I appreciate your help reviewing all the 
>> individual changes carefully. You may want to get that coffee cup filled up 
>> (or whatever keeps you awake)!
>> 
>> 
>> Background:
>> 
>> Because of the size of the total patch and wide range of areas touched, this 
>> patch is one out of in total six partial patches which together make up the 
>> necessary changes to remove the Solaris and SPARC ports. The other patches 
>> are being sent out for review to mailing lists appropriate for the 
>> respective areas the touch. An email will be sent to jdk-dev summarizing all 
>> the patches/reviews. To be clear: this patch is *not* in itself complete and 
>> stand-alone - all of the (six) patches are needed to form a complete patch. 
>> Some changes in this patch may look wrong or incomplete unless also looking 
>> at the corresponding changes in other areas.
>> 
>> For convenience, I’m in

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

[Mikael Vidstedt]

Max,

Thank you so much for the thorough review! I’m working on an incremental webrev 
but could use some help - comments/questions inline..

> On May 4, 2020, at 6:58 AM, Weijun Wang  wrote:
> 
> I've gone through all files. Many of them are PKCS11-related, it will be nice 
> if Valerie can confirm my findings.
> 
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java:
> 
>   Maybe we should not support $ISA at all?

It does seem like it could go, but I’m not comfortable making that change 
myself. How about a follow-up issue?

> test/jdk/com/sun/security/auth/login/ConfigFile/InconsistentError.java:
> 
>   Please also remove the whole else block from line 61.

Fixed.

> test/jdk/java/security/KeyPairGenerator/SolarisShortDSA.java:
> 
>   It's not worth keeping this test. Error has never occurred on other 
> platforms.

Fixed.

> test/jdk/java/security/KeyStore/TestKeyStoreBasic.java:
> 
>   128-135: There is no need to use a try-catch block.

Fixed.

> test/jdk/java/security/KeyStore/TestKeyStoreEntry.java:
> 
>   54-60: No need

Fixed.

>   81-97: No need. Just a single run() is OK. 
>   101: Remove the ", p” argument

I’m not sure I understand what to do here. Help? :)

> test/jdk/java/security/MessageDigest/TestDigestIOStream.java:
> test/jdk/java/security/MessageDigest/TestSameLength.java:
> test/jdk/java/security/MessageDigest/TestSameValue.java:
> 
>   No need for isSHA3Supported(). The SUN provider should always be there.

Fixed.

>   Inside the test where NoSuchAlgorithmException is caught, the catch block 
> can be removed and no need to try

Fixed.

> test/jdk/java/security/MessageDigest/UnsupportedProvider.java:
> 
>   Not worth keeping this test.

Fixed.

> test/jdk/sun/security/jca/PreferredProviderTest.java:
> 
>   53: remove "for other platform”

Fixed.

> test/jdk/sun/security/pkcs/pkcs8/PKCS8Test.java
> 
>   Remove the comments on lines 37-40

Fixed.

> test/jdk/sun/security/pkcs11/Config/ReadConfInUTF16Env.java:
> 
>   Not worth keeping the tests in this directory.

Fixed.

> test/jdk/sun/security/pkcs11/Mac/MacSameTest.java:
> 
>   No need for try

Fixed.

> test/jdk/sun/security/pkcs11/Provider/ConfigShortPath.java:
> 
>   Will this test ever run? There is no out-of-box SunPKCS11 provider. Even if 
> one is configured manually, the name is likely to be SunPKCS11-XYZ,

You tell me? :)

> test/jdk/sun/security/pkcs11/tls/TestKeyMaterial.java:
> 
>   No need for this catch block

Fixed (removed the InvalidAlgorithmParameterException catch).

> test/jdk/sun/security/tools/keytool/fakegen/PSS.java:
> 
>   Please also remove the comment on lines 35-37.

Fixed.

> test/jdk/sun/security/tools/keytool/fakegen/DefaultSignatureAlgorithm.java:
> 
>   Please also remove the comment on lines 36-38.

Fixed.

Cheers,
Mikael

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

RFR: 8244224: Implementation of JEP 381: Remove the Solaris and SPARC Ports (security)

[Mikael Vidstedt]

Please review this change which implements part of JEP 381:

JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/security/open/webrev/
JEP: https://bugs.openjdk.java.net/browse/JDK-8241787


Note: When reviewing this, please be aware that this exercise was *extremely* 
mind-numbing, so I appreciate your help reviewing all the individual changes 
carefully. You may want to get that coffee cup filled up (or whatever keeps you 
awake)!


Background:

Because of the size of the total patch and wide range of areas touched, this 
patch is one out of in total six partial patches which together make up the 
necessary changes to remove the Solaris and SPARC ports. The other patches are 
being sent out for review to mailing lists appropriate for the respective areas 
the touch. An email will be sent to jdk-dev summarizing all the 
patches/reviews. To be clear: this patch is *not* in itself complete and 
stand-alone - all of the (six) patches are needed to form a complete patch. 
Some changes in this patch may look wrong or incomplete unless also looking at 
the corresponding changes in other areas.

For convenience, I’m including a link below[1] to the full webrev, but in case 
you have comments on changes in other areas, outside of the files included in 
this thread, please provide those comments directly in the thread on the 
appropriate mailing list for that area if possible.

In case it helps, the changes were effectively produced by searching for and 
updating any code mentioning “solaris", “sparc”, “solstudio”, “sunos”, etc. 
More information about the areas impacted can be found in the JEP itself.


Testing:

A slightly earlier version of this change successfully passed tier1-8, as well 
as client tier1-2. Additional testing will be done after the first round of 
reviews has been completed.

Cheers,
Mikael

[1] 
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/

Re: RFR(XS): 8210912: Build error in src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_convert.c after JDK-8029661

[Mikael Vidstedt]

Thank you! Change pushed and noreg-build label added.

Cheers,
Mikael

> On Sep 19, 2018, at 9:13 AM, Sean Mullan  wrote:
> 
> Looks ok to me. The bug needs an appropriate noreg label.
> 
> --Sean
> 
> On 9/19/18 12:05 PM, Mikael Vidstedt wrote:
>> Please review this change which fixes a Solaris/SPARC build issue:
>> bug: https://bugs.openjdk.java.net/browse/JDK-8210912
>> webrev: 
>> http://cr.openjdk.java.net/~mikael/webrevs/8210912/webrev.00/open/webrev/ 
>> <http://cr.openjdk.java.net/%7Emikael/webrevs/8210912/webrev.00/open/webrev/>
>> Testing:
>> Tier1 test job (still in progress, but relevant builds passed).
>> Cheers,
>> Mikael

RFR(XS): 8210912: Build error in src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_convert.c after JDK-8029661

[Mikael Vidstedt]

Please review this change which fixes a Solaris/SPARC build issue:

bug: https://bugs.openjdk.java.net/browse/JDK-8210912 

webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8210912/webrev.00/open/webrev/ 


Testing:

Tier1 test job (still in progress, but relevant builds passed).

Cheers,
Mikael

RFR JDK-8202060: Add javax/net/ssl/DTLS/CipherSuite.java to ProblemList

[Mikael Vidstedt]

Please review this change which adds javax/net/ssl/DTLS/CipherSuite.java to the 
ProblemList until https://bugs.openjdk.java.net/browse/JDK-8202059 
 is fixed.

Bug: https://bugs.openjdk.java.net/browse/JDK-8202060 

Webrev: 
http://cr.openjdk.java.net/~mikael/webrevs/8202060/webrev.00/open/webrev/ 


Cheers,
Mikael

Re: 8034856/8034857: More gcc warnings

[Mikael Vidstedt]

Alan,

I made the change to JarFacade.c myself last week, only to then see the comment 
a few lines above where you added the new include. It seems to indicate that 
including ctype.h on Solaris/SPARC is a bad idea. I have no idea if the comment 
is still relevant, but that may be worth understanding first.

Cheers,
Mikael

 On Feb 13, 2014, at 5:18, Alan Bateman alan.bate...@oracle.com wrote:
 
 
 The number of native code warnings in the build is annoying so this is 
 another drive-by fix that eliminates a few of them in the serviceability and 
 security areas. The webrev with the changes is here:
 
 http://cr.openjdk.java.net/~alanb/8034856+8034857/webrev/
 
 In the pkcs11 code the issue is the function prototypes for the throwXXX 
 functions aren't included. This is fixed by including pkcs11wrapper.h but 
 that exposes another issue with the header file includes that needed to be 
 fixed.
 
 In JarFacade the issue is that it uses isspace but doesn't include the ctype.h
 
 For LinuxOperatingSystem.c then there are 12 warnings related to fscanf 
 usages where the format specifier is %lld and the code wants to read into a 
 uint64_t. I've changed the format specifier to%SCNd64 so that it matches 
 uint64_t and should be okay on both 32 and 64-bit.
 
 Thanks,
 Alan.

Re: 8034856/8034857: More gcc warnings

[Mikael Vidstedt]

On 2014-02-13 10:23, Alan Bateman wrote:

On 13/02/2014 17:56, Mikael Vidstedt wrote:

Alan,

I made the change to JarFacade.c myself last week, only to then see 
the comment a few lines above where you added the new include. It 
seems to indicate that including ctype.h on Solaris/SPARC is a bad 
idea. I have no idea if the comment is still relevant, but that may 
be worth understanding first.



Do you have cycles to look into it? As the code is using isspace 
already then it's not clear (unless there are different versions). 
Before pushing the changes then I ran the tests on all platforms 
(including Solaris) and the j.l.i tests include a number of tests 
exercise these manifest attributes with a non-US characters.


The change in question appears to come from 
https://bugs.openjdk.java.net/browse/JDK-6679866, but I'm not sure the 
bug gives enough additional information. My speculation (and it's really 
just a speculation) is that it's not related to isspace per-se, but to 
something else which gets defined/redefined/undefined by including 
ctype.h. I guess it would be good to know if we have tests which cover 
the thing the comment is alluding to (non-ascii in Premain-Class).


As an aside, the native code warnings coming from the jdk repository 
are really annoying so this is the reason for the drive-by fixes when 
I get a few minutes. I think others are doing the same.


Absolutely support this work! As a matter of fact I have a couple of 
change in a sandbox I should send out for review.


Cheers,
Mikael