Re: RFR: 8297065: DerOutputStream operations should not throw IOExceptions [v2]
On Wed, 23 Nov 2022 19:48:48 GMT, Weijun Wang wrote:
>> This is mostly cleanup. Everything around `DerOutputStream` no longer throws
>> an `IOException`. This includes:
>>
>> - Remove unnecessary `throws IOException` in code and spec
>> - Remove `catch (IOException ioe)` blocks. If new exception is thrown,
>> remove further `throws` in code and spec
>> - More classes implementing `DerEncoder` if they have a
>> `encode(DerOutputStream)` method
>> - Modify `write(bytes)` to `writeBytes(bytes)` to avoid IOE
>> - Some unused methods removed
>> - `DerOutputStream` is final
>>
>> The only actual changes (still trivial) are:
>> - Since `PKCS7::encodeSignedData(OutputStream out)` is removed, its usages
>> in `PKCS7::constructToken` and `TsaSigner::createResponse` (in test) are
>> modified to use another method with a `DerOutputStream` argument.
>> - In PKCS8Key, since `getEncodedInternal` never returns non-null, remove
>> null check on result
>> - Since `DerOutputStream` is final, no need to clone bytes in `encode` of
>> `X509CertImpl` and `X509CRLImpl`.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> one more
src/java.base/share/classes/sun/security/x509/RDN.java line 336:
> 334: *
> 335: * @param out DerOutputStream to which RDN is to be written
> 336: * @throws IOException on error
should remove this line?
src/java.base/share/classes/sun/security/x509/SerialNumber.java line 111:
> 109: *
> 110: * @param out the DerOutputStream to marshal the contents to.
> 111: * @exception IOException on errors.
should remove this line?
src/java.base/share/classes/sun/security/x509/UniqueIdentity.java line 95:
> 93: * @param out the DerOutputStream to marshal the contents to.
> 94: * @param tag encode it under the following tag.
> 95: * @exception IOException on errors.
should remove this line?
src/java.base/share/classes/sun/security/x509/X509CRLImpl.java line 298:
> 296: * Encodes the "to-be-signed" TBSCertList to the OutputStream.
> 297: *
> 298: * @exception CRLException on encoding errors.
should remove this line?
src/java.base/share/classes/sun/security/x509/X509CRLImpl.java line 300:
> 298: * @exception CRLException on encoding errors.
> 299: */
> 300: public byte[] encodeInfo() throws CRLException {
"throws CRLException" can be removed?
src/java.base/share/classes/sun/security/x509/X509CRLImpl.java line 601:
> 599: */
> 600: public static X509CRLImpl newSigned(TBSCertList info, PrivateKey
> key, String algorithm, String provider)
> 601: throws CRLException, NoSuchAlgorithmException,
> InvalidKeyException,
Does this method still has calls which throw CRLException?
-
PR: https://git.openjdk.org/jdk/pull/11302
Re: RFR: 8297065: DerOutputStream operations should not throw IOExceptions [v2]
On Wed, 23 Nov 2022 19:48:48 GMT, Weijun Wang wrote: >> This is mostly cleanup. Everything around `DerOutputStream` no longer throws >> an `IOException`. This includes: >> >> - Remove unnecessary `throws IOException` in code and spec >> - Remove `catch (IOException ioe)` blocks. If new exception is thrown, >> remove further `throws` in code and spec >> - More classes implementing `DerEncoder` if they have a >> `encode(DerOutputStream)` method >> - Modify `write(bytes)` to `writeBytes(bytes)` to avoid IOE >> - Some unused methods removed >> - `DerOutputStream` is final >> >> The only actual changes (still trivial) are: >> - Since `PKCS7::encodeSignedData(OutputStream out)` is removed, its usages >> in `PKCS7::constructToken` and `TsaSigner::createResponse` (in test) are >> modified to use another method with a `DerOutputStream` argument. >> - In PKCS8Key, since `getEncodedInternal` never returns non-null, remove >> null check on result >> - Since `DerOutputStream` is final, no need to clone bytes in `encode` of >> `X509CertImpl` and `X509CRLImpl`. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > one more src/java.base/share/classes/sun/security/x509/X509CertImpl.java line 467: > 465: */ > 466: public static X509CertImpl newSigned(X509CertInfo info, PrivateKey > key, String algorithm, String provider) > 467: throws CertificateException, NoSuchAlgorithmException, The javadoc above states that CertificateException is thrown on encoding errors. Will there still be encoding errors? The updated code seems not. src/java.base/share/classes/sun/security/x509/X509CertInfo.java line 148: > 146: * @param out an output stream to which the certificate is appended. > 147: * @exception CertificateException on encoding errors. > 148: * @exception IOException on other errors. should remove this line? - PR: https://git.openjdk.org/jdk/pull/11302
Re: RFR: 8297065: DerOutputStream operations should not throw IOExceptions [v2]
On Wed, 23 Nov 2022 19:48:48 GMT, Weijun Wang wrote: >> This is mostly cleanup. Everything around `DerOutputStream` no longer throws >> an `IOException`. This includes: >> >> - Remove unnecessary `throws IOException` in code and spec >> - Remove `catch (IOException ioe)` blocks. If new exception is thrown, >> remove further `throws` in code and spec >> - More classes implementing `DerEncoder` if they have a >> `encode(DerOutputStream)` method >> - Modify `write(bytes)` to `writeBytes(bytes)` to avoid IOE >> - Some unused methods removed >> - `DerOutputStream` is final >> >> The only actual changes (still trivial) are: >> - Since `PKCS7::encodeSignedData(OutputStream out)` is removed, its usages >> in `PKCS7::constructToken` and `TsaSigner::createResponse` (in test) are >> modified to use another method with a `DerOutputStream` argument. >> - In PKCS8Key, since `getEncodedInternal` never returns non-null, remove >> null check on result >> - Since `DerOutputStream` is final, no need to clone bytes in `encode` of >> `X509CertImpl` and `X509CRLImpl`. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > one more src/java.base/share/classes/sun/security/x509/X509Key.java line 321: > 319: * Returns the DER-encoded form of the key as a byte array. > 320: * > 321: * @exception InvalidKeyException on encoding errors. should remove this line? - PR: https://git.openjdk.org/jdk/pull/11302
Re: RFR: 8297065: DerOutputStream operations should not throw IOExceptions [v2]
On Wed, 23 Nov 2022 19:48:48 GMT, Weijun Wang wrote:
>> This is mostly cleanup. Everything around `DerOutputStream` no longer throws
>> an `IOException`. This includes:
>>
>> - Remove unnecessary `throws IOException` in code and spec
>> - Remove `catch (IOException ioe)` blocks. If new exception is thrown,
>> remove further `throws` in code and spec
>> - More classes implementing `DerEncoder` if they have a
>> `encode(DerOutputStream)` method
>> - Modify `write(bytes)` to `writeBytes(bytes)` to avoid IOE
>> - Some unused methods removed
>> - `DerOutputStream` is final
>>
>> The only actual changes (still trivial) are:
>> - Since `PKCS7::encodeSignedData(OutputStream out)` is removed, its usages
>> in `PKCS7::constructToken` and `TsaSigner::createResponse` (in test) are
>> modified to use another method with a `DerOutputStream` argument.
>> - In PKCS8Key, since `getEncodedInternal` never returns non-null, remove
>> null check on result
>> - Since `DerOutputStream` is final, no need to clone bytes in `encode` of
>> `X509CertImpl` and `X509CRLImpl`.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> one more
src/java.security.jgss/share/classes/sun/security/jgss/spnego/NegTokenTarg.java
line 77:
> 75: }
> 76:
> 77: final byte[] encode() throws GSSException {
Probably no need for GSSException?
src/jdk.crypto.ec/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 99:
> 97: }
> 98:
> 99: private void makeEncoding(byte[] s) throws InvalidKeyException {
No need for the InvalidKeyException?
src/jdk.crypto.ec/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 113:
> 111: }
> 112:
> 113: private void makeEncoding(BigInteger s) throws InvalidKeyException {
No need for the InvalidKeyException?
-
PR: https://git.openjdk.org/jdk/pull/11302
Re: RFR: 8288050: Add support of SHA-512/224 and SHA-512/256 to the PBKDF2 and PBES2 impls in SunJCE provider
On Thu, 24 Nov 2022 20:13:20 GMT, Andrey Turbanov wrote:
>> This RFE enhances existing PBE algorithms with the "SHA512/224" and
>> "SHA512/256" support.
>> Current transformation parsing in javax.crypto.Cipher class is re-written to
>> handle the additional "/" in the "SHA512/224" and "SHA512/256" algorithm
>> names. Existing tests are updated with the additional new algorithms.
>
> src/java.base/share/classes/com/sun/crypto/provider/PBEKeyFactory.java line
> 180:
>
>> 178: public static final class PBEWithHmacSHA512_256AndAES_128
>> 179: extends PBEKeyFactory {
>> 180: public PBEWithHmacSHA512_256AndAES_128() {
>
> Nit:
> Suggestion:
>
> public PBEWithHmacSHA512_256AndAES_128() {
Sure, will fix.
-
PR: https://git.openjdk.org/jdk/pull/11339
Re: RFR: 8288050: Add support of SHA-512/224 and SHA-512/256 to the PBKDF2 and PBES2 impls in SunJCE provider [v2]
> This RFE enhances existing PBE algorithms with the "SHA512/224" and > "SHA512/256" support. > Current transformation parsing in javax.crypto.Cipher class is re-written to > handle the additional "/" in the "SHA512/224" and "SHA512/256" algorithm > names. Existing tests are updated with the additional new algorithms. Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: remove extra whitespace - Changes: - all: https://git.openjdk.org/jdk/pull/11339/files - new: https://git.openjdk.org/jdk/pull/11339/files/5ca95f82..3bb914b1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11339&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11339&range=00-01 Stats: 39 lines in 1 file changed: 0 ins; 9 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/11339.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11339/head:pull/11339 PR: https://git.openjdk.org/jdk/pull/11339
RFR: 8297683: Use enhanced-for cycle instead of Enumeration in java.security.jgss
java.util.Enumeration is a legacy interface from java 1.0. There are a few places with cycles which use it to iterate over collections. We can replace this manual cycle with enchanced-for, which is shorter and easier to read. sun.security.jgss.ProviderList#addAllMechsFromProvider sun.security.jgss.GSSCredentialImpl#dispose sun.security.jgss.GSSCredentialImpl#getRemainingLifetime sun.security.jgss.GSSCredentialImpl#getUsage() sun.security.jgss.GSSCredentialImpl#getElements - Commit messages: - 8297683: Use enhanced-for cycle instead of Enumeration in java.security.jgss Changes: https://git.openjdk.org/jdk/pull/11377/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=11377&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8297683 Stats: 31 lines in 2 files changed: 0 ins; 23 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/11377.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11377/head:pull/11377 PR: https://git.openjdk.org/jdk/pull/11377
Re: RFR: 8296142: CertAttrSet::(getName|getElements|delete) are mostly useless
On Tue, 1 Nov 2022 13:34:40 GMT, Weijun Wang wrote:
> This code change removes `getElements()` and `delete()` from `CertAttrSet`
> and all its subclasses. The `getName()` method is moved to `s.s.x.Extension`
> since it's only useful there.
>
> Except for removing lines, the only place where real changes are made are:
>
> 1. `getName()` in `Extension`.
> 2. No need to cast to into a `CertAttrSet` object in
> `CertificateExtensions::parseExtension` at
> https://github.com/openjdk/jdk/pull/10934/files#diff-0b9f279d1a1537d66ce5adf4e2abcff9a15d4905f500539a6cf52e9f6710aef7R108-R109,
> since `getName()` is in `Extension` now.
> 3. No need to call different methods `CertAttrSet::getName` and
> `Extension::getExtensionId::toString` depending on the types in
> `CertificateExtensions::equals` at
> https://github.com/openjdk/jdk/pull/10934/files#diff-0b9f279d1a1537d66ce5adf4e2abcff9a15d4905f500539a6cf52e9f6710aef7L298-L303,
> since the default implementation in `Extension::getName` is already
> returning the OID.
>
> The last 2 points are also true for `CRLExtensions`.
src/java.base/share/classes/sun/security/x509/NetscapeCertTypeExtension.java
line 281:
> 279: */
> 280: public Enumeration getElements() {
> 281: return mAttributeNames.elements();
Looks like we can drop `mAttributeNames` now. It was used only here.
-
PR: https://git.openjdk.org/jdk/pull/10934
Re: RFR: 8297519: Improve expressions and modernise code
On Thu, 24 Nov 2022 08:24:37 GMT, Per Minborg wrote:
> During the work of another PR (https://github.com/openjdk/jdk/pull/11260),
> several improvement areas were identified. These are now adressed in this
> separate PR proposing the use of more modern Java constructs as well as
> simplifying a large number of logical expressions that were previously
> non-normative.
>
>
> This branch has been tested and passed tier1-4 tests.
These can all be `switch` expressions:
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java line 487:
> 485: case "noDb" -> nssDbMode = Secmod.DbMode.NO_DB;
> 486: default -> throw excToken("nssDbMode must be one of
> readWrite, readOnly, and noDb:");
> 487: }
Suggestion:
nssDbMode = switch (mode) {
case "readWrite" -> Secmod.DbMode.READ_WRITE;
case "readOnly" -> Secmod.DbMode.READ_ONLY;
case "noDb" -> Secmod.DbMode.NO_DB;
default -> throw excToken("nssDbMode must be one of
readWrite, readOnly, and noDb:");
};
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/Config.java line 1042:
> 1040: case "halt" -> handleStartupErrors = ERR_HALT;
> 1041: default -> throw excToken("Invalid value for
> handleStartupErrors:");
> 1042: }
Suggestion:
handleStartupErrors = switch (val) {
case "ignoreAll" -> ERR_IGNORE_ALL;
case "ignoreMissingLibrary" -> ERR_IGNORE_LIB;
case "halt" -> ERR_HALT;
default -> throw excToken("Invalid value for handleStartupErrors:");
};
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line
241:
> 239: default -> throw new NoSuchAlgorithmException("Unsupported
> mode " + mode);
> 240: }
> 241: return result;
Suggestion:
return switch (mode) {
case "ECB" -> MODE_ECB;
case "CBC" -> {
if (blockSize == 0) {
throw new NoSuchAlgorithmException
("CBC mode not supported with stream ciphers");
}
yield MODE_CBC;
}
case "CTR" -> MODE_CTR;
default -> throw new NoSuchAlgorithmException("Unsupported mode " +
mode);
};
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java line
373:
> 371: // should never happen; checked by Cipher.init()
> 372: throw new AssertionError("Unknown mode: " + opmode);
> 373: }
Suggestion:
encrypt = switch (opmode) {
case Cipher.ENCRYPT_MODE -> true;
case Cipher.DECRYPT_MODE -> false;
case Cipher.WRAP_MODE, Cipher.UNWRAP_MODE -> throw new
UnsupportedOperationException
("Unsupported mode: " + opmode);
default ->
// should never happen; checked by Cipher.init()
throw new AssertionError("Unknown mode: " + opmode);
};
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyWrapCipher.java
line 756:
> 754: throw new AssertionError();
> 755: }
> 756: ;
Empty statement:
Suggestion:
src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11RSACipher.java
line 392:
> 390: default -> throw new ProviderException("internal error");
> 391: }
> 392: return n;
Suggestion:
return switch (mode) {
case MODE_ENCRYPT -> p11.C_Encrypt
(session.id(), 0, buffer, 0, bufOfs, 0, out, outOfs,
outLen);
case MODE_DECRYPT -> p11.C_Decrypt
(session.id(), 0, buffer, 0, bufOfs, 0, out, outOfs,
outLen);
case MODE_SIGN -> {
byte[] tmpBuffer = new byte[bufOfs];
System.arraycopy(buffer, 0, tmpBuffer, 0, bufOfs);
tmpBuffer = p11.C_Sign(session.id(), tmpBuffer);
if (tmpBuffer.length > outLen) {
throw new BadPaddingException(
"Output buffer (" + outLen + ") is too small to
" +
"hold the produced data (" +
tmpBuffer.length + ")");
}
System.arraycopy(tmpBuffer, 0, out, outOfs,
tmpBuffer.length);
yield tmpBuffer.length;
}
case MODE_VERIFY -> p11.C_VerifyRecover
(session.id(), buffer, 0, bufOfs, out, outOfs, outLen);
default -> throw new ProviderException("internal error");
};
-
PR: https://git.openjdk.org/jdk/pull/11348
Re: RFR: 8297065: DerOutputStream operations should not throw IOExceptions [v2]
On Sun, 27 Nov 2022 07:57:35 GMT, Valerie Peng wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> one more
>
> src/java.base/share/classes/sun/security/x509/X509CRLImpl.java line 300:
>
>> 298: * @exception CRLException on encoding errors.
>> 299: */
>> 300: public byte[] encodeInfo() throws CRLException {
>
> "throws CRLException" can be removed?
Inside the method there is
if ((version == 0) && (issuer.toString() == null))
throw new CRLException("Null Issuer DN not allowed in v1 CRL");
> src/java.base/share/classes/sun/security/x509/X509CRLImpl.java line 601:
>
>> 599: */
>> 600: public static X509CRLImpl newSigned(TBSCertList info, PrivateKey
>> key, String algorithm, String provider)
>> 601: throws CRLException, NoSuchAlgorithmException,
>> InvalidKeyException,
>
> Does this method still has calls which throw CRLException?
Because it calls `TBSCertList::encodeInfo`.
> src/java.base/share/classes/sun/security/x509/X509CertImpl.java line 467:
>
>> 465: */
>> 466: public static X509CertImpl newSigned(X509CertInfo info, PrivateKey
>> key, String algorithm, String provider)
>> 467: throws CertificateException, NoSuchAlgorithmException,
>
> The javadoc above states that CertificateException is thrown on encoding
> errors. Will there still be encoding errors? The updated code seems not.
Inside `X509CertInfo::emit`, there is
if ((version.compare(CertificateVersion.V1) == 0) &&
(issuer.toString() == null))
throw new CertificateParsingException(
"Null issuer DN not allowed in v1 certificate");
and more places where the exception might be thrown.
> src/java.security.jgss/share/classes/sun/security/jgss/spnego/NegTokenTarg.java
> line 77:
>
>> 75: }
>> 76:
>> 77: final byte[] encode() throws GSSException {
>
> Probably no need for GSSException?
This is because `Oid::getDER` still throws it. Of course it shouldn't (I've
added a comment on it). I'll see if I can catch it inside this method and
ignore it.
> src/jdk.crypto.ec/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 99:
>
>> 97: }
>> 98:
>> 99: private void makeEncoding(byte[] s) throws InvalidKeyException {
>
> No need for the InvalidKeyException?
`ECParameters::getAlgorithmParameters` throws it.
> src/jdk.crypto.ec/share/classes/sun/security/ec/ECPrivateKeyImpl.java line
> 113:
>
>> 111: }
>> 112:
>> 113: private void makeEncoding(BigInteger s) throws InvalidKeyException {
>
> No need for the InvalidKeyException?
Same as above.
-
PR: https://git.openjdk.org/jdk/pull/11302
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v14]
On Thu, 24 Nov 2022 16:21:42 GMT, Per Minborg wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> try-with-resources friendly access method was added. This made is more easy
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas
>> in certain fundamental JDK classes might incur bootstrap problems.
>>
>> The aforementioned TwR is using a "session acquisition" that is not used
>> explicitly in the try block itself. This raises a warning that is suppressed
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove
>> these suppressions by using the reserved variable name `_`. Another
>> alternative was evaluated where a non-autocloseable resource was used.
>> However, it became relatively complicated to guarantee that the session was
>> always released and, at the same time, the existing 'final` block was always
>> executed properly (as they both might throw exceptions). In the end, the
>> proposed solution was much simpler and robust despite requiring a
>> non-referenced TwR variable.
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> ~~These cases have been documented in the code.~~
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reformat and fix comment
The latest version (30aff118) looks quite good but the naming at the use sites
(`var scope = NIO_ACCESS.acquireSession(buffer);`) is temporarily confusing.
Will that be fixed with the JEP 434 integrated or soon afterwards?
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8296024: Usage of DIrectBuffer::address should be guarded [v14]
On Thu, 24 Nov 2022 16:21:42 GMT, Per Minborg wrote:
>> This PR proposes the introduction of **guarding** of the use of
>> `DirectBuffer::address` within the JDK. With the introduction of the Foreign
>> Function and Memory access, it is possible to derive Buffer instances that
>> are backed by native memory that, in turn, can be closed asynchronously by
>> the user (and not only via a `Cleaner` when there is no other reference to
>> the `Buffer` object). If another thread is invoking `MemorySession::close`
>> while a thread is doing work using raw addresses, the outcome is undefined.
>> This means the JVM might crash or even worse, silent modification of
>> unrelated memory might occur.
>>
>> Design choices in this PR:
>>
>> There is already a method `MemorySession::whileAlive` that takes a runnable
>> and that will perform the provided action while acquiring the underlying`
>> MemorySession` (if any). However, using this method entailed relatively
>> large changes while converting larger/nested code segments into lambdas.
>> This would also risk introducing lambda capturing. So, instead, a
>> try-with-resources friendly access method was added. This made is more easy
>> to add guarding and did not risk lambda capturing. Also, introducing lambdas
>> in certain fundamental JDK classes might incur bootstrap problems.
>>
>> The aforementioned TwR is using a "session acquisition" that is not used
>> explicitly in the try block itself. This raises a warning that is suppressed
>> using `@SuppressWarnings("try")`. In the future, we might be able to remove
>> these suppressions by using the reserved variable name `_`. Another
>> alternative was evaluated where a non-autocloseable resource was used.
>> However, it became relatively complicated to guarantee that the session was
>> always released and, at the same time, the existing 'final` block was always
>> executed properly (as they both might throw exceptions). In the end, the
>> proposed solution was much simpler and robust despite requiring a
>> non-referenced TwR variable.
>>
>> In some cases, where is is guaranteed that the backing memory session is
>> non-closeable, we do not have to guard the use of `DirectBuffer::address`.
>> ~~These cases have been documented in the code.~~
>>
>> On some occasions, a plurality of acquisitions are made. This would never
>> lead to deadlocks as acquisitions are fundamentally concurrent counters and
>> not resources that only one thread can "own".
>>
>> I have added comments (and not javadocs) before the declaration of the
>> non-public-api `DirectBuffer::address` method, that the use of the returned
>> address needs to be guarded. It can be discussed if this is preferable or
>> not.
>>
>> This PR spawns several areas of responsibility and so, I expect more than
>> one reviewer before promoting the PR.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Reformat and fix comment
src/java.base/share/classes/java/util/zip/Deflater.java line 594:
> 592: try {
> 593: result = deflateBufferBytes(zsRef.address(),
> 594: ((DirectBuffer) input).address() +
> inputPos, inputRem,
Somewhat subjective but the original code, with inputAddress, was a easier to
read and avoided having too much going on in the parameters to
deflateBufferBytes. In any case, you probably should fix the formatting to
avoid have different parameters with different indentations.
src/java.base/share/classes/java/util/zip/Deflater.java line 717:
> 715: result = deflateBytesBuffer(zsRef.address(),
> 716: inputArray, inputPos, inputLim - inputPos,
> 717: ((DirectBuffer) output).address() +
> outputPos, outputRem,
Same here, I think I would keep outputAddress to keep the call to
deflateByteBuffer simple.
src/java.base/share/classes/java/util/zip/Deflater.java line 740:
> 738: try {
> 739: result =
> deflateBufferBuffer(zsRef.address(),
> 740: ((DirectBuffer) input).address() +
> inputPos, inputRem,
Another one where inputAddress and outputAddress have been removed so the call
to deflatBufferBuffer is much hard to digest.
-
PR: https://git.openjdk.org/jdk/pull/11260
Re: RFR: 8294321: Fix typos in files under test/jdk/java, test/jdk/jdk, test/jdk/jni [v2]
On Mon, 26 Sep 2022 16:51:36 GMT, Michael Ernst wrote: >> 8294321: Fix typos in files under test/jdk/java, test/jdk/jdk, test/jdk/jni > > Michael Ernst has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains six commits: > > - Reinstate typos in Apache code that is copied into the JDK > - Merge ../jdk-openjdk into typos-typos > - Remove file that was removed upstream > - Fix inconsistency in capitalization > - Undo change in zlip > - Fix typos Could someone who knows the undocumented ins and outs of creating JDK pull requests could split this pull request up into multiple PRs? Then it can be merged, rather than wasting all the effort that went into it. - PR: https://git.openjdk.org/jdk/pull/10029
Re: RFR: 8295010: Reduce if required in EC limbs operations [v6]
On Wed, 23 Nov 2022 06:21:05 GMT, Xue-Lei Andrew Fan wrote: >> Hi, >> >> May I have this update reviewed? With this update, the result will be >> reduced if required in EC limbs operations in the JDK implementation. >> >> In the current implementation, the EC limbs addition and subtraction result >> is not reduced before the value is returned. This behavior is different >> from multiplication and square. To get it right, a maximum addition limit >> is introduced for EC limbs addition and subtraction. If the limit is >> reached, an exception will be thrown so as to indicate an EC implementation >> problem. The exception is not recoverable from the viewpoint of an >> application. >> >> The design is error prone as the EC implementation code must be carefully >> checked so that the limit cannot reach. If a reach is noticed, a reduce >> operation would have to be hard-coded additionally. In the FieldGen.java >> implementation, the limit can only be 1 or 2 as the implementation code is >> only able to handle 2. But the FieldGen.java does not really check if 1 or >> 2 really works for the specific filed generation parameters. >> >> The design impact the performance as well. Because of this limit, the >> maximum limb size is 28 bits for 2 max adding limit. Otherwise there are >> integer (long) overflow issues. For example for 256 bits curves, 10 limbs >> are required for 28-bit limb size; and 9 limbs for 29-bit size. By reducing >> 1 limbs from 10 to 9, the Signature performance could get improved by 20%. >> >> In the IntegerPolynomial class description, it is said "All >> IntegerPolynomial implementations allow at most one addition before >> multiplication. Additions after that will result in an ArithmeticException." >> It's too strict to follow without exam the code very carefully. Indeed, the >> implementation does not really follow the spec, and 2 addition may be >> allowed. >> >> It would be nice if there is no addition limit, and then we are free from >> these issues. It is doable by reducing the limbs if required for EC limbs >> operations. >> >> The benchmark for ECDSA Signature is checked in order to see how much the >> performance could be impacted. Here are the benchmark numbers before the >> patch applied: >> >> Benchmark(curveName) (messageLength) Mode Cnt ScoreError >> Units >> Signatures.signsecp256r1 64 thrpt 15 1417.507 ± 5.809 >> ops/s >> Signatures.signsecp256r1 512 thrpt 15 1412.620 ± 7.040 >> ops/s >> Signatures.signsecp256r1 2048 thrpt 15 1417.364 ± 6.643 >> ops/s >> Signatures.signsecp256r116384 thrpt 15 1364.720 ± 44.354 >> ops/s >> Signatures.signsecp384r1 64 thrpt 15 609.924 ± 9.858 >> ops/s >> Signatures.signsecp384r1 512 thrpt 15 610.873 ± 5.519 >> ops/s >> Signatures.signsecp384r1 2048 thrpt 15 608.573 ± 13.324 >> ops/s >> Signatures.signsecp384r116384 thrpt 15 597.802 ± 7.074 >> ops/s >> Signatures.signsecp521r1 64 thrpt 15 301.954 ± 6.449 >> ops/s >> Signatures.signsecp521r1 512 thrpt 15 300.774 ± 5.849 >> ops/s >> Signatures.signsecp521r1 2048 thrpt 15 295.831 ± 8.423 >> ops/s >> Signatures.signsecp521r116384 thrpt 15 276.258 ± 43.146 >> ops/s >> Signatures.sign Ed25519 64 thrpt 15 1171.878 ± 4.187 >> ops/s >> Signatures.sign Ed25519 512 thrpt 15 1175.175 ± 4.611 >> ops/s >> Signatures.sign Ed25519 2048 thrpt 15 1168.459 ± 5.750 >> ops/s >> Signatures.sign Ed2551916384 thrpt 15 1086.906 ± 6.514 >> ops/s >> Signatures.signEd448 64 thrpt 15 326.475 ± 3.274 >> ops/s >> Signatures.signEd448 512 thrpt 15 328.709 ± 3.867 >> ops/s >> Signatures.signEd448 2048 thrpt 15 315.480 ± 15.377 >> ops/s >> Signatures.signEd44816384 thrpt 15 312.388 ± 1.067 >> ops/s >> >> >> and here are the numbers with this patch applied: >> >> Benchmark(curveName) (messageLength) Mode Cnt ScoreError >> Units >> Signatures.signsecp256r1 64 thrpt 15 1377.447 ± 38.889 >> ops/s >> Signatures.signsecp256r1 512 thrpt 15 1403.149 ± 5.151 >> ops/s >> Signatures.signsecp256r1 2048 thrpt 15 1401.101 ± 6.937 >> ops/s >> Signatures.signsecp256r116384 thrpt 15 1381.855 ± 10.606 >> ops/s >> Signatures.signsecp384r1 64 thrpt 15 595.979 ± 1.967 >> ops/s >> Signatures.signsecp384r1 512 thrpt 15 592.419 ± 6.087 >> ops/s >> Signatures.signsecp384r1 2048 thrpt 15 581.800 ± 18.815 >> ops/s
