Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v7]
> This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. Xue-Lei Andrew Fan has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains ten commits: - Merge - Merge master - Merge - add timeout parameter - rollback is not in this branch - rollback JDK-8287384 - back to wait 1 second - Remove trailing white space - 8287596: Reorg jdk.test.lib.util.ForceGC - Changes: https://git.openjdk.java.net/jdk/pull/8979/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8979=06 Stats: 86 lines in 10 files changed: 12 ins; 29 del; 45 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v6]
> This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. Xue-Lei Andrew Fan has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits: - Merge master - Merge - add timeout parameter - rollback is not in this branch - rollback JDK-8287384 - back to wait 1 second - Remove trailing white space - 8287596: Reorg jdk.test.lib.util.ForceGC - Changes: https://git.openjdk.java.net/jdk/pull/8979/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8979=05 Stats: 86 lines in 10 files changed: 12 ins; 29 del; 45 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v2]
On Wed, 1 Jun 2022 21:09:15 GMT, Mandy Chung wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> back to wait 1 second > > No, it doesn't work. You can build a fastdebug build with `configure > --enable-debug`. I reproduce it on macOS. If I restore to the previous > version without 8287384, the test passes. Inspired by @mlchung (See https://github.com/openjdk/jdk/pull/9021), use less waiting time for UnloadingTest and re-open this PR. - PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v5]
> This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. Xue-Lei Andrew Fan has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains seven commits: - Merge - add timeout parameter - rollback is not in this branch - rollback JDK-8287384 - back to wait 1 second - Remove trailing white space - 8287596: Reorg jdk.test.lib.util.ForceGC - Changes: https://git.openjdk.java.net/jdk/pull/8979/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8979=04 Stats: 87 lines in 10 files changed: 19 ins; 25 del; 43 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v4]
> This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: rollback is not in this branch - Changes: - all: https://git.openjdk.java.net/jdk/pull/8979/files - new: https://git.openjdk.java.net/jdk/pull/8979/files/a8768e09..4a80db95 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8979=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8979=02-03 Stats: 58 lines in 1 file changed: 17 ins; 24 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
Withdrawn: 8287596: Reorg jdk.test.lib.util.ForceGC
On Wed, 1 Jun 2022 19:08:03 GMT, Xue-Lei Andrew Fan wrote: > This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v3]
> This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: rollback JDK-8287384 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8979/files - new: https://git.openjdk.java.net/jdk/pull/8979/files/f3d9eb82..a8768e09 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8979=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8979=01-02 Stats: 55 lines in 1 file changed: 21 ins; 14 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v2]
On Wed, 1 Jun 2022 21:07:16 GMT, Xue-Lei Andrew Fan wrote: >> This is a follow up update per comments in [JDK-8287384 >> PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in >> open part looks good to me. Please help to run Mach5 just case the closed >> test cases are impacted. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > back to wait 1 second My macOS (M1) and Linux (CentOS) may be too powerful to reproduce the failure. I tried to set the JTREG time out to 60 seconds, and the timeout issue could be reproduced. In `test/jdk/java/lang/invoke/defineHiddenClass/UnloadingTest.java`, there are 6 calls to the method `assertFalse(unloader.tryUnload())`. Each call to tryUnload() takes at least 10 seconds. So the 6 calls takes 60 seconds at least. If I set the regression timeout value to 70 seconds, the test still can pass. It implies the rest part other than tryUnload() is pretty fast. It looks like a regression introduced with the update for [JDK-8287384](https://bugs.openjdk.java.net/browse/JDK-8287384). I did not fine the cause yet. I will have more checking tomorrow. - PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC
On Wed, 1 Jun 2022 20:45:07 GMT, Mandy Chung wrote: > JDK-8287384 causes > `test/jdk/java/lang/invoke/defineHiddenClass/UnloadingTest.java` to timeout > when running with fastdebug VM. I think this might be caused by more frequent > GCs. > > I tried your patch and the test fails. I updated the waiting time back to 1 second. Would you please check again? > JDK-8287384 causes > `test/jdk/java/lang/invoke/defineHiddenClass/UnloadingTest.java` to timeout > when running with fastdebug VM. I think this might be caused by more frequent > GCs. > > I tried your patch and the test fails. I updated the waiting time back to 1 second. Would you please check again? - PR: https://git.openjdk.java.net/jdk/pull/8979
Re: RFR: 8287596: Reorg jdk.test.lib.util.ForceGC [v2]
> This is a follow up update per comments in [JDK-8287384 > PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in > open part looks good to me. Please help to run Mach5 just case the closed > test cases are impacted. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: back to wait 1 second - Changes: - all: https://git.openjdk.java.net/jdk/pull/8979/files - new: https://git.openjdk.java.net/jdk/pull/8979/files/a1d91596..f3d9eb82 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8979=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8979=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
RFR: 8287596: Reorg jdk.test.lib.util.ForceGC
This is a follow up update per comments in [JDK-8287384 PR](https://github.com/openjdk/jdk/pull/8907). The tier1 and tier2 test in open part looks good to me. Please help to run Mach5 just case the closed test cases are impacted. - Commit messages: - Remove trailing white space - 8287596: Reorg jdk.test.lib.util.ForceGC Changes: https://git.openjdk.java.net/jdk/pull/8979/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8979=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287596 Stats: 83 lines in 10 files changed: 12 ins; 40 del; 31 mod Patch: https://git.openjdk.java.net/jdk/pull/8979.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8979/head:pull/8979 PR: https://git.openjdk.java.net/jdk/pull/8979
Integrated: 8286045: Use ForceGC for cleaner test cases
On Wed, 25 May 2022 15:20:45 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have the test case update reviewed? > > This patch is trying to use ForceGC for cleaner test cases. As would make > the test more stable and easier to maintain. > > Thanks, > Xuelei This pull request has now been integrated. Changeset: 7eb15593 Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/7eb15593e18a923bbc18c8d596cff87d87019640 Stats: 57 lines in 3 files changed: 19 ins; 28 del; 10 mod 8286045: Use ForceGC for cleaner test cases Reviewed-by: rriggs - PR: https://git.openjdk.java.net/jdk/pull/8885
Re: RFR: 8286045: Use ForceGC for cleaner test cases
On Wed, 25 May 2022 17:23:06 GMT, Roger Riggs wrote: > (But ForceGC is a heavyweight blunt instrument. It creates a new Cleaner for > every instance and an instance can only be used once. Also, its minimum > wait/sleep time is 1 second, that's a lng time.). I thought about something similar when I read the ForceGC implementation. I may file an enhancement for ForceGC later. - PR: https://git.openjdk.java.net/jdk/pull/8885
RFR: 8286045: Use ForceGC for cleaner test cases
Hi, May I have the test case update reviewed? This patch is trying to use ForceGC for cleaner test cases. As would make the test more stable and easier to maintain. Thanks, Xuelei - Commit messages: - 8286045: Use ForceGC for cleaner test cases Changes: https://git.openjdk.java.net/jdk/pull/8885/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8885=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286045 Stats: 57 lines in 3 files changed: 19 ins; 28 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/8885.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8885/head:pull/8885 PR: https://git.openjdk.java.net/jdk/pull/8885
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Thu, 19 May 2022 09:31:07 GMT, Kevin Walls wrote: >> Replaces usages of articles that follow each other in all combinations: >> a/the, an?/an?, the/the… >> >> Also, I fixed a couple of spelling mistakes. > > test/jdk/sun/security/tools/jarsigner/OldSig.java line 32: > >> 30: /* >> 31: * See also PreserveRawManifestEntryAndDigest.java for tests with >> arbitrarily >> 32: * formatted individual sections in addition the main attributes tested > > "in addition to the..." +1. - PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284191: Replace usages of 'a the' in hotspot and java.base
On Wed, 18 May 2022 13:27:24 GMT, Alexey Ivanov wrote: > Replaces usages of articles that follow each other in all combinations: > a/the, an?/an?, the/the… > > Also, I fixed a couple of spelling mistakes. The security/crypto parts look good to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8768
Re: RFR: 8284926: Share the certificate NamedGroup in SignatureScheme::getSignerOfPreferableAlgorithm [v2]
On Mon, 18 Apr 2022 12:37:15 GMT, John Jiang wrote:
>> It would not to generate the certificate's ECParameterSpec and NamedGroup
>> multiple times in method `SignatureScheme::getSignerOfPreferableAlgorithm`.
>
> John Jiang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> cache ParamSpec and NamedGroup in X509Possession
src/java.base/share/classes/sun/security/ssl/ECDHClientKeyExchange.java line
274:
> 272: // Iteratively determine the X509Possession type's
> ParameterSpec.
> 273: ECParameterSpec ecParams =
> x509Possession.getECParameterSpec();
> 274: NamedParameterSpec namedParams =
> x509Possession.getXECParameterSpec();
It may not necessary to define 'ecParams' and 'namedParams' any longer, which
was used to find out the named group. Now, the checking could be placed on the
"namedGroup" (if the named group is EC/CDH) around line 293.
src/java.base/share/classes/sun/security/ssl/SignatureScheme.java line 476:
> 474: PrivateKey signingKey = x509Possession.popPrivateKey;
> 475:
> 476: ECParameterSpec params = x509Possession.getECParameterSpec();
This 'params' variable is used for debug only. Maybe, it could be moved to the
debug log block.
src/java.base/share/classes/sun/security/ssl/X509Authentication.java line 157:
> 155: }
> 156:
> 157: private ECParameterSpec getECParams() {
'getECParamSpec' may be a better method name.
src/java.base/share/classes/sun/security/ssl/X509Authentication.java line 182:
> 180:
> 181: // Similar to above, but for XEC.
> 182: private NamedParameterSpec getXECParams() {
'getXECParamSpec' may be a better method name.
-
PR: https://git.openjdk.java.net/jdk/pull/8271
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]
On Mon, 16 May 2022 21:08:48 GMT, Anthony Scarpino
wrote:
> There is too much grey area. It says the src buffer maybe modified, which one
> could interpret it cannot be a read-only, but that would still need
> clarification to explicitly say "no read only buffers". And other than these
> internal 'in-place' crypto reason, there is no API reason to not allow
> read-only buffers as input. I did think about closing the CSR as the text was
> already there about the src buffer, even thought it was using a different
> term. But I felt strongly enough that I wanted to prevent that confusion in
> the future.
I think it is good to make the clarification with a CSR.
As the spec says, "The inbound network buffer, {@code src}, may be modified",
applications cannot assume that the inbound network buffer cannot be modified
(read-only) any longer. It does not grant that an application will work with
read-only inbound buffers for another JSSE provider. As a result, an
application that want to support read-only buffers may also need to support
non-read-only buffers. As makes it more complicated in both application layers
and the JSSE implementation layer. It may be simple that applications and JSSE
implementation codes only need to handle non-read-only buffers.
Just my $0.02. I will let you and Brad for the final decision.
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8286773: cleanup @returns in sun.security classes
On Sat, 14 May 2022 13:41:38 GMT, John Jiang wrote: > The below sun.security classes should use `@return` rather than `@returns`. > sun/security/tools/keytool/Main.java > sun/security/util/DerValue.java Looks good to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8714
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]
On Sat, 14 May 2022 03:29:14 GMT, Anthony Scarpino wrote: >> Hi, >> >> I need a review of this fix to allow a read-only 'src' buffer to be used >> with SSLEngine.unwrap(). A temporary read-write buffer is created in the >> SSLCipher operation when a read-only buffer is passed. If the 'src' is >> read-write, there is no effect on the current operation >> >> The PR also includes a CSR for an API implementation note to the >> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption >> operation. 'unwrap()' has had this behavior forever, so there is no >> compatibility issue with this note. Using the 'src' buffer for in-place >> decryption was a performance decision. >> >> Tony > > Anthony Scarpino has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - review update > - update some nits > - PR ready > - Initial As the specification has been indicate that the input buffer could be updated, what do you think if closing the bug as "Not an issue" (or clarify the spec but no implementation update)? I was just wondering if it really worthy the effort to make the code more complicated. - PR: https://git.openjdk.java.net/jdk/pull/8462
Integrated: 8286423: Destroy password protection in the example code in KeyStore
On Tue, 10 May 2022 04:13:43 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this simple example update in the KeyStore specification? > > Password protection should be destroyed in the example code in KeyStore > specification. Otherwise, applications may just copy and past the code, and > forget to clean up password protection. > > It's a trivial update, and may not worthy of a CSR. But please let me know > if you would like to have a CSR filed. > > Thanks, > Xuelei This pull request has now been integrated. Changeset: 1904e9d2 Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/1904e9d280d1cce2deead4d4aa39dda1beb9dff1 Stats: 28 lines in 1 file changed: 14 ins; 13 del; 1 mod 8286423: Destroy password protection in the example code in KeyStore Reviewed-by: weijun - PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Wed, 11 May 2022 22:49:02 GMT, Anthony Scarpino
wrote:
>> src/java.base/share/classes/javax/net/ssl/SSLEngine.java line 677:
>>
>>> 675: * @see #unwrap(ByteBuffer, ByteBuffer[], int, int)
>>> 676: *
>>> 677: * @implNote The data in {@code src} may be modified during the
>>> decryption
>>
>> It looks like a note for the API users to me. Is apiNote tag more
>> appropriate here?
>
> The CSR and this code is changing, the note I was adding was already
> documented, but its existing wording should be more clear.
I like the new update as specified in the CSR. I added my name as Reviewer for
the CSR.
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8286433: Cache certificates decoded from TLS session tickets
On Mon, 9 May 2022 19:38:36 GMT, Daniel Jeliński wrote: > When a TLS server resumes a session from a stateless session ticket, it > populates the `SSLSessionImpl`'s `localCerts` and `peerCerts` fields with > certificates deserialized from the session ticket. These certificates are > often the same across a large number of tickets. > > This patch implements a certificate cache lookup for these certificates. This > enables us to avoid deserializing the same certificates repeatedly, and saves > memory by reusing the same certificate objects. It looks good to me. Thanks! - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8608
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer
On Fri, 29 Apr 2022 03:58:57 GMT, Anthony Scarpino
wrote:
> Hi,
>
> I need a review of this fix to allow a read-only 'src' buffer to be used with
> SSLEngine.unwrap(). A temporary read-write buffer is created in the SSLCipher
> operation when a read-only buffer is passed. If the 'src' is read-write,
> there is no effect on the current operation
>
> The PR also includes a CSR for an API implementation note to the
> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption
> operation. 'unwrap()' has had this behavior forever, so there is no
> compatibility issue with this note. Using the 'src' buffer for in-place
> decryption was a performance decision.
>
> Tony
src/java.base/share/classes/javax/net/ssl/SSLEngine.java line 677:
> 675: * @see #unwrap(ByteBuffer, ByteBuffer[], int, int)
> 676: *
> 677: * @implNote The data in {@code src} may be modified during the
> decryption
It looks like a note for the API users to me. Is apiNote tag more appropriate
here?
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore [v3]
On Tue, 10 May 2022 22:09:14 GMT, Weijun Wang wrote: >> Oops, I tried to check but finally forgot about it. Thanks! > > It's probably better to convert these long example code to snippets, maybe > next time. The length is a little bit long, but it is fine to me. It may be nice to revise the example and description, but let's do it later. I modified the example by using one try clause only. - PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore [v3]
> Hi, > > May I have this simple example update in the KeyStore specification? > > Password protection should be destroyed in the example code in KeyStore > specification. Otherwise, applications may just copy and past the code, and > forget to clean up password protection. > > It's a trivial update, and may not worthy of a CSR. But please let me know > if you would like to have a CSR filed. > > Thanks, > Xuelei Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: More udapte - Changes: - all: https://git.openjdk.java.net/jdk/pull/8623/files - new: https://git.openjdk.java.net/jdk/pull/8623/files/e0bd03d0..442c1fda Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8623=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8623=01-02 Stats: 4 lines in 1 file changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8623.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8623/head:pull/8623 PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8286378: Address possibly lossy conversions in java.base
On Tue, 10 May 2022 21:32:10 GMT, Roger Riggs wrote: > PR#8599 8244681: proposes to add compiler warnings for possible lossy > conversions > From the CSR: > > "If the type of the right-hand operand of a compound assignment is not > assignment compatible with the type of the variable, a cast is implied and > possible lossy conversion may silently occur. While similar situations are > resolved as compilation errors for primitive assignments, there are no > similar rules defined for compound assignments." > > This PR anticipates the warnings and adds explicit casts to replace the > implicit casts. > In most cases, the cast matches the type of the right-hand side to type of > the compound assignment. > Since these casts are truncating the value, there might be a different > refactoring that avoids the cast > but a direct replacement was chosen here. > > (Advise and suggestions will inform similar updates to other OpenJDK modules). The update in security area looks good to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8642
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore [v2]
On Tue, 10 May 2022 13:36:19 GMT, Weijun Wang wrote:
>> Xue-Lei Andrew Fan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use PasswordProtection
>
> src/java.base/share/classes/java/security/KeyStore.java line 165:
>
>> 163: *}
>> 164: *} finally {
>> 165: *protParam.destroy();
>
> `KeyStore.ProtectionParameter` does not have a `destroy` method. Only
> `PasswordProtection` does.
Oops, I tried to check but finally forgot about it. Thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8286423: Destroy password protection in the example code in KeyStore [v2]
> Hi, > > May I have this simple example update in the KeyStore specification? > > Password protection should be destroyed in the example code in KeyStore > specification. Otherwise, applications may just copy and past the code, and > forget to clean up password protection. > > It's a trivial update, and may not worthy of a CSR. But please let me know > if you would like to have a CSR filed. > > Thanks, > Xuelei Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: Use PasswordProtection - Changes: - all: https://git.openjdk.java.net/jdk/pull/8623/files - new: https://git.openjdk.java.net/jdk/pull/8623/files/12c745d9..e0bd03d0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8623=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8623=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8623.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8623/head:pull/8623 PR: https://git.openjdk.java.net/jdk/pull/8623
RFR: 8286423: Destroy password protection in the example code in KeyStore
Hi, May I have this simple example update in the KeyStore specification? Password protection should be destroyed in the example code in KeyStore specification. Otherwise, applications may just copy and past the code, and forget to clean up password protection. It's a trivial update, and may not worthy of a CSR. But please let me know if you would like to have a CSR filed. Thanks, Xuelei - Commit messages: - 8286423: Destroy password protection in the example code in KeyStore Changes: https://git.openjdk.java.net/jdk/pull/8623/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8623=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286423 Stats: 18 lines in 1 file changed: 3 ins; 0 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/8623.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8623/head:pull/8623 PR: https://git.openjdk.java.net/jdk/pull/8623
Re: RFR: 8285743: Ensure each IntegerPolynomial object is only created once [v2]
On Fri, 29 Apr 2022 22:57:20 GMT, Weijun Wang wrote: >> All `IntegerPolynimial`s are singletons now. Also, hand-coded >> implementations for Ed25519 and Ed448 are removed. They were not used since >> `FieldGen` starts generating classes for them. >> >> No new regression test. This is a clean-up. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > move singleton back into impl and make constructor private It looks good and safe cleanup to me. - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8476
Integrated: 8285516: clearPassword should be called in a finally try block
On Sun, 24 Apr 2022 05:13:36 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > Could I have the simple update reviewed? > > In the PKCS12 key store implementation, the PBEKeySpec.clearPassword() should > be called in a finally try block. Otherwise, the password cleanup could be > interrupted by exceptions. > > Thanks, > Xuelei This pull request has now been integrated. Changeset: 36e4df9d Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/36e4df9d66134ef160bbba0e59d0e3dbb183ba4b Stats: 6 lines in 1 file changed: 2 ins; 2 del; 2 mod 8285516: clearPassword should be called in a finally try block Reviewed-by: mullan, hchao - PR: https://git.openjdk.java.net/jdk/pull/8377
Integrated: 8212136: Remove finalizer implementation in SSLSocketImpl
On Thu, 31 Mar 2022 20:15:35 GMT, Xue-Lei Andrew Fan wrote: > Please review the update to remove finalizer method in the SunJSSE provider > implementation. It is one of the efforts to clean up the use of finalizer > method in JDK. This pull request has now been integrated. Changeset: 034f20fe Author: Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/034f20fe86babb63bf178251a732ac004297cc2d Stats: 39 lines in 1 file changed: 7 ins; 31 del; 1 mod 8212136: Remove finalizer implementation in SSLSocketImpl Reviewed-by: wetmore - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8285516: clearPassword should be called in a finally try block [v2]
On Wed, 4 May 2022 17:35:13 GMT, Weijun Wang wrote: > Please merge your PR with master and I can run it for you. Merged. Thank you! - PR: https://git.openjdk.java.net/jdk/pull/8377
Re: RFR: 8285516: clearPassword should be called in a finally try block [v3]
> Hi, > > Could I have the simple update reviewed? > > In the PKCS12 key store implementation, the PBEKeySpec.clearPassword() should > be called in a finally try block. Otherwise, the password cleanup could be > interrupted by exceptions. > > Thanks, > Xuelei Xue-Lei Andrew Fan has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Merge - an extra whitespace added - 8285516: clearPassword should be called in a finally try block - Changes: - all: https://git.openjdk.java.net/jdk/pull/8377/files - new: https://git.openjdk.java.net/jdk/pull/8377/files/99079d30..1df828df Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8377=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8377=01-02 Stats: 32346 lines in 832 files changed: 22171 ins; 4511 del; 5664 mod Patch: https://git.openjdk.java.net/jdk/pull/8377.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8377/head:pull/8377 PR: https://git.openjdk.java.net/jdk/pull/8377
Re: RFR: 8285516: clearPassword should be called in a finally try block [v2]
On Mon, 25 Apr 2022 14:23:17 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> Could I have the simple update reviewed? >> >> In the PKCS12 key store implementation, the PBEKeySpec.clearPassword() >> should be called in a finally try block. Otherwise, the password cleanup >> could be interrupted by exceptions. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > an extra whitespace added Could someone in Oracle help to run Mach5 testing (tier1 and tier2), just in case unexpected failure happens? - PR: https://git.openjdk.java.net/jdk/pull/8377
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Mon, 2 May 2022 21:42:28 GMT, Valerie Peng wrote:
>>> What kind of additional sentence do you have in mind?
>>
>> It may be fine to put it into the state for 'null" returned value. For
>> example:
>>
>>
>> The returned parameters may be the same that were used to initialize
>> this signature, or may contain additional default or random parameter
>> values used by the underlying signature implementation, or null if the
>> underlying signature implementation does not support returning the
>> parameters as {@code AlgorithmParameters}.
>>
>>
>>
>> The null return conditional in the following sentence may be able to combine
>> together.
>>
>>
>> The returned parameters may be the same that were used to initialize
>> this signature, or may contain additional default or random parameter
>> values used by the underlying signature implementation. {@code null}
>> may be returned if the underlying signature implementation does not
>> support returning the parameters as {@code AlgorithmParameters}, or > conditions>
>
> How about the case when no parameters are given? Say A is the user-supplied
> values, B is the provider specific default or random values, your suggestion
> has A, A+B, and null. Isn't the sentence about B needed (no A and provider
> can generate the parameters)?
I think this sentence covers case B, "... or may contain additional default or
random parameter
values used by the underlying signature implementation."
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v5]
On Tue, 3 May 2022 02:07:13 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > More note update Could someone in Oracle help to run the Mach5 testing for security and network components (or just tier1 and tier2), and let me know if this update cause any failures? - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v14]
On Tue, 3 May 2022 02:20:23 GMT, Xue-Lei Andrew Fan wrote: >> Hi. Sorry, I should have brought this up earlier, but there is a jtreg test >> library to help with ensuring the GC runs, >> `test/lib/jdk/test/lib/util/ForceGC.java`. You might consider replacing the >> test code that runs/checks the GC with `ForceGC.await()`. > >> Hi. Sorry, I should have brought this up earlier, but there is a jtreg test >> library to help with ensuring the GC runs, >> `test/lib/jdk/test/lib/util/ForceGC.java`. You might consider replacing the >> test code that runs/checks the GC with `ForceGC.await()`. > > It looks like a graceful GC utility. There are also some other test cases I > committed that do no use ForceGC. I filed a [new > bug](https://bugs.openjdk.java.net/browse/JDK-8286045), and will make the > update all together. Thank you. > @XueleiFan Did you check to make sure someone ran Mach5 and the results were > ok before integrating? If I'm right, Weijun made the test. Please let me know if there are mach5 testing failures. BTW, it would be nice if Mach5 testing could be supported to run in Github. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v5]
On Tue, 3 May 2022 02:07:13 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > More note update CSR and release note requests are filed in JBS. May I have them reviewed? CSR: https://bugs.openjdk.java.net/browse/JDK-8286064 Release Note: https://bugs.openjdk.java.net/browse/JDK-8286065 - PR: https://git.openjdk.java.net/jdk/pull/8065
Integrated: 8284490: Remove finalizer method in java.security.jgss
On Thu, 7 Apr 2022 04:10:55 GMT, Xue-Lei Andrew Fan wrote: > Please review the update to remove finalizer method in the java.security.jgss > module. It is one of the efforts to clean up the use of finalizer method in > JDK. This pull request has now been integrated. Changeset: ffca23a5 Author: Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/ffca23a5313855a6f9797ad6b342bb2e2cb1b49b Stats: 430 lines in 11 files changed: 393 ins; 12 del; 25 mod 8284490: Remove finalizer method in java.security.jgss Reviewed-by: rriggs, dfuchs, weijun - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v14]
On Mon, 2 May 2022 22:35:08 GMT, Brent Christian wrote: > Hi. Sorry, I should have brought this up earlier, but there is a jtreg test > library to help with ensuring the GC runs, > `test/lib/jdk/test/lib/util/ForceGC.java`. You might consider replacing the > test code that runs/checks the GC with `ForceGC.await()`. It looks like a graceful GC utility. There are also some other test cases I committed that do no use ForceGC. I filed a [new bug](https://bugs.openjdk.java.net/browse/JDK-8286045), and will make the update all together. Thank you. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v3]
On Mon, 2 May 2022 17:55:56 GMT, Bradford Wetmore wrote: >> Thanks for the rewording. Updated. > >> Thanks for the rewording. Updated. > > I made one more tweak that reads better. Yes, it looks better. Updated. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v5]
> Please review the update to remove finalizer method in the SunJSSE provider > implementation. It is one of the efforts to clean up the use of finalizer > method in JDK. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: More note update - Changes: - all: https://git.openjdk.java.net/jdk/pull/8065/files - new: https://git.openjdk.java.net/jdk/pull/8065/files/6130f5e6..8caf85af Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8065=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8065=03-04 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8065/head:pull/8065 PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v3]
On Mon, 2 May 2022 16:46:17 GMT, Bradford Wetmore wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> comment about remove finalize() method > > src/java.base/share/classes/sun/security/ssl/BaseSSLSocketImpl.java line 44: > >> 42: * overridden in SSLSocketImpl. >> 43: * >> 44: * There used to be a finalize() implementation, but decided that was > > Since you haven't pushed yet, maybe: > > There used to be a finalize() implementation that sent close_notify's, > but decided that was not needed. Not closing properly is more properly an > error condition that should be avoided. Applications should close sockets > and not rely on garbage collection. > > The underlying native resources are handled by the Socket Cleaner. Thanks for the rewording. Updated. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v4]
> Please review the update to remove finalizer method in the SunJSSE provider > implementation. It is one of the efforts to clean up the use of finalizer > method in JDK. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: update the note - Changes: - all: https://git.openjdk.java.net/jdk/pull/8065/files - new: https://git.openjdk.java.net/jdk/pull/8065/files/c90e25a1..6130f5e6 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8065=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8065=02-03 Stats: 5 lines in 1 file changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/8065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8065/head:pull/8065 PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v14]
> Please review the update to remove finalizer method in the java.security.jgss > module. It is one of the efforts to clean up the use of finalizer method in > JDK. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: add intermittent test keyword - Changes: - all: https://git.openjdk.java.net/jdk/pull/8136/files - new: https://git.openjdk.java.net/jdk/pull/8136/files/d554c724..e933d404 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8136=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8136=12-13 Stats: 3 lines in 3 files changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8136.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8136/head:pull/8136 PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v3]
On Sat, 30 Apr 2022 03:46:23 GMT, Bradford Wetmore wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> comment about remove finalize() method > > src/java.base/share/classes/sun/security/ssl/BaseSSLSocketImpl.java line 265: > >> 263: } >> 264: >> 265: /** > > Can you please add a quick couple lines here with the technical rationale for > removing it, so we don't forget down the road. > > i.e. > > There used to be a finalize() here, but decided that was not > needed. If users don't properly close the socket... > > The native resources are handled by the Socket Cleaner. It may be not common to comment on non-existing method. Maybe, the class description could be a place to add this note. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Wed, 27 Apr 2022 06:55:42 GMT, Xue-Lei Andrew Fan wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - typo blank linke correction >> - revise the update > > ping ... > @XueleiFan, would you please add a note to the bug itself with the > end-result, and a quick note in the place of the finalizer a quick summary of > what we decided. Sure. Notes were added in JBS and the patch. - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v3]
> Please review the update to remove finalizer method in the SunJSSE provider > implementation. It is one of the efforts to clean up the use of finalizer > method in JDK. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: comment about remove finalize() method - Changes: - all: https://git.openjdk.java.net/jdk/pull/8065/files - new: https://git.openjdk.java.net/jdk/pull/8065/files/ccfc42da..c90e25a1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8065=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8065=01-02 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8065.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8065/head:pull/8065 PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Thu, 28 Apr 2022 23:09:00 GMT, Valerie Peng wrote:
> What kind of additional sentence do you have in mind?
It may be fine to put it into the state for 'null" returned value. For example:
The returned parameters may be the same that were used to initialize
this signature, or may contain additional default or random parameter
values used by the underlying signature implementation, **or null if the
underlying signature implementation does not support returning the
parameters as {@code AlgorithmParameters}.**
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v12]
On Thu, 28 Apr 2022 04:34:36 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the >> java.security.jgss module. It is one of the efforts to clean up the use of >> finalizer method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > change to use othervm The Thread.sleep() was added back. Without the sleep, the test may fail intermittent. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v13]
> Please review the update to remove finalizer method in the java.security.jgss > module. It is one of the efforts to clean up the use of finalizer method in > JDK. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: add sleep back - Changes: - all: https://git.openjdk.java.net/jdk/pull/8136/files - new: https://git.openjdk.java.net/jdk/pull/8136/files/c0890841..d554c724 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8136=12 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8136=11-12 Stats: 2 lines in 2 files changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8136.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8136/head:pull/8136 PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
On Thu, 28 Apr 2022 07:01:25 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java test > case failed on one of the test setups. The test runs gc in a loop and > expects the GC to have garbage collected contents of a WeakHashMap. The loop > runs for 10 iterations. Some delay needs to be added between each iteration > to increase the chances of GC garbage collecting the instances. > > Thanks, > Xuelei > Alternatively, the loop count could be raised by 10x. That would keep the > typical running time low and still allow for a worst case. Update: I tried to run the test 1000 times. I still can see failure if using 10x loop count, without sleep. - PR: https://git.openjdk.java.net/jdk/pull/8443
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v12]
On Thu, 28 Apr 2022 17:48:20 GMT, Weijun Wang wrote: > I see you removed the `Thread.sleep(100)` calls. Given the failure of another > similar test, maybe it's safer to add them back? Yes. I'm evaluating if other proposal works or not. Otherwise, I will add the sleep back. - PR: https://git.openjdk.java.net/jdk/pull/8136
Integrated: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
On Thu, 28 Apr 2022 07:01:25 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java test > case failed on one of the test setups. The test runs gc in a loop and > expects the GC to have garbage collected contents of a WeakHashMap. The loop > runs for 10 iterations. Some delay needs to be added between each iteration > to increase the chances of GC garbage collecting the instances. > > Thanks, > Xuelei This pull request has now been integrated. Changeset: b9d1e851 Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/b9d1e85151d9d4016639e6298c90737db10f6072 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8285785: CheckCleanerBound test fails with PasswordCallback object is not released Reviewed-by: dfuchs, mullan, rriggs - PR: https://git.openjdk.java.net/jdk/pull/8443
Re: RFR: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
On Thu, 28 Apr 2022 13:34:04 GMT, Roger Riggs wrote: >> Hi, >> >> May I have this test update reviewed? >> >> The javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java >> test case failed on one of the test setups. The test runs gc in a loop and >> expects the GC to have garbage collected contents of a WeakHashMap. The loop >> runs for 10 iterations. Some delay needs to be added between each iteration >> to increase the chances of GC garbage collecting the instances. >> >> Thanks, >> Xuelei > > Marked as reviewed by rriggs (Reviewer). I will check the proposal suggested by @RogerRiggs and @dfuch. As the test failure could be annoying, I would like to integrate this update first. - PR: https://git.openjdk.java.net/jdk/pull/8443
RFR: 8285785: CheckCleanerBound test fails with PasswordCallback object is not released
Hi, May I have this test update reviewed? The javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java test case failed on one of the test setups. The test runs gc in a loop and expects the GC to have garbage collected contents of a WeakHashMap. The loop runs for 10 iterations. Some delay needs to be added between each iteration to increase the chances of GC garbage collecting the instances. Thanks, Xuelei - Commit messages: - 8285785: CheckCleanerBound test fails with PasswordCallback object is not released Changes: https://git.openjdk.java.net/jdk/pull/8443/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8443=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285785 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8443.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8443/head:pull/8443 PR: https://git.openjdk.java.net/jdk/pull/8443
Re: RFR: 8284910: Buffer clean in PasswordCallback [v9]
On Thu, 28 Apr 2022 06:31:30 GMT, Jaikiran Pai wrote: > More of a FYI - the CheckCleanerBound test failed on one of the test setups. > So I've created https://bugs.openjdk.java.net/browse/JDK-8285785 to track > that failure. Thank you! I will add the sleep back. - PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Thu, 28 Apr 2022 04:44:43 GMT, Xue-Lei Andrew Fan wrote:
>>> > What does it refer to with 'it'? Is 'it' refer to the implementation
>>> > generated parameter values?
>>>
>>> 'It' refers to the parameters containing all of the parameter values
>>> including the supplied ones and provider-generated ones if any.
>>
>> The full sentence is, "If the required parameters were not supplied and the
>> underlying signature implementation can generate the parameter values, it
>> will be returned." As there is no supplied value, I think 'it' refer to
>> the provider-generated ones if any. As the previous noun is "the parameters
>> values", I'm not sure if the use of 'it' here is properly.
>
>> Can you clarify what is the A and B that you are referring to?
>
> The sentence is, “If the required parameters were not supplied and the
> underlying signature implementation can generate the parameter values, it
> will be returned. Otherwise, {https://github.com/code null} is returned."
>
> I read "the required parameters were not supplied" as condition A; and "the
> underlying signature implementation can generate the parameter values" as
> condition B.
> With Signature class, there is a caveat for EdDSA, the supplied parameters
> are set but null is being returned when getParameters() is called. This is
> currently covered by the condition `if the underlying signature
> implementation supports returning the parameters as {@code
> AlgorithmParameters}` as the underlying signature does not support
> AlgorithmParameters for the supplied EdDSAParameterSpec object due to lack of
> ASN.1 definition.
I see now. My 1st read of the condition, I did not get the point and thought
it is not necessary as the method is trying to return "AlgorithmParameters".
Could it be more clear if describe this case in an additional sentence?
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Wed, 27 Apr 2022 23:35:19 GMT, Valerie Peng wrote:
>> With Signature class, there is a caveat for EdDSA, the supplied parameters
>> are set but null is being returned when getParameters() is called. This is
>> currently covered by the condition `if the underlying signature
>> implementation supports returning the parameters as {@code
>> AlgorithmParameters}` as the underlying signature does not support
>> AlgorithmParameters for the supplied EdDSAParameterSpec object due to lack
>> of ASN.1 definition.
>
> Besides this Signature-specific condition, there is the common condition
> where provider cannot (or do not) generate default parameter values. {@code
> null} is used as the catch-all result, but as you said, describe various
> conditions tersely and correctly is key.
> > What does it refer to with 'it'? Is 'it' refer to the implementation
> > generated parameter values?
>
> 'It' refers to the parameters containing all of the parameter values
> including the supplied ones and provider-generated ones if any.
The full sentence is, "If the required parameters were not supplied and the
underlying signature implementation can generate the parameter values, it will
be returned." As there is no supplied value, I think 'it' refer to the
provider-generated ones if any. As the previous noun is "the parameters
values", I'm not sure if the use of 'it' here is properly.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Thu, 28 Apr 2022 04:41:20 GMT, Xue-Lei Andrew Fan wrote:
>> Besides this Signature-specific condition, there is the common condition
>> where provider cannot (or do not) generate default parameter values. {@code
>> null} is used as the catch-all result, but as you said, describe various
>> conditions tersely and correctly is key.
>
>> > What does it refer to with 'it'? Is 'it' refer to the implementation
>> > generated parameter values?
>>
>> 'It' refers to the parameters containing all of the parameter values
>> including the supplied ones and provider-generated ones if any.
>
> The full sentence is, "If the required parameters were not supplied and the
> underlying signature implementation can generate the parameter values, it
> will be returned." As there is no supplied value, I think 'it' refer to the
> provider-generated ones if any. As the previous noun is "the parameters
> values", I'm not sure if the use of 'it' here is properly.
> Can you clarify what is the A and B that you are referring to?
The sentence is, “If the required parameters were not supplied and the
underlying signature implementation can generate the parameter values, it will
be returned. Otherwise, {https://github.com/code null} is returned."
I read "the required parameters were not supplied" as condition A; and "the
underlying signature implementation can generate the parameter values" as
condition B.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v11]
On Wed, 27 Apr 2022 22:56:08 GMT, Weijun Wang wrote: > please change them to use `othervm`. Thanks for the catch. Updated to use othervm. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v12]
> Please review the update to remove finalizer method in the java.security.jgss > module. It is one of the efforts to clean up the use of finalizer method in > JDK. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: change to use othervm - Changes: - all: https://git.openjdk.java.net/jdk/pull/8136/files - new: https://git.openjdk.java.net/jdk/pull/8136/files/35599747..c0890841 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8136=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8136=10-11 Stats: 4 lines in 2 files changed: 2 ins; 2 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8136.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8136/head:pull/8136 PR: https://git.openjdk.java.net/jdk/pull/8136
Integrated: 8284910: Buffer clean in PasswordCallback
On Sat, 16 Apr 2022 15:45:21 GMT, Xue-Lei Andrew Fan wrote: > Please review this password cleanup enhancement in the PasswordCallback > implementation. This is one of the effort to clean up the buffered passwords. > > The PasswordCallback.setPassword() clones the password, but is not registered > for cleanup. An application could call clearPassword() for the purpose, but > it would be nice to cleanup the buffer as well if clearPassword() was not > called in an application. And, if the setPassword() get called multiple > times, the clearPassword() should also be called the same times if not > relying on finalization. It could be fragile in practice. This pull request has now been integrated. Changeset: 89fd6d34 Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/89fd6d34f859d61d9cf5a1edf9419eee7c338390 Stats: 147 lines in 3 files changed: 141 ins; 0 del; 6 mod 8284910: Buffer clean in PasswordCallback Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v10]
On Wed, 27 Apr 2022 17:25:52 GMT, Weijun Wang wrote: > I see you are still using the 1st version of the `Cleaners.java` test which > runs on Windows. Please update to the current version (I've updated the code > in place). Oops, I missed it. Updated. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v11]
> Please review the update to remove finalizer method in the java.security.jgss > module. It is one of the efforts to clean up the use of finalizer method in > JDK. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: renew the Cleaners.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/8136/files - new: https://git.openjdk.java.net/jdk/pull/8136/files/dbbb5d53..35599747 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8136=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8136=09-10 Stats: 10 lines in 1 file changed: 2 ins; 3 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8136.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8136/head:pull/8136 PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284910: Buffer clean in PasswordCallback [v9]
> Please review this password cleanup enhancement in the PasswordCallback > implementation. This is one of the effort to clean up the buffered passwords. > > The PasswordCallback.setPassword() clones the password, but is not registered > for cleanup. An application could call clearPassword() for the purpose, but > it would be nice to cleanup the buffer as well if clearPassword() was not > called in an application. And, if the setPassword() get called multiple > times, the clearPassword() should also be called the same times if not > relying on finalization. It could be fragile in practice. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: no sleep for waiting cleanup - Changes: - all: https://git.openjdk.java.net/jdk/pull/8272/files - new: https://git.openjdk.java.net/jdk/pull/8272/files/6b07617e..fe4698a3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8272=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8272=07-08 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8272.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8272/head:pull/8272 PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284910: Buffer clean in PasswordCallback [v8]
On Wed, 27 Apr 2022 16:02:04 GMT, Roger Riggs wrote:
>> Xue-Lei Andrew Fan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> remove trailing whitespace
>
> test/jdk/javax/security/auth/callback/PasswordCallback/CheckCleanerBound.java
> line 50:
>
>> 48: for (int i = 0; i < 10 && weakHashMap.size() != 0; i++) {
>> 49: System.gc();
>> 50: Thread.sleep(100);
>
> You can drop this sleep to 10ms to cut the average test time. It might be
> interesting to know how many retries are typical.
Hm, it looks like a good idea to improve the testing performance (code
updated). The cleaner get called in the 1st GC collection on my local laptop.
-
PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8285696: AlgorithmConstraints:permits not throwing IllegalArgumentException when 'alg' is null [v2]
On Wed, 27 Apr 2022 16:16:18 GMT, Daniel Jeliński wrote: >> Please review this follow up to #8349. >> >> As JCK pointed out, `permits` is supposed to throw IAE on null input. >> However, now that we're looking up the result in a `ConcurrentHashMap`, a >> `NullPointerException` is thrown. This patch restores the original behavior. >> >> Verified that the JCK test passes with this change. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Move check to permits method Looks good to me. Thanks! - Marked as reviewed by xuelei (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8427
Re: RFR: 8284910: Buffer clean in PasswordCallback [v8]
> Please review this password cleanup enhancement in the PasswordCallback > implementation. This is one of the effort to clean up the buffered passwords. > > The PasswordCallback.setPassword() clones the password, but is not registered > for cleanup. An application could call clearPassword() for the purpose, but > it would be nice to cleanup the buffer as well if clearPassword() was not > called in an application. And, if the setPassword() get called multiple > times, the clearPassword() should also be called the same times if not > relying on finalization. It could be fragile in practice. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: remove trailing whitespace - Changes: - all: https://git.openjdk.java.net/jdk/pull/8272/files - new: https://git.openjdk.java.net/jdk/pull/8272/files/7acd0ca8..6b07617e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8272=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8272=06-07 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8272.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8272/head:pull/8272 PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8285696: AlgorithmConstraints:permits not throwing IllegalArgumentException when 'alg' is null
On Wed, 27 Apr 2022 14:03:15 GMT, Daniel Jeliński wrote: > Please review this follow up to #8349. > > As JCK pointed out, `permits` is supposed to throw IAE on null input. > However, now that we're looking up the result in a `ConcurrentHashMap`, a > `NullPointerException` is thrown. This patch restores the original behavior. > > Verified that the JCK test passes with this change. Maybe, the checking could be placed in permits() method (line 158-173) so that it follows the spec, and easier to check. - PR: https://git.openjdk.java.net/jdk/pull/8427
Re: RFR: 8284910: Buffer clean in PasswordCallback [v6]
On Wed, 27 Apr 2022 13:44:14 GMT, Sean Mullan wrote: >> Xue-Lei Andrew Fan has updated the pull request incrementally with one >> additional commit since the last revision: >> >> rename and split the test case > > test/jdk/javax/security/auth/callback/PasswordCallback/PasswordCleanup.java > line 55: > >> 53: } >> 54: >> 55: // Check if the PasswordCallback object could be collected. > > Since you are already checking if the Cleaner works properly in the > `CheckCleanerBound` test, I don't see a reason why you need to test that > again. It's a fair point. Since the cleaner test is pretty slow, I'm going to remove that part in this test. - PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284910: Buffer clean in PasswordCallback [v7]
> Please review this password cleanup enhancement in the PasswordCallback > implementation. This is one of the effort to clean up the buffered passwords. > > The PasswordCallback.setPassword() clones the password, but is not registered > for cleanup. An application could call clearPassword() for the purpose, but > it would be nice to cleanup the buffer as well if clearPassword() was not > called in an application. And, if the setPassword() get called multiple > times, the clearPassword() should also be called the same times if not > relying on finalization. It could be fragile in practice. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: Remove duplicated test case - Changes: - all: https://git.openjdk.java.net/jdk/pull/8272/files - new: https://git.openjdk.java.net/jdk/pull/8272/files/9d38fdd3..7acd0ca8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8272=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8272=05-06 Stats: 23 lines in 1 file changed: 0 ins; 22 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8272.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8272/head:pull/8272 PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8285493: ECC calculation error
On Wed, 27 Apr 2022 12:57:20 GMT, Weijun Wang wrote: >> src/jdk.crypto.ec/share/classes/sun/security/ec/ECDSAOperations.java line >> 261: >> >>> 259: IntegerModuloP result = p1.asAffine().getX(); >>> 260: b2a(result, orderField, temp1); >>> 261: return MessageDigest.isEqual(temp1, r); >> >> I did not get the point of this update. Is it the broken case you mentioned >> in the PR description? What's the issue of the original code? > > Here, `result`'s modulus is `field`, and `ri`'s is `orderField`. Therefore > you cannot simply subtract one from the other. One new `assert` would fail. Got it. It looks like a safe update. - PR: https://git.openjdk.java.net/jdk/pull/8409
Re: RFR: 8285493: ECC calculation error
On Tue, 26 Apr 2022 21:02:49 GMT, Weijun Wang wrote: > Only numbers from the same modular fields can be involved in arithmetic > calculations. Add `assert` to guarantee this. > > Also, found one broken case and rewrote it. Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8409
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v10]
> Please review the update to remove finalizer method in the java.security.jgss > module. It is one of the efforts to clean up the use of finalizer method in > JDK. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: Correct test failure - Changes: - all: https://git.openjdk.java.net/jdk/pull/8136/files - new: https://git.openjdk.java.net/jdk/pull/8136/files/32e0a469..dbbb5d53 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8136=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8136=08-09 Stats: 12 lines in 1 file changed: 9 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8136.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8136/head:pull/8136 PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v8]
On Wed, 27 Apr 2022 07:11:16 GMT, Xue-Lei Andrew Fan wrote: > Can you try out this test? Awesome! Thank you! - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v8]
On Tue, 26 Apr 2022 15:43:09 GMT, Weijun Wang wrote:
> Can you try out this test?
>
> ```
> diff --git a/test/jdk/sun/security/krb5/auto/Cleaners.java
> b/test/jdk/sun/security/krb5/auto/Cleaners.java
> new file mode 100644
> index 000..43f06cb9f60
> --- /dev/null
> +++ b/test/jdk/sun/security/krb5/auto/Cleaners.java
> @@ -0,0 +1,179 @@
> +/*
> + * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 only, as
> + * published by the Free Software Foundation.
> + *
> + * This code is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * version 2 for more details (a copy is included in the LICENSE file that
> + * accompanied this code).
> + *
> + * You should have received a copy of the GNU General Public License version
> + * 2 along with this work; if not, write to the Free Software Foundation,
> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
> + * or visit www.oracle.com if you need additional information or have any
> + * questions.
> + */
> +
> +/*
> + * @test
> + * @bug 8284490
> + * @summary Remove finalizer method in java.security.jgss
> + * @requires os.family != "windows"
> + * @library /test/lib
> + * @compile -XDignore.symbol.file Cleaners.java
> + * @run main/othervm Cleaners launcher
> + */
> +
> +import java.nio.charset.StandardCharsets;
> +import java.nio.file.Files;
> +import java.nio.file.Paths;
> +import java.nio.file.attribute.PosixFilePermission;
> +import java.util.Arrays;
> +import java.util.Set;
> +
> +import jdk.test.lib.Asserts;
> +import jdk.test.lib.process.Proc;
> +import org.ietf.jgss.Oid;
> +import sun.security.krb5.Config;
> +
> +public class Cleaners {
> +
> +private static final String CONF = "krb5.conf";
> +private static final String KTAB_S = "server.ktab";
> +private static final String KTAB_B = "backend.ktab";
> +
> +private static final String HOST = "localhost";
> +private static final String SERVER = "server/" + HOST;
> +private static final String BACKEND = "backend/" + HOST;
> +private static final String USER = "user";
> +private static final char[] PASS = "password".toCharArray();
> +private static final String REALM = "REALM";
> +
> +private static final byte[] MSG = "12345678".repeat(128)
> +.getBytes(StandardCharsets.UTF_8);
> +
> +public static void main(String[] args) throws Exception {
> +
> +Oid oid = new Oid("1.2.840.113554.1.2.2");
> +byte[] token, msg;
> +
> +switch (args[0]) {
> +case "launcher" -> {
> +KDC kdc = KDC.create(REALM, HOST, 0, true);
> +kdc.addPrincipal(USER, PASS);
> +kdc.addPrincipalRandKey("krbtgt/" + REALM);
> +kdc.addPrincipalRandKey(SERVER);
> +kdc.addPrincipalRandKey(BACKEND);
> +
> +// Native lib might do some name lookup
> +KDC.saveConfig(CONF, kdc,
> +"dns_lookup_kdc = no",
> +"ticket_lifetime = 1h",
> +"dns_lookup_realm = no",
> +"dns_canonicalize_hostname = false",
> +"forwardable = true");
> +System.setProperty("java.security.krb5.conf", CONF);
> +Config.refresh();
> +
> +// Create kaytab and ccache files for native clients
> +kdc.writeKtab(KTAB_S, false, SERVER);
> +kdc.writeKtab(KTAB_B, false, BACKEND);
> +kdc.kinit(USER, "ccache");
> +Files.setPosixFilePermissions(Paths.get("ccache"),
> +Set.of(PosixFilePermission.OWNER_READ,
> +PosixFilePermission.OWNER_WRITE));
> +
> +Proc pc = proc("client")
> +.env("KRB5CCNAME", "FILE:ccache")
> +.env("KRB5_KTNAME", "none") // Do not try system
> ktab if ccache fails
> +.start();
> +Proc ps = proc("server")
> +.env("KRB5_KTNAME", KTAB_S)
> +.start();
> +Proc pb = proc("backend")
> +.env("KRB5_KTNAME", KTAB_B)
> +.start();
> +
> +// Client and server
> +ps.println(pc.readData()); // AP-REQ
> +pc.println(ps.readData()); // AP-REP, mutual auth
> +ps.println(pc.readData()); // wrap msg
> +ps.println(pc.readData());
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v9]
> Please review the update to remove finalizer method in the java.security.jgss > module. It is one of the efforts to clean up the use of finalizer method in > JDK. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: final pName and new test case - Changes: - all: https://git.openjdk.java.net/jdk/pull/8136/files - new: https://git.openjdk.java.net/jdk/pull/8136/files/69fe16d0..32e0a469 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8136=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8136=07-08 Stats: 199 lines in 3 files changed: 195 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8136.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8136/head:pull/8136 PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan wrote: >> Please review the update to remove finalizer method in the SunJSSE provider >> implementation. It is one of the efforts to clean up the use of finalizer >> method in JDK. > > Xue-Lei Andrew Fan has updated the pull request incrementally with two > additional commits since the last revision: > > - typo blank linke correction > - revise the update ping ... - PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8225433: Clarify behavior of PKIXParameters.setRevocationEnabled when PKIXRevocationChecker is used
On Mon, 18 Apr 2022 13:35:25 GMT, Sean Mullan wrote:
> This change improves the specification for the case when a
> `PKIXRevocationChecker` is supplied as one of the `CertPathChecker`
> parameters. Specifically, it makes it more clear that a
> `PKIXRevocationChecker` overrides the default revocation checking mechanism
> of a PKIX service provider, and will be used to check revocation irrespective
> of the setting of the RevocationEnabled parameter.
>
> Will also file a CSR.
Looks good to me, except a minor nit.
src/java.base/share/classes/java/security/cert/PKIXParameters.java line 339:
> 337: * #setCertPathCheckers setCertPathCheckers} methods).
> 338: *
> 339: * However, if a {@code PKIXRevocationChecker} is passed in as a
> parameter
The word "However" may be not necessary as the previous paragraph is ending
with a substitute mechanism. This sentence could be a further explanation of
the substitute mechanism.
-
Marked as reviewed by xuelei (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8287
Re: RFR: 8285493: ECC calculation error
On Tue, 26 Apr 2022 21:02:49 GMT, Weijun Wang wrote: > Only numbers from the same modular fields can be involved in arithmetic > calculations. Add `assert` to guarantee this. > > Also, found one broken case and rewrote it. src/jdk.crypto.ec/share/classes/sun/security/ec/ECDSAOperations.java line 261: > 259: IntegerModuloP result = p1.asAffine().getX(); > 260: b2a(result, orderField, temp1); > 261: return MessageDigest.isEqual(temp1, r); I did not get the point of this update. Is it the broken case you mentioned in the PR description? What's the issue of the original code? src/jdk.crypto.ec/share/classes/sun/security/ec/ECDSAOperations.java line 261: > 259: IntegerModuloP result = p1.asAffine().getX(); > 260: b2a(result, orderField, temp1); > 261: return MessageDigest.isEqual(temp1, r); I did not get the point of this update. Is it the broken case you mentioned in the PR description? What's the issue of the original code? - PR: https://git.openjdk.java.net/jdk/pull/8409
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v2]
On Tue, 26 Apr 2022 17:51:45 GMT, Valerie Peng wrote:
>> This is to update the method javadoc of
>> java.security.Signature.getParameters() with the missing `@throws
>> UnsupportedOperationException`. In addition, the wording on the returned
>> parameters are updated to match those in Cipher and CipherSpi classes.
>>
>> CSR will be filed later.
>>
>> Thanks,
>> Valerie
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Undo un-intentional changes.
src/java.base/share/classes/java/security/Signature.java line 1012:
> 1010: * values used by the underlying signature implementation if the
> underlying
> 1011: * signature implementation supports returning the parameters as
> 1012: * {@code AlgorithmParameters}. If the required
"... if the underlying signature implementation supports returning the
parameters a {@code AlgorithmParameters}", it looks this part is duplicated and
may be removed.
src/java.base/share/classes/java/security/Signature.java line 1014:
> 1012: * {@code AlgorithmParameters}. If the required
> 1013: * parameters were not supplied and the underlying signature
> implementation
> 1014: * can generate the parameter values, it will be returned.
> Otherwise,
> If the required parameters were not supplied and the underlying signature
> implementation can generate the parameter values, it will be returned.
What does it refer to with 'it'? Is 'it' refer to the implementation generated
parameter values?
> If the required parameters were not supplied and the underlying signature
> implementation can generate the parameter values, it will be returned.
> Otherwise, {@code null} is returned.
The logic looks like
if (A & B) {
// it will be returned
} else {
// {@code null} is returned.
}
If I read it correctly, the behavior may look like:
1. If the required parameters were supplied, {@code null} is returned; (if !A)
2. if the underlying signature implementation cannot generate the parameter
values, {@code null} is returned; (if !B)
3. if not 1 and 2, ... (if A & B)
It does not look like right. The expected behavior may be:
if (A) {
if (B) {
// it will be returned
} else {
// {@code null} is returned.
}
}
Maybe, the confusion could be mitigated by re-org the logic:
if (A & !B) {
// {@code null} is returned.
} // Otherwise, refer to 1st sentence.
"The returned parameters may be the same that were used to initialize this
signature, or may contain additional default or random parameter values used by
the underlying signature implementation. {@code null} is returned if the
required parameters were not supplied and the underlying signature
implementation cannot generate the parameter values."
Similar comment to [PR 8117](https://github.com/openjdk/jdk/pull/8117), if you
want to use similar description there as well.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8285683: Missing @ since 11 in java.security.spec.MGF1ParameterSpec fields
On Tue, 26 Apr 2022 22:55:29 GMT, Bradford Wetmore wrote: > Two new constant fields `MGF1ParameterSpec.SHA512_224` and > `MGF1ParameterSpec.SHA512_256` didn't have `@since 11` tag added as part of > [JDK-8146293](https://bugs.openjdk.java.net/browse/JDK-8146293). > > This bug addresses this issue. Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8411
Re: RFR: 8284910: Buffer clean in PasswordCallback [v4]
On Tue, 26 Apr 2022 15:45:47 GMT, Xue-Lei Andrew Fan wrote: >> Ok, then I would suggest changing the name of the test as it is misleading. >> I suggest creating a directory named "PasswordCallback" and then adding a >> test named perhaps "CheckCleanerNotBoundToThis" or something like that. I >> would change the name of the `checkClearing` method as you are not checking >> if passwords are cleared. Also update the @summary to describe what it is >> actually testing. Use code comments if you need to explain it further. > > The test has two case now. One is used to check the PasswordCallback object > collection at finalization. The other one is check the password clearing for > clearPassword() method. Is it better to split into two test files so that > the class name could be better fit the purpose? Never mind, I'm splitting the test case into two. - PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284910: Buffer clean in PasswordCallback [v6]
> Please review this password cleanup enhancement in the PasswordCallback > implementation. This is one of the effort to clean up the buffered passwords. > > The PasswordCallback.setPassword() clones the password, but is not registered > for cleanup. An application could call clearPassword() for the purpose, but > it would be nice to cleanup the buffer as well if clearPassword() was not > called in an application. And, if the setPassword() get called multiple > times, the clearPassword() should also be called the same times if not > relying on finalization. It could be fragile in practice. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: rename and split the test case - Changes: - all: https://git.openjdk.java.net/jdk/pull/8272/files - new: https://git.openjdk.java.net/jdk/pull/8272/files/aaedee46..9d38fdd3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8272=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8272=04-05 Stats: 233 lines in 3 files changed: 136 ins; 97 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8272.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8272/head:pull/8272 PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284910: Buffer clean in PasswordCallback [v4]
On Tue, 26 Apr 2022 15:19:30 GMT, Sean Mullan wrote: >> The test case is used to check that the Cleaner used is not bind to 'this' >> object, and the cleaner during finalization could work. Unfortunately, as >> the cleaner behavior is not visible, I don't find a way to automated test >> that the password is really cleared during finalization. > > Ok, then I would suggest changing the name of the test as it is misleading. I > suggest creating a directory named "PasswordCallback" and then adding a test > named perhaps "CheckCleanerNotBoundToThis" or something like that. I would > change the name of the `checkClearing` method as you are not checking if > passwords are cleared. Also update the @summary to describe what it is > actually testing. Use code comments if you need to explain it further. The test has two case now. One is used to check the PasswordCallback object collection at finalization. The other one is check the password clearing for clearPassword() method. Is it better to split into two test files so that the class name could be better fit the purpose? - PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v8]
On Tue, 26 Apr 2022 11:54:36 GMT, Weijun Wang wrote: >> Did you mean pName? The dispose() method will reset it to zero. 'pName" is >> used a lot in native implementation. It may be doable to make it final, but >> it may be more complicated than I could expect. I would like to leave it as >> it is in this PR. > > IMO, there's no need to reset it to zero in `dispose()`. As for the usage in > native implementation, if there is really a case that it gets updated, then > the cleanable you registered at the beginning will be useless. Isn't that a > real problem? The native code does not update pName. As dispose is public, if pName is set to zero, I'm not sure if there is compatibility issues if more methods get called after dispose(). If you are confident with that, I could set pName to final and update the dispose() impl. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v8]
On Tue, 26 Apr 2022 02:02:09 GMT, Weijun Wang wrote: > Where is a test for `GSSCredential`? I did not find a way to create a GSSCredential object successfully, exception thrown. Is there an example I can refer to? - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v8]
On Tue, 26 Apr 2022 01:53:43 GMT, Weijun Wang wrote: >> Xue-Lei Andrew Fan has updated the pull request with a new target base due >> to a merge or a rebase. The pull request now contains 10 commits: >> >> - Merge and resovle merge conflict >> - change the calling order in dispose() >> - More code cleanup >> - re-org the code per feedback >> - Update to set context method >> - add test cases >> - Merge >> - Update copyright year >> - the object reference issue for Cleaner >> - 8284490: Remove finalizer method in java.security.jgss > > src/java.security.jgss/share/classes/sun/security/jgss/wrapper/GSSNameElement.java > line 54: > >> 52: private final Cleaner.Cleanable cleanable; >> 53: >> 54: long pName = 0; // Pointer to the gss_name_t structure > > Can this be final? Did you mean pName? The dispose() method will reset it to zero. 'pName" is used a lot in native implementation. It may be doable to make it final, but it may be more complicated than I could expect. I would like to leave it as it is in this PR. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284910: Buffer clean in PasswordCallback [v5]
> Please review this password cleanup enhancement in the PasswordCallback > implementation. This is one of the effort to clean up the buffered passwords. > > The PasswordCallback.setPassword() clones the password, but is not registered > for cleanup. An application could call clearPassword() for the purpose, but > it would be nice to cleanup the buffer as well if clearPassword() was not > called in an application. And, if the setPassword() get called multiple > times, the clearPassword() should also be called the same times if not > relying on finalization. It could be fragile in practice. Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: correct test typo and test clearPassword() - Changes: - all: https://git.openjdk.java.net/jdk/pull/8272/files - new: https://git.openjdk.java.net/jdk/pull/8272/files/01542bb6..aaedee46 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8272=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8272=03-04 Stats: 12 lines in 1 file changed: 10 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8272.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8272/head:pull/8272 PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284910: Buffer clean in PasswordCallback [v4]
On Mon, 25 Apr 2022 20:41:47 GMT, Sean Mullan wrote:
>> Xue-Lei Andrew Fan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Code clean up per feedback
>
> test/jdk/javax/security/auth/callback/PasswordCleanup.java line 58:
>
>> 56: }
>> 57:
>> 58: private static void clearWithMethod() throws Exception {
>
> This looks like the exact same test as `clearAtCollection`.
Nice catch. The passwordCallback.clearPassword() should not be called in
clearAtCollection.
> test/jdk/javax/security/auth/callback/PasswordCleanup.java line 74:
>
>> 72: }
>> 73:
>> 74: private static void checkClearing() throws Exception {
>
> How is this test testing that the password is cleared?
The test case is used to check that the Cleaner used is not bind to 'this'
object, and the cleaner during finalization could work. Unfortunately, as the
cleaner behavior is not visible, I don't find a way to automated test that the
password is really cleared during finalization.
-
PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284910: Buffer clean in PasswordCallback [v4]
On Mon, 25 Apr 2022 20:37:38 GMT, Sean Mullan wrote:
>> Xue-Lei Andrew Fan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Code clean up per feedback
>
> test/jdk/javax/security/auth/callback/PasswordCleanup.java line 83:
>
>> 81: // Check if the object has been collected.
>> 82: if (weakHashMap.size() > 0) {
>> 83: throw new RuntimeException("GSSName object is not released");
>
> Did you mean to say "PasswordCallback object is not released"?
Ooops, bad copy and past of mine.
-
PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8285398: Cache the results of constraint checks
On Thu, 21 Apr 2022 19:58:39 GMT, Daniel Jeliński wrote: > Profiling the TLS handshakes using SSLHandshake benchmark shows that a large > portion of time is spent in HandshakeContext initialization, specifically in > DisabledAlgorithmConstraints class. > > There are only a few instances of that class, and they are immutable. Caching > the results should be a low-risk operation. > > The cache is implemented as a softly reachable ConcurrentHashMap; this way it > can be removed from memory after a period of inactivity. Under normal > circumstances the cache holds no more than 100 algorithms. Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8349
Re: RFR: 8285398: Cache the results of constraint checks
On Mon, 25 Apr 2022 16:04:22 GMT, Anthony Scarpino wrote: > It also shows that more caching probably would help further. +1. - PR: https://git.openjdk.java.net/jdk/pull/8349
Re: RFR: 8285398: Cache the results of constraint checks
On Mon, 25 Apr 2022 17:22:57 GMT, Daniel Jeliński wrote: >>> With all the above in mind I decided not to use `sun.security.util.Cache` >>> here >> >> I was not meant to use Cache and timeout for this update. >> >> SoftReference and this patch should work good in larger memory boxes. >> Performance improvement sometimes is a trade-off game between memory and >> CPU. Did you have a chance to check the performance in the limited memory >> circumstance? like '-mx64M". > > I run a few tests: > `-Xmx16m`, with this patch, still better than the original version: > > Benchmark (resume) (tlsVersion) Mode Cnt Score > Error Units > SSLHandshake.doHandshake true TLSv1.2 thrpt5 4477.444 ± > 375.975 ops/s > SSLHandshake.doHandshake true TLS thrpt5 681.471 ± > 72.531 ops/s > SSLHandshake.doHandshake false TLSv1.2 thrpt5 335.366 ± > 89.056 ops/s > SSLHandshake.doHandshake false TLS thrpt5 308.711 ± > 90.134 ops/s > > > `-Xmx8m`, before patch: > > Benchmark (resume) (tlsVersion) Mode CntScore > Error Units > SSLHandshake.doHandshake true TLSv1.2 thrpt5 153.760 ± > 12.025 ops/s > SSLHandshake.doHandshake true TLS thrpt5 70.700 ± > 7.506 ops/s > SSLHandshake.doHandshake false TLSv1.2 thrpt5 67.459 ± > 4.325 ops/s > SSLHandshake.doHandshake false TLS thrpt5 64.695 ± > 1.647 ops/s > > after: > > Benchmark (resume) (tlsVersion) Mode CntScore > Error Units > SSLHandshake.doHandshake true TLSv1.2 thrpt5 557.324 ± > 50.269 ops/s > SSLHandshake.doHandshake true TLS thrpt5 155.258 ± > 13.866 ops/s > SSLHandshake.doHandshake false TLSv1.2 thrpt5 181.755 ± > 29.319 ops/s > SSLHandshake.doHandshake false TLS thrpt5 120.627 ± > 25.832 ops/s > > Much slower, but still faster with the patch. > With `-Xmx4m` the test failed with OOM. Thanks for the additional testing. The improvement is impressive. - PR: https://git.openjdk.java.net/jdk/pull/8349
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v7]
On Fri, 22 Apr 2022 13:27:12 GMT, Daniel Fuchs wrote: > Please get another reviewer for the security-libs related and native changes. @wangweij Did you have cycle and have another look at the update? - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8285398: Cache the results of constraint checks
On Mon, 25 Apr 2022 13:59:44 GMT, Daniel Jeliński wrote: >> FWIW: I wouldn't expect `SoftReference` (as opposed to `WeakReference`) to >> be eagerly cleaned. > > `SoftReference`s are guaranteed to survive one GC after use; beyond that > their lifespan is determined by `SoftRefLRUPolicyMSPerMB` and the amount of > memory available. > With all the above in mind I decided not to use `sun.security.util.Cache` here I was not meant to use Cache and timeout for this update. SoftReference and this patch should work good in larger memory boxes. Performance improvement sometimes is a trade-off game between memory and CPU. Did you have a chance to check the performance in the limited memory circumstance? like '-mx64M". - PR: https://git.openjdk.java.net/jdk/pull/8349
Re: RFR: 8285516: clearPassword should be called in a finally try block [v2]
> Hi, > > Could I have the simple update reviewed? > > In the PKCS12 key store implementation, the PBEKeySpec.clearPassword() should > be called in a finally try block. Otherwise, the password cleanup could be > interrupted by exceptions. > > Thanks, > Xuelei Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: an extra whitespace added - Changes: - all: https://git.openjdk.java.net/jdk/pull/8377/files - new: https://git.openjdk.java.net/jdk/pull/8377/files/123374d1..99079d30 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8377=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8377=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8377.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8377/head:pull/8377 PR: https://git.openjdk.java.net/jdk/pull/8377
