Integrated: 8284933: Improve debug in jdk.crypto.cryptoki
On Sun, 17 Apr 2022 14:45:49 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have the simple update reviewed? > > In the jdk.crypto.cryptoki module implementation, some of the debug > information could be calculated even if the debug is not enabled, which is > not resource friendly. > > Thanks, > Xuelei This pull request has now been integrated. Changeset: 0f81d8fc Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/0f81d8fcc3fb703760b1cddb01861ea5031023fb Stats: 37 lines in 1 file changed: 20 ins; 6 del; 11 mod 8284933: Improve debug in jdk.crypto.cryptoki Reviewed-by: valeriep - PR: https://git.openjdk.java.net/jdk/pull/8277
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
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.
src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line
97:
> 95: return withDefaultCertPathConstraints ? DEFAULT :
> DEFAULT_SSL_ONLY;
> 96: }
> 97: return new SSLAlgorithmConstraints(userSpecifiedConstraints,
> withDefaultCertPathConstraints);
It would be nice to limit each line within 80 characters, which is useful for
terminal users.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
On Tue, 19 Apr 2022 17:21:20 GMT, Daniel Jeliński wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
>> line 94:
>>
>>> 92: * @return a SSLAlgorithmConstraints instance
>>> 93: */
>>> 94: static AlgorithmConstraints forSocket(SSLSocket socket,
>>
>> I like the idea to use a static method for the construction. What do you
>> think if the same coding style is used for other constructors, by making
>> them private and add forSocket() methods accordingly? For example, changing
>> the following constructor to a private method.
>>
>> SSLAlgorithmConstraints(SSLEngine engine, String[] supportedAlgorithms,
>> boolean withDefaultCertPathConstraints) {
>
> it won't change the performance in any way - when `supportedAlgorithms` are
> set, we always need to create a new object. But you're right, it's
> inconsistent to offer both constructors and factory methods in the same
> class. Changed.
The variants of supportedAlgorithms could be limited. There might be a
potential improvement. But it is out of the scope of this PR.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: JDK-8284688 Minor cleanup could be done in java.security.jgss [v3]
> https://bugs.openjdk.java.net/browse/JDK-8284688 > > [JDK-8273046](https://bugs.openjdk.java.net/browse/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.security.jgss/share/classes/javax/security > open/src/java.security.jgss/share/classes/org/ietf > open/src/java.security.jgss/share/classes/sun/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 ten commits: - fifth iteration - Merge - fourth iteration - third iteration - Merge - Merge - Merge - second iteration - first iteration - Changes: https://git.openjdk.java.net/jdk/pull/7746/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7746=02 Stats: 929 lines in 73 files changed: 88 ins; 193 del; 648 mod Patch: https://git.openjdk.java.net/jdk/pull/7746.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7746/head:pull/7746 PR: https://git.openjdk.java.net/jdk/pull/7746
Re: RFR: 8284933: Improve debug in jdk.crypto.cryptoki [v2]
On Tue, 19 Apr 2022 13:48:26 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> May I have the simple update reviewed? >> >> In the jdk.crypto.cryptoki module implementation, some of the debug >> information could be calculated even if the debug is not enabled, which is >> not resource friendly. >> >> Thanks, >> Xuelei > > Xue-Lei Andrew Fan has updated the pull request incrementally with one > additional commit since the last revision: > > Remove debug() method Looks good to me. Thanks~ - Marked as reviewed by valeriep (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8277
Integrated: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki
On Thu, 14 Apr 2022 18:06:10 GMT, Xue-Lei Andrew Fan wrote:
> This is an effort to fix a problem introduced in the fix for
> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However, there
> is a problem with the code changes. The Runnables registered with Cleaner
> refer to the object being registered ('this'). Meaning, the Cleaner mechanism
> will keep the objects reachable, preventing them from being cleaned and
> collected.
This pull request has now been integrated.
Changeset: 60446746
Author:Xue-Lei Andrew Fan
URL:
https://git.openjdk.java.net/jdk/commit/60446746d41c3c80d9788a252b4a55afe44e1e7b
Stats: 82 lines in 7 files changed: 32 ins; 24 del; 26 mod
8284855: Update needed to Cleaners added to jdk.crypto.cryptoki
Reviewed-by: valeriep
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec [v4]
> 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 Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: Remove "the" and change to "@throws". - Changes: - all: https://git.openjdk.java.net/jdk/pull/8191/files - new: https://git.openjdk.java.net/jdk/pull/8191/files/a760fc51..cbae8613 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8191=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8191=02-03 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8191.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8191/head:pull/8191 PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec [v3]
On Tue, 19 Apr 2022 19:54:24 GMT, Sean Mullan wrote:
>> Valerie Peng has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Removed the private no-arg default constructor and minor javadoc update.
>
> src/java.base/share/classes/javax/crypto/spec/OAEPParameterSpec.java line 126:
>
>> 124: * {@link #getMGFParameters()}
>> 125: * @param pSrc the source of the encoding input P
>> 126: * @exception NullPointerException if {@code mdName},
>
> Change `@exception` to the more accepted `@throws` tag.
Yes, will change.
-
PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec [v3]
On Tue, 19 Apr 2022 19:52:34 GMT, Sean Mullan wrote: >> Valerie Peng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Removed the private no-arg default constructor and minor javadoc update. > > src/java.base/share/classes/javax/crypto/spec/OAEPParameterSpec.java line 99: > >> 97: * @deprecated This field uses the default values defined in the >> PKCS #1 >> 98: * standard. Some of these defaults are no longer >> recommended due >> 99: * to advances in cryptanalysis -- see the > > Remove "the". Ok~ - PR: https://git.openjdk.java.net/jdk/pull/8191
Integrated: 8284893: Fix typos in java.base
On Thu, 14 Apr 2022 19:07:09 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. This pull request has now been integrated. Changeset: fb469fb8 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/fb469fb894ed84686f9fec5787ac99eb535fdd18 Stats: 369 lines in 162 files changed: 0 ins; 0 del; 369 mod 8284893: Fix typos in java.base Reviewed-by: iris, wetmore, lancea, mullan, naoto - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec [v3]
On Mon, 18 Apr 2022 23:36:56 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
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Removed the private no-arg default constructor and minor javadoc update.
Marked as reviewed by mullan (Reviewer).
src/java.base/share/classes/javax/crypto/spec/OAEPParameterSpec.java line 99:
> 97: * @deprecated This field uses the default values defined in the PKCS
> #1
> 98: * standard. Some of these defaults are no longer recommended
> due
> 99: * to advances in cryptanalysis -- see the
Remove "the".
src/java.base/share/classes/javax/crypto/spec/OAEPParameterSpec.java line 126:
> 124: * {@link #getMGFParameters()}
> 125: * @param pSrc the source of the encoding input P
> 126: * @exception NullPointerException if {@code mdName},
Change `@exception` to the more accepted `@throws` tag.
-
PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v7]
On Tue, 19 Apr 2022 14:00:06 GMT, Xue-Lei Andrew Fan wrote:
>> This is an effort to fix a problem introduced in the fix for
>> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
>> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However,
>> there is a problem with the code changes. The Runnables registered with
>> Cleaner refer to the object being registered ('this'). Meaning, the Cleaner
>> mechanism will keep the objects reachable, preventing them from being
>> cleaned and collected.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> remove duplicated checking
Marked as reviewed by valeriep (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284893: Fix typos in java.base [v4]
On Tue, 19 Apr 2022 16:50:12 GMT, Magnus Ihse Bursie wrote: >> I ran `codespell` on the `src/java.base` directory, and accepted those >> changes where it indeed discovered real typos. >> >> (Due to false positives this can unfortunately not be run automatically) >> >> The majority of fixes are in comments. A handful is in strings, one in a >> local variable name, and a couple in parameter declarations. >> >> Annoyingly, there are several instances of "childs" (instead of "children") >> in the source code, but they were not local and I dared not change them. >> Someone braver than me might take a stab at it, perhaps.. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Update Oracle copyrights > - Also revert changes in ASM (3rd party code) Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284893: Fix typos in java.base [v4]
On Tue, 19 Apr 2022 16:50:12 GMT, Magnus Ihse Bursie wrote: >> I ran `codespell` on the `src/java.base` directory, and accepted those >> changes where it indeed discovered real typos. >> >> (Due to false positives this can unfortunately not be run automatically) >> >> The majority of fixes are in comments. A handful is in strings, one in a >> local variable name, and a couple in parameter declarations. >> >> Annoyingly, there are several instances of "childs" (instead of "children") >> in the source code, but they were not local and I dared not change them. >> Someone braver than me might take a stab at it, perhaps.. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Update Oracle copyrights > - Also revert changes in ASM (3rd party code) Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
On Tue, 19 Apr 2022 14:23:07 GMT, Xue-Lei Andrew Fan wrote:
>> Daniel Jeliński has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - add more factory methods, update copyright
>> - return DEFAULT also when user constraints are null
>
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 94:
>
>> 92: * @return a SSLAlgorithmConstraints instance
>> 93: */
>> 94: static AlgorithmConstraints forSocket(SSLSocket socket,
>
> I like the idea to use a static method for the construction. What do you
> think if the same coding style is used for other constructors, by making them
> private and add forSocket() methods accordingly? For example, changing the
> following constructor to a private method.
>
> SSLAlgorithmConstraints(SSLEngine engine, String[] supportedAlgorithms,
> boolean withDefaultCertPathConstraints) {
it won't change the performance in any way - when `supportedAlgorithms` are
set, we always need to create a new object. But you're right, it's inconsistent
to offer both constructors and factory methods in the same class. Changed.
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 118:
>
>> 116: if (userSpecifiedConstraints == null) {
>> 117: return withDefaultCertPathConstraints ? DEFAULT :
>> DEFAULT_SSL_ONLY;
>> 118: }
>
> It looks like a partial duplicate of nullIfDefault(). What do you think if
> merging the logic into nullIfDefault()? Or even merging nullIfDefault()
> logic into the getUserSpecifiedConstraints() method? New parameters may be
> required for the getUserSpecifiedConstraints() methods.
I'm not a big fan of modifying `getUserSpecifiedConstraints`; that method has 3
`return` clauses. But you're right, there was some code duplication that is
removed now.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes
On Sun, 17 Apr 2022 16:17:30 GMT, liach wrote:
> Convert dynamic proxies to hidden classes. Modifies the serialization of
> proxies (requires change in "Java Object Serialization Specification"). Makes
> the proxies hidden in stack traces. Removes duplicate logic in proxy building.
>
> The main compatibility changes and their rationales are:
> 1. Modification to the serialization specification: In the "An instance of
> the class is allocated... The contents restored appropriately" section, I
> propose explicitly state that handling of proxies are unspecified as to allow
> implementation freedom, though I've seen deliberate attempts for proxies to
> implement interfaces with `readResolve` in order to control their
> serialization behavior.
>- This is for the existing generated constructor accessor is
> bytecode-based, which cannot refer to hidden classes.
>- An alternative is to preserve the behavior, where the serialization
> constructor calls `invokespecial` on the closest serializable superclass'
> no-arg constructor, like in #1830 by @DasBrain.
>- My rationale against preservation is such super calls are unsafe and
> should be discouraged in the long term. Calling the existing constructor with
> a dummy argument, as in my implementation, would be more safe.
> 2. The disappearance of proxies in stack traces.
>- Same behavior exists in lambda expressions: For instance, in
> `((Runnable) () -> { throw new Error(); }).run();`, the `run` method,
> implemented by the lambda, will not appear in the stack trace, and isn't too
> problematic.
>
> A summary of the rest of the changes:
> 1. Merged the two passes of determining module and package of the proxy into
> one. This reduced a lot of code and allowed anchor class (for hidden class
> creation) selection be done together as well.
> 2. Exposed internal API for obtaining a full-privileged lookup to the rest of
> `java.base`. This API is intended for implementation of legacy (pre
> `MethodHandles.Lookup`) caller sensitive public APIs so they don't need more
> complex tricks to obtain proper permissions as lookups.
> 3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959):
> passes methods computed by proxy generator as class data to the hidden proxy
> class to reduce generated proxy class size and improve performance.
>
> In addition, since this change is somewhat large, should we keep the old
> proxy generator as well and have it toggled through a command-line flag (like
> the old v49 proxy generator or the old reflection implementation)?
>
> Please feel free to comment or review. This change definitely requires a CSR,
> but I have yet to determine what specifications should be changed.
Fixing deserialization is not particularly hard. A longer term goal is to
declare a readResolve for proxies (presumably by extra methods on invocation
handlers) to convert serialization result to something else
-
PR: https://git.openjdk.java.net/jdk/pull/8278
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v4]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/8199/files - new: https://git.openjdk.java.net/jdk/pull/8199/files/e4cc8152..2a1f0a1d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8199=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8199=02-03 Stats: 58 lines in 4 files changed: 21 ins; 14 del; 23 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
Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes
On Sun, 17 Apr 2022 16:17:30 GMT, liach wrote:
> Convert dynamic proxies to hidden classes. Modifies the serialization of
> proxies (requires change in "Java Object Serialization Specification"). Makes
> the proxies hidden in stack traces. Removes duplicate logic in proxy building.
>
> The main compatibility changes and their rationales are:
> 1. Modification to the serialization specification: In the "An instance of
> the class is allocated... The contents restored appropriately" section, I
> propose explicitly state that handling of proxies are unspecified as to allow
> implementation freedom, though I've seen deliberate attempts for proxies to
> implement interfaces with `readResolve` in order to control their
> serialization behavior.
>- This is for the existing generated constructor accessor is
> bytecode-based, which cannot refer to hidden classes.
>- An alternative is to preserve the behavior, where the serialization
> constructor calls `invokespecial` on the closest serializable superclass'
> no-arg constructor, like in #1830 by @DasBrain.
>- My rationale against preservation is such super calls are unsafe and
> should be discouraged in the long term. Calling the existing constructor with
> a dummy argument, as in my implementation, would be more safe.
> 2. The disappearance of proxies in stack traces.
>- Same behavior exists in lambda expressions: For instance, in
> `((Runnable) () -> { throw new Error(); }).run();`, the `run` method,
> implemented by the lambda, will not appear in the stack trace, and isn't too
> problematic.
>
> A summary of the rest of the changes:
> 1. Merged the two passes of determining module and package of the proxy into
> one. This reduced a lot of code and allowed anchor class (for hidden class
> creation) selection be done together as well.
> 2. Exposed internal API for obtaining a full-privileged lookup to the rest of
> `java.base`. This API is intended for implementation of legacy (pre
> `MethodHandles.Lookup`) caller sensitive public APIs so they don't need more
> complex tricks to obtain proper permissions as lookups.
> 3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959):
> passes methods computed by proxy generator as class data to the hidden proxy
> class to reduce generated proxy class size and improve performance.
>
> In addition, since this change is somewhat large, should we keep the old
> proxy generator as well and have it toggled through a command-line flag (like
> the old v49 proxy generator or the old reflection implementation)?
>
> Please feel free to comment or review. This change definitely requires a CSR,
> but I have yet to determine what specifications should be changed.
About deserializing Proxies, this is currently done in 3 steps:
1. Finding the class
2. Allocating a new instance and invoke the constructor of the **first
non-serializable superclass**
3. Setting the fields of the instance
Only step 2 is problematic here:
1. For a proxy, the class is serialized using a TC_PROXYCLASSDESC. when
deserialized it invokes `Proxy.getProxyClass`
(https://github.com/openjdk/jdk/blob/13fb1eed52f1a9152242119969a9d4a0c0627513/src/java.base/share/classes/java/io/ObjectInputStream.java#L891).
For this step, it doesn't matter if `Proxy.getProxyClass` returns a normal
class or a hidden class.
2. Allocating and calling the constructor:
This part is currently implemented by spinning a class. The generated
bytecode looks more or less like this:
anew ProxyClass
invokespecial Object.()
The big lie here is that methods and constructors are different - but
constructors are just methods, and the verifier makes sure that a constructor
is called only once, exactly once, and that uninitialized instances don't
escape.
This doesn't work for hidden classes - as the hidden class can not be named
in an other class.
But MethodHandles can do this. Here is one of my old prototypes, based on a
prototype for implementing reflection using MethodHandles from Mandy Chung:
https://github.com/dasbrain/jdk/compare/ae45c5de7fc635df4dc86b764405158c245d2765...fbb0a63436f696a85e7039c0e109c379dfa1edce
3. Setting the fields uses deep reflection.
But the hidden class themself doesn't have any fields - just it's
superclass `java.lang.reflect.Proxy.h`.
This is not a problem if the class can be instantiated at all (see step 2).
-
PR: https://git.openjdk.java.net/jdk/pull/8278
Re: RFR: 8284893: Fix typos in java.base [v4]
On Tue, 19 Apr 2022 16:50:12 GMT, Magnus Ihse Bursie wrote: >> I ran `codespell` on the `src/java.base` directory, and accepted those >> changes where it indeed discovered real typos. >> >> (Due to false positives this can unfortunately not be run automatically) >> >> The majority of fixes are in comments. A handful is in strings, one in a >> local variable name, and a couple in parameter declarations. >> >> Annoyingly, there are several instances of "childs" (instead of "children") >> in the source code, but they were not local and I dared not change them. >> Someone braver than me might take a stab at it, perhaps.. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Update Oracle copyrights > - Also revert changes in ASM (3rd party code) Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284893: Fix typos in java.base [v3]
On Tue, 19 Apr 2022 16:24:43 GMT, Magnus Ihse Bursie wrote: >> I ran `codespell` on the `src/java.base` directory, and accepted those >> changes where it indeed discovered real typos. >> >> (Due to false positives this can unfortunately not be run automatically) >> >> The majority of fixes are in comments. A handful is in strings, one in a >> local variable name, and a couple in parameter declarations. >> >> Annoyingly, there are several instances of "childs" (instead of "children") >> in the source code, but they were not local and I dared not change them. >> Someone braver than me might take a stab at it, perhaps.. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Vertices -> vertices It's a bit annoying that 3rd party code is not more distinctly handled in the JDK source. :( I reverted the code pointed out by reviewers, but then later found the ASM code as well. If I feel really motivated (or bored) I might try to submit PRs with these fixes upstream. Or not. Everybody happy now? - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284893: Fix typos in java.base [v4]
> I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. Magnus Ihse Bursie has updated the pull request incrementally with two additional commits since the last revision: - Update Oracle copyrights - Also revert changes in ASM (3rd party code) - Changes: - all: https://git.openjdk.java.net/jdk/pull/8250/files - new: https://git.openjdk.java.net/jdk/pull/8250/files/2b029279..a3f75247 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8250=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8250=02-03 Stats: 134 lines in 133 files changed: 0 ins; 0 del; 134 mod Patch: https://git.openjdk.java.net/jdk/pull/8250.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8250/head:pull/8250 PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284893: Fix typos in java.base [v3]
> I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Vertices -> vertices - Changes: - all: https://git.openjdk.java.net/jdk/pull/8250/files - new: https://git.openjdk.java.net/jdk/pull/8250/files/31baf0a3..2b029279 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8250=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8250=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8250.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8250/head:pull/8250 PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284893: Fix typos in java.base [v2]
> I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Revert changes to 3rd party code - Changes: - all: https://git.openjdk.java.net/jdk/pull/8250/files - new: https://git.openjdk.java.net/jdk/pull/8250/files/5e3a01c6..31baf0a3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8250=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8250=00-01 Stats: 15 lines in 11 files changed: 0 ins; 0 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/8250.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8250/head:pull/8250 PR: https://git.openjdk.java.net/jdk/pull/8250
RFR: 8255552: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms
Please review these changes to add DES/3DES/MD5 to `jdk.security.legacyAlgorithms` security property, and to add the legacy algorithm constraint checking to `keytool` commands that are associated with secret key entries stored in the keystore. These `keytool` commands are -genseckey, -importpass, -list, and -importkeystore. As a result, `keytool` will be able to generate warnings when it detects that the secret key based algorithms and PBE based Mac and cipher algorithms are weak. Also removes the "This algorithm will be disabled in a future update.” from the existing warnings for the asymmetric keys/certificates. Will also file a CSR. - Commit messages: - 822: Add DES/3DES/MD5 to jdk.security.legacyAlgorithms Changes: https://git.openjdk.java.net/jdk/pull/8300/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8300=00 Issue: https://bugs.openjdk.java.net/browse/JDK-822 Stats: 319 lines in 7 files changed: 233 ins; 0 del; 86 mod Patch: https://git.openjdk.java.net/jdk/pull/8300.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8300/head:pull/8300 PR: https://git.openjdk.java.net/jdk/pull/8300
Re: RFR: 8284910: Buffer clean in PasswordCallback [v3]
> 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: cleanup the previous password - Changes: - all: https://git.openjdk.java.net/jdk/pull/8272/files - new: https://git.openjdk.java.net/jdk/pull/8272/files/73692489..324f89fc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8272=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8272=01-02 Stats: 6 lines in 1 file changed: 6 ins; 0 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 [v2]
On Tue, 19 Apr 2022 14:35:37 GMT, Xue-Lei Andrew Fan wrote: >> Yes, I suppose that is a good enough reason, although this class never had a >> finalizer AFAIK. Won't there be a small performance hit (perhaps negligible) >> for code that already calls `clearPassword`? A specification clarification >> would provide clarity to applications that they do not have to call >> `clearPassword` in between calls to `setPassword`. Something as simple as: >> "This method clears the value of any previously stored password before >> storing the input password". > >> If `setPassword` is called twice in succession, should the previous password >> be cleaned before the new one is assigned and registered? > > Awesome, thank you! That what I want to archive while I filed the bug, but > did not get an idea about how to clean the existing passwords during > cleanup. It's pretty simple and straightforward to get it done in the > setPassword. > 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. - PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284910: Buffer clean in PasswordCallback [v2]
On Mon, 18 Apr 2022 17:45:22 GMT, Sean Mullan 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 > 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. BTW, it may be nice to clarify the method relationship behaviors of PasswordCallback. It may be worthy of another PR. > Even if so, then @stuart-marks suggestion about clearing interim passwords is > relevant and this solution seems incomplete. You are right. @stuart-marks suggestion makes sense to me. I will make the update accordingly. - PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284910: Buffer clean in PasswordCallback [v2]
On Mon, 18 Apr 2022 20:04:05 GMT, Sean Mullan wrote: >> Yes, exactly. I'd recommend it calling `cleanable.clean()` prior to storing >> the new password, so that the cleaning action for the old password is >> explicitly and immediately unregistered. > > Yes, I suppose that is a good enough reason, although this class never had a > finalizer AFAIK. Won't there be a small performance hit (perhaps negligible) > for code that already calls `clearPassword`? A specification clarification > would provide clarity to applications that they do not have to call > `clearPassword` in between calls to `setPassword`. Something as simple as: > "This method clears the value of any previously stored password before > storing the input password". > If `setPassword` is called twice in succession, should the previous password > be cleaned before the new one is assigned and registered? Awesome, thank you! That what I want to archive while I filed the bug, but did not get an idea about how to clean the existing passwords during cleanup. It's pretty simple and straightforward to get it done in the setPassword. - PR: https://git.openjdk.java.net/jdk/pull/8272
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
On Thu, 14 Apr 2022 21:05:06 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 two
> additional commits since the last revision:
>
> - add more factory methods, update copyright
> - return DEFAULT also when user constraints are null
Thanks for the update. It looks good to me, except comments for the coding
style.
src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line
94:
> 92: * @return a SSLAlgorithmConstraints instance
> 93: */
> 94: static AlgorithmConstraints forSocket(SSLSocket socket,
I like the idea to use a static method for the construction. What do you think
if the same coding style is used for other constructors, by making them private
and add forSocket() methods accordingly? For example, changing the following
constructor to a private method.
SSLAlgorithmConstraints(SSLEngine engine, String[] supportedAlgorithms,
boolean withDefaultCertPathConstraints) {
src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line
118:
> 116: if (userSpecifiedConstraints == null) {
> 117: return withDefaultCertPathConstraints ? DEFAULT :
> DEFAULT_SSL_ONLY;
> 118: }
It looks like a partial duplicate of nullIfDefault(). What do you think if
merging the logic into nullIfDefault()? Or even merging nullIfDefault() logic
into the getUserSpecifiedConstraints() method? New parameters may be required
for the getUserSpecifiedConstraints() methods.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
On Wed, 13 Apr 2022 06:46:51 GMT, Xue-Lei Andrew Fan wrote: >> Daniel Jeliński has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - add more factory methods, update copyright >> - return DEFAULT also when user constraints are null > > Nice catch. Thank you! > hi @XueleiFan I'd appreciate your approval here. Thanks! Sorry, I missed the notification emails. I will have a look today. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v6]
On Tue, 19 Apr 2022 00:14:06 GMT, Valerie Peng wrote:
>> Xue-Lei Andrew Fan has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update signatures for native code
>
> src/jdk.crypto.cryptoki/unix/native/libj2pkcs11/p11_md.c line 274:
>
>> 272: ModuleData *moduleData = jlong_to_ptr(ckpNativeData);
>> 273:
>> 274: if (moduleData != NULL && moduleData->hModule != NULL) {
>
> The moduleData != NULL check seems to duplicate with the line 271? Otherwise
> looks fine.
Hm, I see. Updated.
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v7]
> This is an effort to fix a problem introduced in the fix for
> [JDK-8284368](https://bugs.openjdk.java.net/browse/JDK-8284368), which
> replaced the finalizers in jdk.crypto.cryptoki with Cleaners. However, there
> is a problem with the code changes. The Runnables registered with Cleaner
> refer to the object being registered ('this'). Meaning, the Cleaner mechanism
> will keep the objects reachable, preventing them from being cleaned and
> collected.
Xue-Lei Andrew Fan has updated the pull request incrementally with one
additional commit since the last revision:
remove duplicated checking
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8248/files
- new: https://git.openjdk.java.net/jdk/pull/8248/files/71b957fe..0de98b92
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8248=06
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8248=05-06
Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/8248.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8248/head:pull/8248
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284855: Update needed to Cleaners added to jdk.crypto.cryptoki [v5]
On Tue, 19 Apr 2022 00:12:26 GMT, Brent Christian wrote:
>> I think it is safer to add the check for 'hModule'.
>>
>>
>> -if (moduleData != NULL) {
>> +if (moduleData != NULL && moduleData->hModule != NULL) {
>
> That is very safe -- we already checked that `ckpNativeData != 0L`
Hm, I see now.
-
PR: https://git.openjdk.java.net/jdk/pull/8248
Re: RFR: 8284933: Improve debug in jdk.crypto.cryptoki
On Mon, 18 Apr 2022 21:54:22 GMT, Valerie Peng wrote: > With this change, I see no reason to have debug(...) method. May as well just > remove it and simply do a "System.out.println(...);"? Yes, that's my preferred style as well. Updated. - PR: https://git.openjdk.java.net/jdk/pull/8277
Re: RFR: 8284933: Improve debug in jdk.crypto.cryptoki [v2]
> Hi, > > May I have the simple update reviewed? > > In the jdk.crypto.cryptoki module implementation, some of the debug > information could be calculated even if the debug is not enabled, which is > not resource friendly. > > Thanks, > Xuelei Xue-Lei Andrew Fan has updated the pull request incrementally with one additional commit since the last revision: Remove debug() method - Changes: - all: https://git.openjdk.java.net/jdk/pull/8277/files - new: https://git.openjdk.java.net/jdk/pull/8277/files/b75a8487..75061bdc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8277=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8277=00-01 Stats: 14 lines in 1 file changed: 0 ins; 4 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/8277.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8277/head:pull/8277 PR: https://git.openjdk.java.net/jdk/pull/8277
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v3]
On Wed, 13 Apr 2022 06:46:51 GMT, Xue-Lei Andrew Fan wrote: >> Daniel Jeliński has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - add more factory methods, update copyright >> - return DEFAULT also when user constraints are null > > Nice catch. Thank you! hi @XueleiFan I'd appreciate your approval here. Thanks! - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes
- Original Message -
> From: "liach"
> To: "core-libs-dev" , "security-dev"
>
> Sent: Tuesday, April 19, 2022 3:31:24 AM
> Subject: Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes
> On Mon, 18 Apr 2022 20:42:48 GMT, Remi Forax wrote:
>
>> The third parameter of defineProxy() is a lambda which is called for each
>> method
>> that can be overriden, either toString/equals/hashCode but also any default
>> methods,
> if the lambda return true, the method is overriden, otherwise the default
> implementation is used.
>
> Not quite, I mean calling default implementations in the super interface,
> consider:
>
> interface Root { void cleanUp(); }
> interface FeatureOne extends Root { default void cleanUp() { /* do something
> */
> } }
> interface FeatureTwo extends Root { default void cleanUp() { /* do something
> */
> } }
>
> My proxy implements both feature one and two, but in your API, there is no way
> for my `cleanUp` to call both `FeatureOne.super.cleanUp();` and
> `FeatureTwo.super.cleanUp();`. You should probably expose the lookup in your
> linker too, especially that you enabled nest access and you can just use that
> lookup to resolve, say, an implementation static method in the proxy user
> class.
yes, you are right, i should send the Lookup too.
>
> Also the "delegate" in your API would significantly benefit from
> https://bugs.openjdk.java.net/browse/JDK-8282798 (#7744), too.
I don't think i need the carrier API, the idea is to have only one field in the
proxy, this field can be a value type (exactly a primitive class) in the
future, so being expanded into multiple fields by the VM at runtime.
>
> -
>
> PR: https://git.openjdk.java.net/jdk/pull/8278
