Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v2]
> https://bugs.openjdk.java.net/browse/JDK-8285263 Minor cleanup could be done > in java.security > > JDK-8273046 is the umbrella bug for this bug. The changes were too large for > a single code review, so it was decided to split into smaller chunks. This is > one such chunk: > > open/src/java.base/share/classes/java/security Mark Powers has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits: - Merge - second iteration - Merge - Merge - first iteration - Changes: https://git.openjdk.java.net/jdk/pull/8319/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8319=01 Stats: 656 lines in 100 files changed: 25 ins; 142 del; 489 mod Patch: https://git.openjdk.java.net/jdk/pull/8319.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8319/head:pull/8319 PR: https://git.openjdk.java.net/jdk/pull/8319
Re: RFR: 8284910: Buffer clean in PasswordCallback [v2]
On Wed, 20 Apr 2022 21:09:54 GMT, Roger Riggs wrote: >>> > Won't there be a small performance hit (perhaps negligible) for code that >>> > already calls clearPassword? >>> >>> The impact should be minimal. If clearPassword() has been called, the >>> cleanup (Cleanerable.clean()) won't happen again in the finalization or >>> setPassword cleanup. >> >> I don't think that is true, but maybe I am missing something. From looking >> at the code, it appears a `clearPassword` followed by a `setPassword` would >> call `Arrays.fill` twice on the same password. I think this can be fixed by >> setting the cleaner to null in the `clearPassword` method after the password >> has been cleared. I think this would address my concern and we may not need >> a spec. update (though I want to think it thru one more time). >> >>> > A specification clarification would provide clarity to applications that >>> > they do not have to call clearPassword in between calls to setPassword. >>> >>> As far as I know from the JDK code, it might be not common to call >>> clearPassword in between calls to setPassword. I would like to have >>> applications calling clearPassword() methods as before, if it is not >>> missed, to speed up the collection rather than rely on finalization. >> >> Yes, I agree calling `clearPassword` should be recommended as a best >> practice and callers should not solely rely on cleaners. >> >>> The relationship among setPassword, getPassword and clearPassword() is >>> complicated. I fully agree that the spec should be clarified. I would like >>> to have the clarify update in another PR, and have this one focus on >>> cleanup if an application forget to call clearPassword properly. >> >> See above. > > Calling `Cleanable.clean()` calls the `Runnable` action at-most-once. > Each `Cleanable` inserted in a list when it is created and is removed when > `clear()` or `clean()` is invoked. > The action is called only if it is still in the list. So there is no extra > overhead. > > There is no harm in clearing the cleanable field; but it does not save much. > > The code in `clearPassword` can be simplified and only test `cleanable != > null`; it will be null unless there is an inputPassword to clean. I'd recommend setting `cleanable` to null after it's been cleaned to make the state machine easier to reason about. The invariant would be: if `cleanable` is non-null, then we have something dirty that needs to be cleaned. If we don't clear it to null after cleaning, it potentially results in confusing states. For example, suppose the app calls `setPassword(nonNull)` and later calls `setPassword(null)`. The second call will set `inputPassword` to null but leave a stale reference in `cleanable`. This isn't necessarily harmful, but it's confusing. - PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284910: Buffer clean in PasswordCallback [v2]
On Wed, 20 Apr 2022 19:59:16 GMT, Sean Mullan wrote: >>> Won't there be a small performance hit (perhaps negligible) for code that >>> already calls clearPassword? >> >> The impact should be minimal. If clearPassword() has been called, the >> cleanup (Cleanerable.clean()) won't happen again in the finalization or >> setPassword cleanup. >> >>> A specification clarification would provide clarity to applications that >>> they do not have to call clearPassword in between calls to setPassword. >> >> As far as I know from the JDK code, it might be not common to call >> clearPassword in between calls to setPassword. I would like to have >> applications calling clearPassword() methods as before, if it is not missed, >> to speed up the collection rather than rely on finalization. >> >> The relationship among setPassword, getPassword and clearPassword() is >> complicated. I fully agree that the spec should be clarified. I would like >> to have the clarify update in another PR, and have this one focus on cleanup >> if an application forget to call clearPassword properly. > >> > Won't there be a small performance hit (perhaps negligible) for code that >> > already calls clearPassword? >> >> The impact should be minimal. If clearPassword() has been called, the >> cleanup (Cleanerable.clean()) won't happen again in the finalization or >> setPassword cleanup. > > I don't think that is true, but maybe I am missing something. From looking at > the code, it appears a `clearPassword` followed by a `setPassword` would call > `Arrays.fill` twice on the same password. I think this can be fixed by > setting the cleaner to null in the `clearPassword` method after the password > has been cleared. I think this would address my concern and we may not need a > spec. update (though I want to think it thru one more time). > >> > A specification clarification would provide clarity to applications that >> > they do not have to call clearPassword in between calls to setPassword. >> >> As far as I know from the JDK code, it might be not common to call >> clearPassword in between calls to setPassword. I would like to have >> applications calling clearPassword() methods as before, if it is not missed, >> to speed up the collection rather than rely on finalization. > > Yes, I agree calling `clearPassword` should be recommended as a best practice > and callers should not solely rely on cleaners. > >> The relationship among setPassword, getPassword and clearPassword() is >> complicated. I fully agree that the spec should be clarified. I would like >> to have the clarify update in another PR, and have this one focus on cleanup >> if an application forget to call clearPassword properly. > > See above. Calling `Cleanable.clean()` calls the `Runnable` action at-most-once. Each `Cleanable` inserted in a list when it is created and is removed when `clear()` or `clean()` is invoked. The action is called only if it is still in the list. So there is no extra overhead. There is no harm in clearing the cleanable field; but it does not save much. The code in `clearPassword` can be simplified and only test `cleanable != null`; it will be null unless there is an inputPassword to clean. - PR: https://git.openjdk.java.net/jdk/pull/8272
RFR: JDK-8285263 Minor cleanup could be done in java.security
https://bugs.openjdk.java.net/browse/JDK-8285263 Minor cleanup could be done in java.security JDK-8273046 is the umbrella bug for this bug. The changes were too large for a single code review, so it was decided to split into smaller chunks. This is one such chunk: open/src/java.base/share/classes/java/security - Commit messages: - second iteration - Merge - Merge - first iteration Changes: https://git.openjdk.java.net/jdk/pull/8319/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8319=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285263 Stats: 665 lines in 100 files changed: 25 ins; 142 del; 498 mod Patch: https://git.openjdk.java.net/jdk/pull/8319.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8319/head:pull/8319 PR: https://git.openjdk.java.net/jdk/pull/8319
Re: AlgorithmConstraints caching [ was Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice]
I think the work done with 8284694 will help alot in any case since I suspect the same SSLAlgorithmConstraints Object will be shared much more now (rather than spin off new copies) Some recent JFRs I looked at show that alot of CPU cycles[1] get taken in the HandshakeContext methods of : sun.security.ssl.HandshakeContext#getActiveCipherSuites sun.security.ssl.HandshakeContext#getActiveProtocols Both methods get called per Handshakecontext construction and I think each TLS handshake gets a new Handshakecontext.I was thinking that a cache of the last known variables used to deduce the getActiveCipherSuites and getActiveProtocols Lists could be created. That might have the potential to avoid alot of needless CPU cycles in this area since the parameters used to produce these Lists don't really change that often. I'm still looking into this potential and hope to share a patch shortly. regards, Sean. [1] Stack Trace Count Percentage TreeMap$Entry java.util.TreeMap.getEntryUsingComparator(Object) 1035 100 % TreeMap$Entry java.util.TreeMap.getEntry(Object) 1035 100 % boolean java.util.TreeMap.containsKey(Object) 1035 100 % boolean java.util.TreeSet.contains(Object) 1035 100 % boolean sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(Set, String, AlgorithmDecomposer) 1035 100 % boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 1035 100 % boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 1035 100 % boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 553 53.4 % boolean sun.security.ssl.HandshakeContext.isActivatable(CipherSuite, AlgorithmConstraints, Map) 309 29.9 % List sun.security.ssl.HandshakeContext.getActiveCipherSuites(List, List, AlgorithmConstraints) 302 29.2 % void sun.security.ssl.HandshakeContext.(SSLContextImpl, TransportContext) 302 29.2 % void sun.security.ssl.ClientHandshakeContext.(SSLContextImpl, TransportContext) 302 29.2 % void sun.security.ssl.TransportContext.kickstart() 302 29.2 % void sun.security.ssl.SSLSocketImpl.startHandshake(boolean) 302 29.2 % On 13/04/2022 18:05, Anthony Scarpino wrote: Hi Sean, Caching is an interesting idea. I've wondered for a while off and on about how to speed it up, but hadn't come up with a solution I liked. The complication with caching is while something like an algorithm name only could be easy in a hashmap, it gets more complicated when you get into key sizes. Such as, how to represent RSA 1k being disallowed and but 2k allowed.. or certificate usage.. Tony On 4/13/22 2:03 AM, Sean Coffey wrote: On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński wrote: During TLS handshake, hundreds of constraints are evaluated to determine which cipher suites are usable. Most of the evaluations are performed using `HandshakeContext#algorithmConstraints` object. By default that object contains a `SSLAlgorithmConstraints` instance wrapping another `SSLAlgorithmConstraints` instance. As a result the constraints defined in `SSLAlgorithmConstraints` are evaluated twice. This PR improves the default case; if the user-specified constraints are left at defaults, we use a single `SSLAlgorithmConstraints` instance, and avoid duplicate checks. Nice work. I've been looking at this area myself in recent weeks also while debugging some JDK 11u performance issues. JFR shows this area as costly. Some other JDK fixes in this area have already improved performance. This will certainly help. Pasting a stacktrace[1] from an old Oracle JDK 11.0.12 report for history purposes. I think there are other improvements that can be made in the TLS constraints code. Caching the last known values from a constraints permits test is something I've begun looking into. [1] Stack Trace Count Percentage boolean java.lang.StringLatin1.regionMatchesCI(byte[], int, byte[], int, int) 1637 100 % boolean java.lang.String.regionMatches(boolean, int, String, int, int) 1637 100 % boolean java.lang.String.equalsIgnoreCase(String) 1637 100 % boolean sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(List, String, AlgorithmDecomposer) 1631 99.6 % boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 1631 99.6 % boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 1631 99.6 % boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 836 51.1 % boolean sun.security.ssl.HandshakeContext.isActivatable(CipherSuite, AlgorithmConstraints, Map) 428 26.1 % List sun.security.ssl.HandshakeContext.getActiveCipherSuites(List, List, AlgorithmConstraints) 418 25.5 % void sun.security.ssl.HandshakeContext.(SSLContextImpl, TransportContext) 418 25.5 % void
Re: RFR: 8284910: Buffer clean in PasswordCallback [v2]
On Tue, 19 Apr 2022 15:21:49 GMT, Xue-Lei Andrew Fan wrote: > > Won't there be a small performance hit (perhaps negligible) for code that > > already calls clearPassword? > > The impact should be minimal. If clearPassword() has been called, the cleanup > (Cleanerable.clean()) won't happen again in the finalization or setPassword > cleanup. I don't think that is true, but maybe I am missing something. From looking at the code, it appears a `clearPassword` followed by a `setPassword` would call `Arrays.fill` twice on the same password. I think this can be fixed by setting the cleaner to null in the `clearPassword` method after the password has been cleared. I think this would address my concern and we may not need a spec. update (though I want to think it thru one more time). > > A specification clarification would provide clarity to applications that > > they do not have to call clearPassword in between calls to setPassword. > > As far as I know from the JDK code, it might be not common to call > clearPassword in between calls to setPassword. I would like to have > applications calling clearPassword() methods as before, if it is not missed, > to speed up the collection rather than rely on finalization. Yes, I agree calling `clearPassword` should be recommended as a best practice and callers should not solely rely on cleaners. > The relationship among setPassword, getPassword and clearPassword() is > complicated. I fully agree that the spec should be clarified. I would like to > have the clarify update in another PR, and have this one focus on cleanup if > an application forget to call clearPassword properly. See above. - PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284910: Buffer clean in PasswordCallback [v2]
On Tue, 19 Apr 2022 15:03:05 GMT, Xue-Lei Andrew Fan wrote: > > I am not quite seeing the rationale for this change. > > There were a lot of effort to clean up the buffering password in JDK. In some > circumstances, the PasswordCallback would be called for further using of the > password. However, because the call to PasswordCallback, the password cleanup > effort was void as PasswordCallback will have a copy of the password in the > memory. > > For example, in the P11KeyStore implementation, there is an effort to clean > up the password while finalizing. ` Arrays.fill(password, ' ');` However, the > password has been set to the PasswordCallback, and where a copy is placed in > the memory. > > ``` > PasswordCallback pc = (PasswordCallback)callbacks[0]; > pc.setPassword(password); // this clones the password if not null > ``` Ok, but the [SunPKCS11 code clears the cloned password](https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java#L1513) as soon as it retrieves it from the Callback: pin = pcall.getPassword(); pcall.clearPassword(); > > Are you trying to protect against callers forgetting to call clearPassword? > > Is that really our responsibility? > > Yes, the clearPassword() method may be not called as expected. It may be not > our responsibility, but it would be good to collect the password even if the > clearPassword() method is not called. It is just something like to clean up > the socket if socket.close() is not called. I may file another PR later, > where password cleanup/destroy is should be called, but not actually. Ok, I can see how this can be a good DiD strategy. However, I think that we need to carefully check the interactions between cleaners and methods that explicitly allow the contents to be cleared so that there are not unexpected results. - PR: https://git.openjdk.java.net/jdk/pull/8272
Integrated: 8284291: sun/security/krb5/auto/Renew.java fails intermittently on Windows 11
On Mon, 4 Apr 2022 21:27:51 GMT, Weijun Wang wrote: > `Thread.sleep()` seems not very precise on some systems. Update this test to > check the current time continously. This pull request has now been integrated. Changeset: 05ae7ed1 Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/05ae7ed1aac6fabc9c8820c12b6567fe93a3546f Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod 8284291: sun/security/krb5/auto/Renew.java fails intermittently on Windows 11 Reviewed-by: aturbanov, ascarpino - PR: https://git.openjdk.java.net/jdk/pull/8098
Integrated: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński wrote: > During TLS handshake, hundreds of constraints are evaluated to determine > which cipher suites are usable. Most of the evaluations are performed using > `HandshakeContext#algorithmConstraints` object. By default that object > contains a `SSLAlgorithmConstraints` instance wrapping another > `SSLAlgorithmConstraints` instance. As a result the constraints defined in > `SSLAlgorithmConstraints` are evaluated twice. > > This PR improves the default case; if the user-specified constraints are left > at defaults, we use a single `SSLAlgorithmConstraints` instance, and avoid > duplicate checks. This pull request has now been integrated. Changeset: d8446b4f Author:Daniel Jeliński URL: https://git.openjdk.java.net/jdk/commit/d8446b4f60472b11e4cdaef97288fe143cca4511 Stats: 427 lines in 7 files changed: 385 ins; 1 del; 41 mod 8284694: Avoid evaluating SSLAlgorithmConstraints twice Reviewed-by: redestad, xuelei, coffeys - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v5]
> During TLS handshake, hundreds of constraints are evaluated to determine > which cipher suites are usable. Most of the evaluations are performed using > `HandshakeContext#algorithmConstraints` object. By default that object > contains a `SSLAlgorithmConstraints` instance wrapping another > `SSLAlgorithmConstraints` instance. As a result the constraints defined in > `SSLAlgorithmConstraints` are evaluated twice. > > This PR improves the default case; if the user-specified constraints are left > at defaults, we use a single `SSLAlgorithmConstraints` instance, and avoid > duplicate checks. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Reduce line length - Changes: - all: https://git.openjdk.java.net/jdk/pull/8199/files - new: https://git.openjdk.java.net/jdk/pull/8199/files/2a1f0a1d..1a131d9e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8199=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8199=03-04 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8199.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8199/head:pull/8199 PR: https://git.openjdk.java.net/jdk/pull/8199
Integrated: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec
On Tue, 12 Apr 2022 01:27:35 GMT, Valerie Peng wrote: > This trivial change is to deprecate the DEFAULT static field of > OAEPParameterSpec class. Wordings are mostly the same as the previous > PSSParameterSpec deprecation change. Rest are just minor code re-factoring. > > The CSR will be filed once review is somewhat finished. > > Thanks, > Valerie This pull request has now been integrated. Changeset: 15ce8c61 Author:Valerie Peng URL: https://git.openjdk.java.net/jdk/commit/15ce8c61956ec433bcb713c694e6cef7a61e3837 Stats: 53 lines in 2 files changed: 15 ins; 18 del; 20 mod 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Wed, 20 Apr 2022 16:10:17 GMT, Sean Mullan wrote:
>> For (1), how about something like below:
>>
>>> * The returned parameters may be the same that were used to
>>> initialize
>>> * this cipher, or may contain additional default or random parameter
>>> * values used by the underlying cipher implementation. If no parameters
>>> * is supplied and this cipher successfully generated the required
>>> * parameter values, it will be returned. Otherwise, {@code null} is
>>> * returned.
>
>> Hmm, I tried the suggested approach in (1), the result looks very lengthy.
>> Actually, the Cipher.init(..) methods already has a few paragraphs
>> describing the behavior for parameter generation, perhaps we should not
>> repeat stating the "If this cipher was initialized" vs "was not initialized"
>> since lengthy description may confuse users and have higher risks of
>> inconsistency. What do you think?
>
> That's a good point, the `init` methods do go into a lot of detail about that.
>
>> As for (2), currently only RSASSA-PSS signature impl uses parameters. When
>> specified using PSSParameterSpec, it's possible that some of the parameters
>> are not set, so it's possible for providers to use provider-specific default
>> values for PSS case. So, yes, Signature class may have to updated in the
>> same way to cover this particular scenario.
>
> Ok, I think we should try to make the spec consistent across `Cipher` and
> `Signature` once we settle on the right wording. I think it would be better
> to do it as part of this issue, but I would be ok doing it later as long as
> it is also fixed in 19.
> For (1), how about something like below:
>
> > ```
> > * The returned parameters may be the same that were used to initialize
> > * this cipher, or may contain additional default or random parameter
> > * values used by the underlying cipher implementation.
I like this first sentence.
> > If no parameters
> > * is supplied and this cipher successfully generated the required
> > * parameter values, it will be returned.
What does "successfully" mean? If it wasn't successful, what happens? Maybe we
should avoid that word. How about:
"If parameters were not supplied and this cipher requires parameters, the
returned parameters will contain the parameter values generated by the
underlying cipher implementation."
> > Otherwise, {@code null} is
> > * returned.
> > ```
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Tue, 19 Apr 2022 02:44:01 GMT, Valerie Peng wrote:
>> Hmm, I tried the suggested approach in (1), the result looks very lengthy.
>> Actually, the Cipher.init(..) methods already has a few paragraphs
>> describing the behavior for parameter generation, perhaps we should not
>> repeat stating the "If this cipher was initialized" vs "was not initialized"
>> since lengthy description may confuse users and have higher risks of
>> inconsistency. What do you think?
>> As for (2), currently only RSASSA-PSS signature impl uses parameters. When
>> specified using PSSParameterSpec, it's possible that some of the parameters
>> are not set, so it's possible for providers to use provider-specific default
>> values for PSS case. So, yes, Signature class may have to updated in the
>> same way to cover this particular scenario.
>
> For (1), how about something like below:
>
>> * The returned parameters may be the same that were used to
>> initialize
>> * this cipher, or may contain additional default or random parameter
>> * values used by the underlying cipher implementation. If no parameters
>> * is supplied and this cipher successfully generated the required
>> * parameter values, it will be returned. Otherwise, {@code null} is
>> * returned.
> Hmm, I tried the suggested approach in (1), the result looks very lengthy.
> Actually, the Cipher.init(..) methods already has a few paragraphs describing
> the behavior for parameter generation, perhaps we should not repeat stating
> the "If this cipher was initialized" vs "was not initialized" since lengthy
> description may confuse users and have higher risks of inconsistency. What do
> you think?
That's a good point, the `init` methods do go into a lot of detail about that.
> As for (2), currently only RSASSA-PSS signature impl uses parameters. When
> specified using PSSParameterSpec, it's possible that some of the parameters
> are not set, so it's possible for providers to use provider-specific default
> values for PSS case. So, yes, Signature class may have to updated in the same
> way to cover this particular scenario.
Ok, I think we should try to make the spec consistent across `Cipher` and
`Signature` once we settle on the right wording. I think it would be better to
do it as part of this issue, but I would be ok doing it later as long as it is
also fixed in 19.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v5]
On Wed, 20 Apr 2022 15:20:15 GMT, Daniel Fuchs wrote: > Also should there be some synchronized block? I thought about the synchronization issue. It looks like this class is not multiple-threads safe, and thus the upper layer will take care of the synchronization. So I did not add synchronization in this class update. > should pContext be set to 0 before calling cleanable.clean() to make sure > cleanable.clean() is not called twice? Cleanable.clean() will work one time only, no matter how many time it is called. But it looks like better to make a change here. - PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v7]
> 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 the calling order in dispose() - Changes: - all: https://git.openjdk.java.net/jdk/pull/8136/files - new: https://git.openjdk.java.net/jdk/pull/8136/files/232d2a60..aed8cd32 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8136=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8136=05-06 Stats: 2 lines in 1 file changed: 1 ins; 1 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: 8284291: sun/security/krb5/auto/Renew.java fails intermittently on Windows 11
On Mon, 4 Apr 2022 21:27:51 GMT, Weijun Wang wrote: > `Thread.sleep()` seems not very precise on some systems. Update this test to > check the current time continously. Marked as reviewed by ascarpino (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8098
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v6]
> 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 two additional commits since the last revision: - More code cleanup - re-org the code per feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/8136/files - new: https://git.openjdk.java.net/jdk/pull/8136/files/83bc4164..232d2a60 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8136=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8136=04-05 Stats: 10 lines in 2 files changed: 2 ins; 6 del; 2 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 [v5]
On Wed, 20 Apr 2022 15:12:23 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:
>
> Update to set context method
src/java.security.jgss/share/classes/sun/security/jgss/wrapper/NativeGSSContext.java
line 378:
> 376: if (pContext != 0 && cleanable != null) {
> 377: cleanable.clean();
> 378: pContext = 0;
Here and below: should pContext be set to 0 before calling cleanable.clean() to
make sure cleanable.clean() is not called twice? Also should there be some
synchronized block?
-
PR: https://git.openjdk.java.net/jdk/pull/8136
Re: RFR: 8284490: Remove finalizer method in java.security.jgss [v5]
> 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: Update to set context method - Changes: - all: https://git.openjdk.java.net/jdk/pull/8136/files - new: https://git.openjdk.java.net/jdk/pull/8136/files/464d3981..83bc4164 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8136=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8136=03-04 Stats: 47 lines in 6 files changed: 29 ins; 9 del; 9 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: Command line flag to disable finalizers.
On 4/15/22 10:46 PM, Peter Firmstone wrote: To securely instrument access controls onto public Java API, we need to have the ability to disable finalizers, to prevent finalizer attacks from circumventing access controls. Since finalizers are planned for removal, as soon as finalizers are officially deprecated, I propose a command line flag be provided to disable jvm calls to finalizer methods. This is already supported. JEP 421 added a "--finalization=disabled" option to JDK 18: https://openjdk.java.net/jeps/421#Command-line-option-to-disable-finalization --Sean
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
On Tue, 19 Apr 2022 17:16:29 GMT, Daniel Jeliński wrote: >> During TLS handshake, hundreds of constraints are evaluated to determine >> which cipher suites are usable. Most of the evaluations are performed using >> `HandshakeContext#algorithmConstraints` object. By default that object >> contains a `SSLAlgorithmConstraints` instance wrapping another >> `SSLAlgorithmConstraints` instance. As a result the constraints defined in >> `SSLAlgorithmConstraints` are evaluated twice. >> >> This PR improves the default case; if the user-specified constraints are >> left at defaults, we use a single `SSLAlgorithmConstraints` instance, and >> avoid duplicate checks. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > Replace remaining constructors with factory methods Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
On Wed, 20 Apr 2022 10:28:39 GMT, Daniel Jeliński wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
>> line 94:
>>
>>> 92: AlgorithmConstraints userSpecifiedConstraints,
>>> 93: boolean withDefaultCertPathConstraints) {
>>> 94: if (nullIfDefault(userSpecifiedConstraints) == null) {
>>
>> Do you wan to check DEFAULT_SSL_ONLY in the nullIfDefault() implementation?
>> The logic of the block is a little bit hard to understand to me.
>
> No I don't; it's for the same reason why I'm using `==` and not `equals`:
> `DEFAULT` is the only `SSLAlgorithmConstraints` instance that is ever used as
> `userSpecifiedConstraints` here.
>
> `DEFAULT` is used because [SSLConfiguration sets
> userSpecifiedAlgorithmConstraints to
> SSLAlgorithmConstraints.DEFAULT](https://github.com/openjdk/jdk/blob/6d8d156c97b90a9ab4776c6b42563a962d959741/src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java#L129).
> This feels wrong; the name suggests that the constraints should be specified
> by user, and should be null if the user doesn't touch them.
> `userSpecifiedAlgorithmConstraints` are accessible by
> `getSSLParameters().getAlgorithmConstraints()` on SSLEngineImpl and
> SSLSocketImpl. Returning `DEFAULT` here also feels wrong; as a user I would
> be concerned that setting my own algorithm constraints would replace the
> default ones. It doesn't, but that is not immediately apparent.
>
> We could initialize `userSpecifiedAlgorithmConstraints` to null, and back out
> all the other changes from this PR. The only reason why I didn't do that was
> because it would change the observable behavior
> (`getSSLParameters().getAlgorithmConstraints()` would return `null`). If you
> think we can live with that, I'll be happy to do that change.
It is not interested to me to use 'null' constraints in ssl configure. I have
no more comments. Thank you for the update!
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284853: Fix various 'expected' typo [v2]
On Thu, 14 Apr 2022 18:04:16 GMT, Andrey Turbanov wrote: > I found [yet another > typo](https://github.com/kelthuzadx/jdk/commit/acb9e15bc0bf5395d1c0875f36992f692734f948) > ... I didn't think "JVMInvokeMethodSlack" was a typo. I think it's the idea of "slack space" meaning leftover space. We require a certain amount of this space. We need some slack on the stack, in order to invoke. - PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
On Wed, 20 Apr 2022 04:19:38 GMT, Xue-Lei Andrew Fan wrote:
>> Daniel Jeliński has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Replace remaining constructors with factory methods
>
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 94:
>
>> 92: AlgorithmConstraints userSpecifiedConstraints,
>> 93: boolean withDefaultCertPathConstraints) {
>> 94: if (nullIfDefault(userSpecifiedConstraints) == null) {
>
> Do you wan to check DEFAULT_SSL_ONLY in the nullIfDefault() implementation?
> The logic of the block is a little bit hard to understand to me.
No I don't; it's for the same reason why I'm using `==` and not `equals`:
`DEFAULT` is the only `SSLAlgorithmConstraints` instance that is ever used as
`userSpecifiedConstraints` here.
`DEFAULT` is used because [SSLConfiguration sets
userSpecifiedAlgorithmConstraints to
SSLAlgorithmConstraints.DEFAULT](https://github.com/openjdk/jdk/blob/6d8d156c97b90a9ab4776c6b42563a962d959741/src/java.base/share/classes/sun/security/ssl/SSLConfiguration.java#L129).
This feels wrong; the name suggests that the constraints should be specified
by user, and should be null if the user doesn't touch them.
`userSpecifiedAlgorithmConstraints` are accessible by
`getSSLParameters().getAlgorithmConstraints()` on SSLEngineImpl and
SSLSocketImpl. Returning `DEFAULT` here also feels wrong; as a user I would be
concerned that setting my own algorithm constraints would replace the default
ones. It doesn't, but that is not immediately apparent.
We could initialize `userSpecifiedAlgorithmConstraints` to null, and back out
all the other changes from this PR. The only reason why I didn't do that was
because it would change the observable behavior
(`getSSLParameters().getAlgorithmConstraints()` would return `null`). If you
think we can live with that, I'll be happy to do that change.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
