Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
On Thu, 17 Jun 2021 10:19:43 GMT, Alan Bateman wrote:
>> Yi Yang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> restore IndexOfOufBoundsException; split exception line
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 471:
>
>> 469: */
>> 470: public int offsetByCodePoints(int index, int codePointOffset) {
>> 471: checkOffset(index, count);
>
> String.offsetByCodePoints is specified to throw IOOBE. checkOffset may throw
> the more specific StringIndexOutOfBoundsException. That's a compatible change
> but I worry that we might want to throw the less specific exception in the
> future. I only mention it because there have been cases in java.lang where
> IOOBE was specified and AIOOBE by the implementation and it has been
> problematic to touch the implementation as a result.
Yes, changing the type of thrown exception may break something. And as David
said, this also requires a CSR approval, which is a relatively long process, so
I restored the original code.
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible [v2]
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex > by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in > the whole JDK codebase. Yi Yang has updated the pull request incrementally with one additional commit since the last revision: restore IndexOfOufBoundsException; split exception line - Changes: - all: https://git.openjdk.java.net/jdk/pull/4507/files - new: https://git.openjdk.java.net/jdk/pull/4507/files/593bf995..ec0a4d44 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4507=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4507=00-01 Stats: 30 lines in 8 files changed: 15 ins; 2 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/4507.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4507/head:pull/4507 PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8266182: Create a manual test for jdk/sun/security/pkcs12/ParamsTest.java [v2]
> ParamsTest is an interop test between keytool <-> openssl. There are some > manual steps listed in jdk/sun/security/pkcs12/params/README to perform after > the execution of jtreg execution. So this test is to perform that manual > steps. Abdul Kolarkunnu has updated the pull request incrementally with one additional commit since the last revision: 8266182: Create a manual test for jdk/sun/security/pkcs12/ParamsTest.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/4413/files - new: https://git.openjdk.java.net/jdk/pull/4413/files/4b866628..4969232f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4413=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4413=00-01 Stats: 102 lines in 3 files changed: 60 ins; 35 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/4413.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4413/head:pull/4413 PR: https://git.openjdk.java.net/jdk/pull/4413
Re: RFR: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance [v2]
On Fri, 18 Jun 2021 04:56:25 GMT, Dongbo He wrote: >> The checkAlgorithm is using equalsIgnoreCase(), so it is safe for it. My >> concern is mainly about the keywords, like "keySize" used the property, not >> really the algorithm name. It is good to keep the current case sensitive >> checking behavior unchanged. > > Hi, Xuelei, thank you for your comments. I may not express it clearly, let me > clarify. My concern is that if we use the HashSet:contains method to check > whether an item is in the hash set, we cannot use equalsIgnoreCase(), so I > used `new TreeSet<>(String.CASE_INSENSITIVE_ORDER)` that can support > equalsIgnoreCase(). > > According to my understanding, the current checkAlgorithm is not case > sensitive, because it ignores the case of the item being checked. Looking > forward to your suggestions。 I see your point now. Thank you for the clarification. I need more time to think about if there is an improvement. I will be back. - PR: https://git.openjdk.java.net/jdk/pull/4424
Re: RFR: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance [v2]
On Fri, 18 Jun 2021 04:14:09 GMT, Xue-Lei Andrew Fan wrote: >> [checkAlgorithm](https://github.com/openjdk/jdk/blob/a051e735cda0d5ee5cb6ce0738aa549a7319a28c/src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java#L94) >> check whether the item is in the collection by ignoring case. If the item >> in the HashSet is case-sensitive, the method will lose its original >> algorithmic logic, but will retain it by using a ` new >> TreeSet<>(String.CASE_INSENSITIVE_ORDER);` >> >> Can we use case sensitivity in checkAlgorithm to check an algorithm? > > The checkAlgorithm is using equalsIgnoreCase(), so it is safe for it. My > concern is mainly about the keywords, like "keySize" used the property, not > really the algorithm name. It is good to keep the current case sensitive > checking behavior unchanged. Hi, Xuelei, thank you for your comments. I may not express it clearly, let me clarify. My concern is that if we use the HashSet:contains method to check whether an item is in the hash set, we cannot use equalsIgnoreCase(), so I used `new TreeSet<>(String.CASE_INSENSITIVE_ORDER)` that can support equalsIgnoreCase(). According to my understanding, the current checkAlgorithm is not case sensitive, because it ignores the case of the item being checked. Looking forward to your suggestions。 - PR: https://git.openjdk.java.net/jdk/pull/4424
Re: RFR: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance [v2]
On Fri, 18 Jun 2021 01:54:30 GMT, Dongbo He wrote: >> Sorry, I missed a "case" in the original comment (corrected). I meant to >> keep the property case sensitive in the hash set so that the keywords like >> "keySize" could be used correctly. > > [checkAlgorithm](https://github.com/openjdk/jdk/blob/a051e735cda0d5ee5cb6ce0738aa549a7319a28c/src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java#L94) > check whether the item is in the collection by ignoring case. If the item in > the HashSet is case-sensitive, the method will lose its original algorithmic > logic, but will retain it by using a ` new > TreeSet<>(String.CASE_INSENSITIVE_ORDER);` > > Can we use case sensitivity in checkAlgorithm to check an algorithm? The checkAlgorithm is using equalsIgnoreCase(), so it is safe for it. My concern is mainly about the keywords, like "keySize" used the property, not really the algorithm name. It is good to keep the current case sensitive checking behavior unchanged. - PR: https://git.openjdk.java.net/jdk/pull/4424
Re: JEP 411: Deprecation with removal would break most existing Java libraries
On 18/06/2021 1:18 pm, Peter Firmstone wrote:
On 16/06/2021 11:18 pm, David Lloyd wrote:
On Mon, Jun 14, 2021 at 6:47 PM Peter Firmstone
wrote:
Permission references can be replaced with Guard references (which
Permissions are instances of).
I guess you've got something fairly complex in mind, could you give
some practical examples of how this would work?
The same service provider mechanism encryption uses. So
implementation may utilize authorization access check points without
any dependencies on current SecurityManager, Policy or Permission API's.
Eg GuardFactorySpi
It's completely up to the implementation to determine how to manage.
The Permission implementations of Guard::check call SecurityManager, so
checks will continue working as expected, but it allows us to intercept
them and do something different.
What do you envision these checks looking like? Where would the JDK
find these Guard instances?
GuardFactory socketGuardFactory = GuardFactory.getInstance("SOCKET");
Guard localhostConnectAccept = socketGuardFactory.orders("localhost",
"connect,accept");
// Permission check
localHostConnectAccept.check();
GuardFactory runtimeGuardFactory = GuardFactory.getInstance("RUNTIME");
Guard createClassLoader =
runtimeGuardFactory.orders("createClassLoader", null);
// Permission check
createClassLoader.check();
--
Regards,
Peter Firmstone
0498 286 363
Zeus Project Services Pty Ltd.
Re: JEP 411: Deprecation with removal would break most existing Java libraries
On 16/06/2021 11:18 pm, David Lloyd wrote:
On Mon, Jun 14, 2021 at 6:47 PM Peter Firmstone
wrote:
Permission references can be replaced with Guard references (which
Permissions are instances of).
I guess you've got something fairly complex in mind, could you give
some practical examples of how this would work?
The same service provider mechanism encryption uses. So implementation
may utilize authorization access check points without any dependencies
on current SecurityManager, Policy or Permission API's.
It's completely up to the implementation to determine how to manage.
The Permission implementations of Guard::check call SecurityManager, so
checks will continue working as expected, but it allows us to intercept
them and do something different.
What do you envision these checks looking like? Where would the JDK
find these Guard instances?
GuardFactory socketGuardFactory = GuardFactory.getInstance("SOCKET");
Guard localhostConnectAccept = socketGuardFactory.orders("localhost",
"connect,accept");
// Permission check
localHostConnectAccept.check();
GuardFactory runtimeGuardFactory = GuardFactory.getInstance("RUNTIME");
Guard createClassLoader =
runtimeGuardFactory.orders("createClassLoader", null);
// Permission check
createClassLoader.check();
--
Regards,
Peter Firmstone
0498 286 363
Zeus Project Services Pty Ltd.
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v5]
On Thu, 17 Jun 2021 17:21:04 GMT, Weijun Wang wrote:
>> More loudly and precise warning messages when a security manager is either
>> enabled at startup or installed at runtime.
>>
>> This is new PR for the `openjdk/jdk17` repo copied from
>> https://github.com/openjdk/jdk/pull/4400. A new commit is added.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> verbose warning message test and renaming in System.java
Hello Sean, Weijung,
>From what I have known, the Java/JDK code has always taken extra precaution
>when it comes to printing out potentially sensitive details like IP addresses
>and paths to file, like jar files in the log messages or exception
>stacktraces. In fact, one of the annoying things about some of the error
>messages that the JarFile API throws is that it doesn't even print out the jar
>file name, let alone the full path of the jar file which ran into issues. At
>least that was the case, unless that has changed in recent times. Furthermore,
>as you will surely know, to print out these details there's an security
>property which needs to be explicitly enabled ("jdk.includeInExceptions") with
>the right values.
Given all that, do you think that we should be printing the jar file paths in
this WARNING message by default? I read the linked CSR, but I didn't see why
the location of the jar or the name of the jar would be useful in this warning
message. As long as the caller class (and perhaps the caller method) is
printed, I think that should be enough of a summary on what's triggering this
warning.
-
PR: https://git.openjdk.java.net/jdk17/pull/13
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v5]
On Thu, 17 Jun 2021 17:21:04 GMT, Weijun Wang wrote:
>> More loudly and precise warning messages when a security manager is either
>> enabled at startup or installed at runtime.
>>
>> This is new PR for the `openjdk/jdk17` repo copied from
>> https://github.com/openjdk/jdk/pull/4400. A new commit is added.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> verbose warning message test and renaming in System.java
src/java.base/share/classes/java/lang/System.java line 381:
> 379: if (allowSecurityManager()) {
> 380: var caller = Reflection.getCallerClass();
> 381: String signature = caller.getName() + " (" +
> codeSource(caller) + ")";
Hello Weijun,
Given that the `codeSource()` method above is allowed to return `null`, there
could be a case where the warning message printed would be something like:
>
> WARNING: A terminally deprecated method in java.lang.System has been called
> WARNING: System::setSecurityManager has been called by foo.bar.Hello (null)
> WARNING: Please consider reporting this to the maintainers of foo.bar.Hello
> WARNING: System::setSecurityManager will be removed in a future release
>
Would that be OK or do you think the presence of "(null)" be unnecessary and
confusing? Maybe in such cases that line should just say
"System::setSecurityManager has been called by foo.bar.Hello"?
Another minor nit - the variable is named `signature` which initially gave me
an impression that it was the method signature of the caller code, but that
isn't the case. Should this variable be renamed perhaps?
-
PR: https://git.openjdk.java.net/jdk17/pull/13
Re: RFR: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance [v2]
On Thu, 17 Jun 2021 16:08:15 GMT, Xue-Lei Andrew Fan wrote: >> I did not get the point to use TreeSet. Is it sufficient if the >> toLowerCase() is not added (and don't compare keywords like "keySize" by >> ignoring cases)? >> >> >> - algorithmsInProperty[i] = algorithmsInProperty[i].trim().toLowerCase(...); >> + algorithmsInProperty[i] = algorithmsInProperty[i].trim(); > > Sorry, I missed a "case" in the original comment (corrected). I meant to > keep the property case sensitive in the hash set so that the keywords like > "keySize" could be used correctly. [checkAlgorithm](https://github.com/openjdk/jdk/blob/a051e735cda0d5ee5cb6ce0738aa549a7319a28c/src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java#L94) check whether the item is in the collection by ignoring case. If the item in the HashSet is case-sensitive, the method will lose its original algorithmic logic, but will retain it by using a ` new TreeSet<>(String.CASE_INSENSITIVE_ORDER);` Can we use case sensitivity in checkAlgorithm to check an algorithm? - PR: https://git.openjdk.java.net/jdk/pull/4424
[jdk17] Integrated: 8268678: LetsEncryptCA.java test fails as Let’s Encrypt Authority X3 is retired
On Fri, 18 Jun 2021 00:54:43 GMT, Rajan Halade wrote: > clean backport to JDK 17 > > Reviewed-by: xuelei This pull request has now been integrated. Changeset: 483f1ee2 Author:Rajan Halade URL: https://git.openjdk.java.net/jdk17/commit/483f1ee211bc0e37b486eb9d38d283ff02f0bdcc Stats: 125 lines in 1 file changed: 11 ins; 9 del; 105 mod 8268678: LetsEncryptCA.java test fails as Let’s Encrypt Authority X3 is retired Backport-of: 58e6e6d919cb15559a61a67805da263be3c9d693 - PR: https://git.openjdk.java.net/jdk17/pull/94
[jdk17] Integrated: 8268678: LetsEncryptCA.java test fails as Let’s Encrypt Authority X3 is retired
clean backport to JDK 17 Reviewed-by: xuelei - Commit messages: - 8268678: LetsEncryptCA.java test fails as Let’s Encrypt Authority X3 is retired Changes: https://git.openjdk.java.net/jdk17/pull/94/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17=94=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268678 Stats: 125 lines in 1 file changed: 11 ins; 9 del; 105 mod Patch: https://git.openjdk.java.net/jdk17/pull/94.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/94/head:pull/94 PR: https://git.openjdk.java.net/jdk17/pull/94
Integrated: Merge jdk17
On Thu, 17 Jun 2021 23:26:26 GMT, Jesper Wilhelmsson wrote: > Forwardport JDK 17 -> JDK 18 This pull request has now been integrated. Changeset: a051e735 Author:Jesper Wilhelmsson URL: https://git.openjdk.java.net/jdk/commit/a051e735cda0d5ee5cb6ce0738aa549a7319a28c Stats: 845 lines in 26 files changed: 408 ins; 384 del; 53 mod Merge - PR: https://git.openjdk.java.net/jdk/pull/4525
Integrated: 8268678: LetsEncryptCA.java test fails as Let’s Encrypt Authority X3 is retired
On Thu, 17 Jun 2021 23:10:58 GMT, Rajan Halade wrote: > See the bug for more details. > > - Intermediate root cert R3 doesn't specify OCSP responder and end entity > test certificates doesn't specify CRLs > - New test artifacts are available but revoked expires on July 7th, 2021 and > valid on August 31st, 2021. so backdated validity check is performed for OCSP. > > If this fix is not acceptable for now then we can wait till CA updates test > sites. This pull request has now been integrated. Changeset: 58e6e6d9 Author:Rajan Halade URL: https://git.openjdk.java.net/jdk/commit/58e6e6d919cb15559a61a67805da263be3c9d693 Stats: 125 lines in 1 file changed: 11 ins; 9 del; 105 mod 8268678: LetsEncryptCA.java test fails as Let’s Encrypt Authority X3 is retired Reviewed-by: xuelei - PR: https://git.openjdk.java.net/jdk/pull/4524
Re: RFR: 8268678: LetsEncryptCA.java test fails as Let’s Encrypt Authority X3 is retired
On Thu, 17 Jun 2021 23:10:58 GMT, Rajan Halade wrote: > See the bug for more details. > > - Intermediate root cert R3 doesn't specify OCSP responder and end entity > test certificates doesn't specify CRLs > - New test artifacts are available but revoked expires on July 7th, 2021 and > valid on August 31st, 2021. so backdated validity check is performed for OCSP. > > If this fix is not acceptable for now then we can wait till CA updates test > sites. Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4524
RFR: Merge jdk17
Forwardport JDK 17 -> JDK 18 - Commit messages: - Merge - 8268371: C2: assert(_gvn.type(obj)->higher_equal(tjp)) failed: cast_up is no longer needed - 8268676: assert(!ik->is_interface() && !ik->has_subklass()) failed: inconsistent klass hierarchy - 8268265: MutableSpaceUsedHelper::take_sample() hits assert(left >= right) failed: avoid overflow - 8268971: ProblemList tools/jpackage/windows/WinInstallerIconTest.java on win-x64 - 8264843: Javac crashes with NullPointerException when finding unencoded XML in tag - 8265297: javax/net/ssl/SSLSession/TestEnabledProtocols.java failed with "RuntimeException: java.net.SocketException: Connection reset" - 8268353: Test libsvml.so is and is not present in jdk image - 8249899: jdk/javadoc/tool/InlineTagsWithBraces.java uses @ignore w/o bug-id - 8268776: Test `ADatagramSocket.java` missing /othervm from @run tag - ... and 3 more: https://git.openjdk.java.net/jdk/compare/bb24fa65...a3951c44 The merge commit only contains trivial merges, so no merge-specific webrevs have been generated. Changes: https://git.openjdk.java.net/jdk/pull/4525/files Stats: 845 lines in 26 files changed: 408 ins; 384 del; 53 mod Patch: https://git.openjdk.java.net/jdk/pull/4525.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4525/head:pull/4525 PR: https://git.openjdk.java.net/jdk/pull/4525
[jdk17] Integrated: 8265500: Some impls of javax.crypto.Cipher.init() do not throw UnsupportedOperationExc for unsupported modes
On Tue, 15 Jun 2021 22:37:29 GMT, Valerie Peng wrote: > Could someone please help review this trivial fix? The real changes are the > two PKCS11 cipher impl classes under > src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/. The rest of > classes are just cleanups, e.g. dead code or unused code. The > test/jdk/javax/crypto/Cipher/TestCipherMode.java is updated to cover more > cipher impls for completeness sake. It passes without this fix, so I only add > the bug id to the the other test, i.e. > test/jdk/sun/security/pkcs11/Cipher/TestCipherMode.java, which verifies the > PKCS#11 cipher impls. > > Thanks, > Valerie This pull request has now been integrated. Changeset: 80dc262e Author:Valerie Peng URL: https://git.openjdk.java.net/jdk17/commit/80dc262e8132204d70b184b32978e6c456460fb0 Stats: 308 lines in 8 files changed: 254 ins; 5 del; 49 mod 8265500: Some impls of javax.crypto.Cipher.init() do not throw UnsupportedOperationExc for unsupported modes Reviewed-by: xuelei - PR: https://git.openjdk.java.net/jdk17/pull/69
RFR: 8268678: LetsEncryptCA.java test fails as Let’s Encrypt Authority X3 is retired
See the bug for more details. - Intermediate root cert R3 doesn't specify OCSP responder and end entity test certificates doesn't specify CRLs - New test artifacts are available but revoked expires on July 7th, 2021 and valid on August 31st, 2021. so backdated validity check is performed for OCSP. If this fix is not acceptable for now then we can wait till CA updates test sites. - Commit messages: - 8268678: LetsEncryptCA.java test fails as Let’s Encrypt Authority X3 is retired Changes: https://git.openjdk.java.net/jdk/pull/4524/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4524=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268678 Stats: 125 lines in 1 file changed: 11 ins; 9 del; 105 mod Patch: https://git.openjdk.java.net/jdk/pull/4524.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4524/head:pull/4524 PR: https://git.openjdk.java.net/jdk/pull/4524
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v4]
On Thu, 17 Jun 2021 14:55:02 GMT, Weijun Wang wrote: >> More loudly and precise warning messages when a security manager is either >> enabled at startup or installed at runtime. >> >> This is new PR for the `openjdk/jdk17` repo copied from >> https://github.com/openjdk/jdk/pull/4400. A new commit is added. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > add bug id into a test A new commit. Much more exact checking of various warning messages. - PR: https://git.openjdk.java.net/jdk17/pull/13
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v5]
> More loudly and precise warning messages when a security manager is either > enabled at startup or installed at runtime. > > This is new PR for the `openjdk/jdk17` repo copied from > https://github.com/openjdk/jdk/pull/4400. A new commit is added. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: verbose warning message test and renaming in System.java - Changes: - all: https://git.openjdk.java.net/jdk17/pull/13/files - new: https://git.openjdk.java.net/jdk17/pull/13/files/2d5b1ba5..1e1e7221 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=13=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=13=03-04 Stats: 111 lines in 2 files changed: 51 ins; 30 del; 30 mod Patch: https://git.openjdk.java.net/jdk17/pull/13.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/13/head:pull/13 PR: https://git.openjdk.java.net/jdk17/pull/13
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v4]
On Thu, 17 Jun 2021 16:27:41 GMT, Alan Bateman wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add bug id into a test > > src/java.base/share/classes/java/lang/System.java line 330: > >> 328: >> 329: // Remember original System.err. setSecurityManager() warning goes >> here >> 330: private static volatile @Stable PrintStream originalErrStream = >> null; > > I'd probably use "initial" rather than "original" here but okay. You can drop > setting it to null as that is its default value. Will do. Thanks. - PR: https://git.openjdk.java.net/jdk17/pull/13
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v4]
On Thu, 17 Jun 2021 14:55:02 GMT, Weijun Wang wrote: >> More loudly and precise warning messages when a security manager is either >> enabled at startup or installed at runtime. >> >> This is new PR for the `openjdk/jdk17` repo copied from >> https://github.com/openjdk/jdk/pull/4400. A new commit is added. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > add bug id into a test Implementation changes/text is okay. src/java.base/share/classes/java/lang/System.java line 330: > 328: > 329: // Remember original System.err. setSecurityManager() warning goes > here > 330: private static volatile @Stable PrintStream originalErrStream = null; I'd probably use "initial" rather than "original" here but okay. You can drop setting it to null as that is its default value. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/13
[jdk17] Integrated: 8265297: javax/net/ssl/SSLSession/TestEnabledProtocols.java failed with "RuntimeException: java.net.SocketException: Connection reset"
On Wed, 16 Jun 2021 14:24:31 GMT, Fernando Guallini wrote: > The following test: javax/net/ssl/SSLSession/TestEnabledProtocols.java, is > failing intermittently because the client side is expecting a SocketException > only if it is wrapped into a SSLException, but it should also expect a > SocketException when there is no exception chaining. This pull request has now been integrated. Changeset: 2047da7d Author:Fernando Guallini Committer: Rajan Halade URL: https://git.openjdk.java.net/jdk17/commit/2047da7dccacb1adb7f811639a58b8fbe1aa3546 Stats: 8 lines in 1 file changed: 1 ins; 0 del; 7 mod 8265297: javax/net/ssl/SSLSession/TestEnabledProtocols.java failed with "RuntimeException: java.net.SocketException: Connection reset" Reviewed-by: xuelei, rhalade - PR: https://git.openjdk.java.net/jdk17/pull/80
Re: RFR: JDK-8268464 : Remove dependancy of TestHttpsServer, HttpTransaction, HttpCallback from open/test/jdk/sun/net/www/protocol/https/ tests [v4]
> …HttpCallback from open/test/jdk/sun/net/www/protocol/https/ tests Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision: Implemented review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/4432/files - new: https://git.openjdk.java.net/jdk/pull/4432/files/db615030..295c2a56 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4432=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4432=02-03 Stats: 5 lines in 1 file changed: 1 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4432.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4432/head:pull/4432 PR: https://git.openjdk.java.net/jdk/pull/4432
Re: RFR: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance [v2]
On Thu, 17 Jun 2021 15:59:45 GMT, Xue-Lei Andrew Fan wrote: >> If we keep property sensitive, we may need to use TreeSet. I have updated >> the PR with TreeSet. Fortunately, the performance hasn't changed much. > > I did not get the point to use TreeSet. Is it sufficient if the > toLowerCase() is not added (and don't compare keywords like "keySize" by > ignoring cases)? > > > - algorithmsInProperty[i] = algorithmsInProperty[i].trim().toLowerCase(...); > + algorithmsInProperty[i] = algorithmsInProperty[i].trim(); Sorry, I missed a "case" in the original comment (corrected). I meant to keep the property case sensitive in the hash set so that the keywords like "keySize" could be used correctly. - PR: https://git.openjdk.java.net/jdk/pull/4424
Re: RFR: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance [v2]
On Thu, 17 Jun 2021 12:05:28 GMT, Dongbo He wrote:
>> src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java
>> line 72:
>>
>>> 70: algorithmsInProperty = property.split(",");
>>> 71: for (int i = 0; i < algorithmsInProperty.length; i++) {
>>> 72: algorithmsInProperty[i] =
>>> algorithmsInProperty[i].trim().toLowerCase(Locale.ENGLISH);
>>
>> Is it possible to keep the current behavior that the property could be
>> sensitive? It may be not desired to allow "keysize" for "keySize" spec in
>> the property.
>
> If we keep property sensitive, we may need to use TreeSet. I have updated the
> PR with TreeSet. Fortunately, the performance hasn't changed much.
I did not get the point to use TreeSet. Is it sufficient if the toLowerCase()
is not added (and don't compare keywords like "keySize" by ignoring cases)?
- algorithmsInProperty[i] = algorithmsInProperty[i].trim().toLowerCase(...);
+ algorithmsInProperty[i] = algorithmsInProperty[i].trim();
-
PR: https://git.openjdk.java.net/jdk/pull/4424
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Thu, 17 Jun 2021 10:21:35 GMT, Alan Bateman wrote:
>> After JDK-8265518(#3615), it's possible to replace all variants of
>> checkIndex by
>> Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in
>> the whole JDK codebase.
>
> src/java.base/share/classes/java/util/Base64.java line 934:
>
>> 932: if (closed)
>> 933: throw new IOException("Stream is closed");
>> 934: Preconditions.checkFromIndexSize(len, off, b.length, (xa,
>> xb) -> new ArrayIndexOutOfBoundsException());
>
> You might want to split this really long line too, to avoid inconsistent line
> length in this source file.
I would suggest factoring out `(xa, xb) -> new
ArrayIndexOutOfBoundsException()` as a reusable component on `Preconditions`,
and maybe even supplying an exception message (since it is rather useful, and
way better than no message).
See the usages of `Preconditions.outOfBoundsExceptionFormatter` (where there
just happens to be many repeated cases of supplying AIOOBE with a message, that
could also be consolidated, separate fix perhaps).
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: blizzard of deprecation warnings related to JEP 411
On 6/17/21 4:56 AM, Alan Bateman wrote:
On 17/06/2021 00:30, Rick Hillegas wrote:
Thanks for that advice, Alan. I have rototilled
@SuppressWarnings("removal") annotations across the Derby codebase
and thrown more memory at javadoc so that it won't crash on JDK 11.
When I run Derby's test suites, I see a blizzard of the following
diagnostics:
WARNING: java.lang.System::setSecurityManager is deprecated and
will be removed in a future release.
Unfortunately, Derby still has more than 100 canon-based tests which
were developed before assertion-based testing became the norm. These
tests are run both with and without a security manager. In the latter
case, they now fail on JDK 17.
Is there a way to disable this diagnostic?
Even better: Could the diagnostic be removed in the next Open JDK
build? It could be re-introduced when Open JDK provides a replacement
for the deprecated functionality. Right now the diagnostic does not
seem to provide any actionable information.
I assume the OOME with javadoc isn't anything to do with this JEP or
the @SupressWarnings annotations, right?
I stopped investigating the problem after I found that I could work
around it by giving javadoc more memory. All I can report is the following:
1) The extra annotations caused the JDK 11 javadoc tool to run out of
memory.
2) The extra annotations did NOT cause the JDK 17 javadoc tool to run
out of memory.
There isn't any way to suppress the warning. This warning is sending a
clear message that that setSecurityManager will be removed in the
future. The "Illegal reflective access" warnings in JDK 9-15 set the
precedent.
For applications that do set a security manager then it is more likely
that they set it once, at startup, rather than setting it hundreds of
times in a running VM. Does Derby call setSecurityManager itself?
The Derby network server installs a security manager if the DBA doesn't.
With some effort, users can override this behavior. This behavior dates
back to 2007 and was introduced by Derby release 10.3.1.4. At that time,
developers from IBM and Sun Microsystems (the major players in the Derby
community) agreed that the client-server configuration should be "secure
by default."
At least in the embedded case then I wouldn't expect it does as it
will be up to the application to do that if it wants. Clearly Derby
has tests to ensure that it can work with a SM so I assume its the
tests that are calling setSecurityManager. I'm not familar with the
term "canon-based tests" but I'm guessing that these are tests that
run with and without SM and are somehow sensitive to the stderr
stream, is that right?
Canon-based testing is an old black-box testing pattern in which you do
the following:
1) Run a test script through a Read-Evaluate-Print-Loop tool.
2) Collect the console output (stdout + stderr).
3) Compare the actual output to a file of expected output (the canon).
There were a small number of these in the JDK test suite too, but not
many.
-Alan
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v4]
On Thu, 17 Jun 2021 14:27:30 GMT, Weijun Wang wrote: >> test/jdk/java/security/ProtectionDomain/RecursionDebug.java line 91: >> >>> 89: } >>> 90: >>> 91: System.setSecurityManager(null); >> >> Why did this line need to be removed? > > This is where the `setSecurityManager` method is called again when a security > manager has already been installed, and the `getProtectionDomain` permission > is needed. While I can add the new permission into the policy file, my > understanding is that this line was just a clean-up and the test was about > implementation of `ProtectionDomain::toString` (esp the `seeAllp` method > called by it) when debug is on. Ok, thanks for the explanation. I think it should be ok to remove this line then. - PR: https://git.openjdk.java.net/jdk17/pull/13
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v4]
On Thu, 17 Jun 2021 14:55:02 GMT, Weijun Wang wrote: >> More loudly and precise warning messages when a security manager is either >> enabled at startup or installed at runtime. >> >> This is new PR for the `openjdk/jdk17` repo copied from >> https://github.com/openjdk/jdk/pull/4400. A new commit is added. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > add bug id into a test Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk17/pull/13
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v4]
> More loudly and precise warning messages when a security manager is either > enabled at startup or installed at runtime. > > This is new PR for the `openjdk/jdk17` repo copied from > https://github.com/openjdk/jdk/pull/4400. A new commit is added. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: add bug id into a test - Changes: - all: https://git.openjdk.java.net/jdk17/pull/13/files - new: https://git.openjdk.java.net/jdk17/pull/13/files/cb732f99..2d5b1ba5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17=13=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=13=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk17/pull/13.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/13/head:pull/13 PR: https://git.openjdk.java.net/jdk17/pull/13
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v3]
On Thu, 17 Jun 2021 14:05:40 GMT, Sean Mullan wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> new warning text again (6/14) > > test/jdk/java/security/ProtectionDomain/RecursionDebug.java line 91: > >> 89: } >> 90: >> 91: System.setSecurityManager(null); > > Why did this line need to be removed? This is where the `setSecurityManager` method is called again when a security manager has already been installed, and the `getProtectionDomain` permission is needed. While I can add the new permission into the policy file, my understanding is that this line was just a clean-up and the test was about implementation of `ProtectionDomain::toString` (esp the `seeAllp` method called by it) when debug is on. - PR: https://git.openjdk.java.net/jdk17/pull/13
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v3]
On Thu, 17 Jun 2021 14:02:35 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> new warning text again (6/14)
>
> test/jdk/java/lang/System/SecurityManagerWarnings.java line 38:
>
>> 36: import java.security.Permission;
>> 37:
>> 38: public class SecurityManagerWarnings {
>
> This should probably have 8268349 in the @bug line.
OK.
-
PR: https://git.openjdk.java.net/jdk17/pull/13
Re: [jdk17] RFR: 8268349: Provide clear run-time warnings about Security Manager deprecation [v3]
On Mon, 14 Jun 2021 22:34:03 GMT, Weijun Wang wrote:
>> More loudly and precise warning messages when a security manager is either
>> enabled at startup or installed at runtime.
>>
>> This is new PR for the `openjdk/jdk17` repo copied from
>> https://github.com/openjdk/jdk/pull/4400. A new commit is added.
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> new warning text again (6/14)
test/jdk/java/lang/System/SecurityManagerWarnings.java line 38:
> 36: import java.security.Permission;
> 37:
> 38: public class SecurityManagerWarnings {
This should probably have 8268349 in the @bug line.
test/jdk/java/security/ProtectionDomain/RecursionDebug.java line 91:
> 89: }
> 90:
> 91: System.setSecurityManager(null);
Why did this line need to be removed?
-
PR: https://git.openjdk.java.net/jdk17/pull/13
RFR: 8268965: TCP Connection Reset when connecting simple socket to SSL server
Please review the fix for JDK-8268965. The new jtreg test is added for the described issue. sun/security/ssl and javax/net/ssl tests are passed - Commit messages: - 8268965: TCP Connection Reset when connecting simple socket to SSL server Changes: https://git.openjdk.java.net/jdk/pull/4520/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4520=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268965 Stats: 114 lines in 2 files changed: 114 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4520.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4520/head:pull/4520 PR: https://git.openjdk.java.net/jdk/pull/4520
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On 17/06/2021 8:50 pm, Alan Bateman wrote: On Thu, 17 Jun 2021 05:16:14 GMT, David Holmes wrote: There are a lot more tests than just tier1. :) I don't expect many, if any, tests to be looking for a specific IOOBE message, and I can't see an easy way to find such tests without running them. If core-libs folk are okay with this update then I guess we can just handle any test failures if they arise. But I'll run this through our tier 1-3 testing. It would be good to run tier 1-3. Off hand I can't think of any tests that are dependent on the exception message, I think I'm more concerned about changing behavior (once you throw a more specific IOOBE is some of the very core classes then it's hard to change it because libraries get dependent on the more specific exception). tiers 1-3 on Linux-x64 passed okay. Any change to the exact type of exception thrown should be affirmed through a CSR request. Cheers, David - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance [v3]
On Thu, 17 Jun 2021 08:16:42 GMT, Dongbo He wrote: >> Now AlgorithmConstraints:checkAlgorithm uses List to check if an algorithm >> has been disabled. It is less efficient when there are more disabled >> elements in the list, we can use Set instead of List to speed up the search. >> >> Patch contains a benchmark that can be run with `make test >> TEST="micro:java.security.AlgorithmConstraintsPermits"`. >> Baseline results before patch: >> >> Benchmark(algorithm) Mode CntScore >> Error Units >> AlgorithmConstraintsPermits.permitsSSLv3 avgt5 21.687 ? >> 1.118 ns/op >> AlgorithmConstraintsPermits.permits DES avgt5 324.216 ? >> 6.233 ns/op >> AlgorithmConstraintsPermits.permits NULL avgt5 709.462 ? >> 51.259 ns/op >> AlgorithmConstraintsPermits.permits TLS1.3 avgt5 687.497 ? >> 170.181 ns/op >> >> Benchmark results after patch: >> >> Benchmark(algorithm) Mode CntScore >> Error Units >> AlgorithmConstraintsPermits.permitsSSLv3 avgt5 46.407 ? >> 1.057 ns/op >> AlgorithmConstraintsPermits.permits DES avgt5 65.722 ? >> 0.578 ns/op >> AlgorithmConstraintsPermits.permits NULL avgt5 43.988 ? >> 1.264 ns/op >> AlgorithmConstraintsPermits.permits TLS1.3 avgt5 399.546 ? >> 11.194 ns/op >> >> SSLv3, DES, NULL are the first, middle and last element in >> `jdk.tls.disabledAlgorithms` from `conf/security/java.security`. >> >> Tomcat(maxKeepAliveRequests=1, which will disable HTTP/1.0 >> keep-alive)+Jmeter: >> Before patch: >> >> summary + 50349 in 00:00:30 = 1678.4/s Avg: 238 Min: 188 Max: 298 >> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 >> summary = 135183 in 00:01:22 = 1654.5/s Avg: 226 Min:16 Max: 1281 >> Err: 0 (0.00%) >> summary + 50240 in 00:00:30 = 1674.1/s Avg: 238 Min: 200 Max: 308 >> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 >> summary = 185423 in 00:01:52 = 1659.7/s Avg: 229 Min:16 Max: 1281 >> Err: 0 (0.00%) >> summary + 50351 in 00:00:30 = 1678.4/s Avg: 238 Min: 191 Max: 306 >> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 >> summary = 235774 in 00:02:22 = 1663.7/s Avg: 231 Min:16 Max: 1281 >> Err: 0 (0.00%) >> summary + 50461 in 00:00:30 = 1681.9/s Avg: 237 Min: 174 Max: 303 >> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 >> >> After patch: >> >> summary + 59003 in 00:00:30 = 1966.6/s Avg: 203 Min: 158 Max: 272 >> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 >> summary = 146675 in 00:01:18 = 1884.6/s Avg: 198 Min:26 Max: 697 >> Err: 0 (0.00%) >> summary + 58965 in 00:00:30 = 1965.9/s Avg: 203 Min: 166 Max: 257 >> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 >> summary = 205640 in 00:01:48 = 1907.2/s Avg: 199 Min:26 Max: 697 >> Err: 0 (0.00%) >> summary + 59104 in 00:00:30 = 1969.1/s Avg: 203 Min: 157 Max: 266 >> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 >> summary = 264744 in 00:02:18 = 1920.7/s Avg: 200 Min:26 Max: 697 >> Err: 0 (0.00%) >> summary + 59323 in 00:00:30 = 1977.6/s Avg: 202 Min: 158 Max: 256 >> Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 >> >> >> Testing: tier1, tier2 > > Dongbo He has updated the pull request incrementally with one additional > commit since the last revision: > > Replace HashSet with TreeSet The following is the Benchmark results for the new commit: JMH: Benchmark(algorithm) Mode CntScoreError Units AlgorithmConstraintsPermits.permitsSSLv3 avgt5 47.353 ? 3.193 ns/op AlgorithmConstraintsPermits.permits DES avgt5 60.307 ? 1.351 ns/op AlgorithmConstraintsPermits.permits NULL avgt5 59.065 ? 0.728 ns/op AlgorithmConstraintsPermits.permits TLS1.3 avgt5 428.311 ? 16.542 ns/op Tomcat+Jmeter: summary + 60455 in 00:00:30 = 2016.4/s Avg: 198 Min: 164 Max: 256 Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 summary = 106982 in 00:00:55 = 1927.6/s Avg: 188 Min:25 Max: 1194 Err: 0 (0.00%) summary + 60430 in 00:00:30 = 2014.6/s Avg: 198 Min: 160 Max: 252 Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 summary = 167412 in 00:01:25 = 1958.1/s Avg: 191 Min:25 Max: 1194 Err: 0 (0.00%) summary + 60480 in 00:00:30 = 2014.6/s Avg: 198 Min: 161 Max: 245 Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 summary = 227892 in 00:01:56 = 1972.8/s Avg: 193 Min:25 Max: 1194 Err: 0 (0.00%) summary + 60506 in 00:00:30 = 2016.6/s Avg: 198 Min: 161 Max: 255 Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 - PR: https://git.openjdk.java.net/jdk/pull/4424
Re: RFR: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance [v2]
On Thu, 17 Jun 2021 04:43:27 GMT, Xue-Lei Andrew Fan wrote:
>> Dongbo He has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Make getAlgorithms() method return a Set
>
> src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java
> line 72:
>
>> 70: algorithmsInProperty = property.split(",");
>> 71: for (int i = 0; i < algorithmsInProperty.length; i++) {
>> 72: algorithmsInProperty[i] =
>> algorithmsInProperty[i].trim().toLowerCase(Locale.ENGLISH);
>
> Is it possible to keep the current behavior that the property could be
> sensitive? It may be not desired to allow "keysize" for "keySize" spec in
> the property.
If we keep property sensitive, we may need to use TreeSet. I have updated the
PR with TreeSet. Fortunately, the performance hasn't changed much.
-
PR: https://git.openjdk.java.net/jdk/pull/4424
Re: blizzard of deprecation warnings related to JEP 411
On 17/06/2021 00:30, Rick Hillegas wrote:
Thanks for that advice, Alan. I have rototilled
@SuppressWarnings("removal") annotations across the Derby codebase and
thrown more memory at javadoc so that it won't crash on JDK 11. When I
run Derby's test suites, I see a blizzard of the following diagnostics:
WARNING: java.lang.System::setSecurityManager is deprecated and will
be removed in a future release.
Unfortunately, Derby still has more than 100 canon-based tests which
were developed before assertion-based testing became the norm. These
tests are run both with and without a security manager. In the latter
case, they now fail on JDK 17.
Is there a way to disable this diagnostic?
Even better: Could the diagnostic be removed in the next Open JDK
build? It could be re-introduced when Open JDK provides a replacement
for the deprecated functionality. Right now the diagnostic does not
seem to provide any actionable information.
I assume the OOME with javadoc isn't anything to do with this JEP or the
@SupressWarnings annotations, right?
There isn't any way to suppress the warning. This warning is sending a
clear message that that setSecurityManager will be removed in the
future. The "Illegal reflective access" warnings in JDK 9-15 set the
precedent.
For applications that do set a security manager then it is more likely
that they set it once, at startup, rather than setting it hundreds of
times in a running VM. Does Derby call setSecurityManager itself? At
least in the embedded case then I wouldn't expect it does as it will be
up to the application to do that if it wants. Clearly Derby has tests to
ensure that it can work with a SM so I assume its the tests that are
calling setSecurityManager. I'm not familar with the term "canon-based
tests" but I'm guessing that these are tests that run with and without
SM and are somehow sensitive to the stderr stream, is that right? There
were a small number of these in the JDK test suite too, but not many.
-Alan
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Thu, 17 Jun 2021 05:16:14 GMT, David Holmes wrote: > There are a lot more tests than just tier1. :) I don't expect many, if any, > tests to be looking for a specific IOOBE message, and I can't see an easy way > to find such tests without running them. If core-libs folk are okay with this > update then I guess we can just handle any test failures if they arise. But > I'll run this through our tier 1-3 testing. It would be good to run tier 1-3. Off hand I can't think of any tests that are dependent on the exception message, I think I'm more concerned about changing behavior (once you throw a more specific IOOBE is some of the very core classes then it's hard to change it because libraries get dependent on the more specific exception). - PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268698: Use Objects.check{Index, FromToIndex, FromIndexSize} where possible
On Wed, 16 Jun 2021 08:08:47 GMT, Yi Yang wrote:
> After JDK-8265518(#3615), it's possible to replace all variants of checkIndex
> by Objects.checkIndex/Objects.checkFromToIndex/Objects.checkFromIndexSize in
> the whole JDK codebase.
I looked through the changes in java.base and only spotted one case where a
different (and more specific) exception is thrown.
The changes to to files in java.util.zip lead to annoying long lines so would
be good to fix all those.
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 471:
> 469: */
> 470: public int offsetByCodePoints(int index, int codePointOffset) {
> 471: checkOffset(index, count);
String.offsetByCodePoints is specified to throw IOOBE. checkOffset may throw
the more specific StringIndexOutOfBoundsException. That's a compatible change
but I worry that we might want to throw the less specific exception in the
further because code. I only mention is because there have been cases in
java.lang where IOOBE was specified and AIOOBE by the implementation and it has
been problematic to touch the implementation as a result.
src/java.base/share/classes/java/util/Base64.java line 934:
> 932: if (closed)
> 933: throw new IOException("Stream is closed");
> 934: Preconditions.checkFromIndexSize(len, off, b.length, (xa,
> xb) -> new ArrayIndexOutOfBoundsException());
You might to split this really long line to avoid inconsistent line length in
this source file.
-
PR: https://git.openjdk.java.net/jdk/pull/4507
Re: RFR: 8268873: Unnecessary Vector usage in java.base
On Wed, 16 Jun 2021 09:49:17 GMT, Andrey Turbanov
wrote:
>> src/java.base/share/classes/sun/net/www/protocol/https/HttpsClient.java line
>> 154:
>>
>>> 152: while (tokenizer.hasMoreTokens())
>>> 153: v.add(tokenizer.nextToken());
>>> 154: ciphers = new String [v.size()];
>>
>> Looks like this whole `else` block can be simplified to `ciphers =
>> cipherString.split(",");`
>
> It's not a drop-in replacement. Result is different for some Strings. For
> example for `, A`
> I would prefer to preserve existing behavior under this cleanup.
Then let's keep it as is
-
PR: https://git.openjdk.java.net/jdk/pull/4482
Re: RFR: 8268427: Improve AlgorithmConstraints:checkAlgorithm performance [v3]
> Now AlgorithmConstraints:checkAlgorithm uses List to check if an algorithm > has been disabled. It is less efficient when there are more disabled elements > in the list, we can use Set instead of List to speed up the search. > > Patch contains a benchmark that can be run with `make test > TEST="micro:java.security.AlgorithmConstraintsPermits"`. > Baseline results before patch: > > Benchmark(algorithm) Mode CntScore > Error Units > AlgorithmConstraintsPermits.permitsSSLv3 avgt5 21.687 ? > 1.118 ns/op > AlgorithmConstraintsPermits.permits DES avgt5 324.216 ? > 6.233 ns/op > AlgorithmConstraintsPermits.permits NULL avgt5 709.462 ? > 51.259 ns/op > AlgorithmConstraintsPermits.permits TLS1.3 avgt5 687.497 ? > 170.181 ns/op > > Benchmark results after patch: > > Benchmark(algorithm) Mode CntScoreError > Units > AlgorithmConstraintsPermits.permitsSSLv3 avgt5 46.407 ? 1.057 > ns/op > AlgorithmConstraintsPermits.permits DES avgt5 65.722 ? 0.578 > ns/op > AlgorithmConstraintsPermits.permits NULL avgt5 43.988 ? 1.264 > ns/op > AlgorithmConstraintsPermits.permits TLS1.3 avgt5 399.546 ? 11.194 > ns/op > > SSLv3, DES, NULL are the first, middle and last element in > `jdk.tls.disabledAlgorithms` from `conf/security/java.security`. > > Tomcat(maxKeepAliveRequests=1, which will disable HTTP/1.0 keep-alive)+Jmeter: > Before patch: > > summary + 50349 in 00:00:30 = 1678.4/s Avg: 238 Min: 188 Max: 298 Err: > 0 (0.00%) Active: 400 Started: 400 Finished: 0 > summary = 135183 in 00:01:22 = 1654.5/s Avg: 226 Min:16 Max: 1281 Err: > 0 (0.00%) > summary + 50240 in 00:00:30 = 1674.1/s Avg: 238 Min: 200 Max: 308 Err: > 0 (0.00%) Active: 400 Started: 400 Finished: 0 > summary = 185423 in 00:01:52 = 1659.7/s Avg: 229 Min:16 Max: 1281 Err: > 0 (0.00%) > summary + 50351 in 00:00:30 = 1678.4/s Avg: 238 Min: 191 Max: 306 Err: > 0 (0.00%) Active: 400 Started: 400 Finished: 0 > summary = 235774 in 00:02:22 = 1663.7/s Avg: 231 Min:16 Max: 1281 Err: > 0 (0.00%) > summary + 50461 in 00:00:30 = 1681.9/s Avg: 237 Min: 174 Max: 303 Err: > 0 (0.00%) Active: 400 Started: 400 Finished: 0 > > After patch: > > summary + 59003 in 00:00:30 = 1966.6/s Avg: 203 Min: 158 Max: 272 Err: > 0 (0.00%) Active: 400 Started: 400 Finished: 0 > summary = 146675 in 00:01:18 = 1884.6/s Avg: 198 Min:26 Max: 697 Err: > 0 (0.00%) > summary + 58965 in 00:00:30 = 1965.9/s Avg: 203 Min: 166 Max: 257 Err: > 0 (0.00%) Active: 400 Started: 400 Finished: 0 > summary = 205640 in 00:01:48 = 1907.2/s Avg: 199 Min:26 Max: 697 Err: > 0 (0.00%) > summary + 59104 in 00:00:30 = 1969.1/s Avg: 203 Min: 157 Max: 266 Err: > 0 (0.00%) Active: 400 Started: 400 Finished: 0 > summary = 264744 in 00:02:18 = 1920.7/s Avg: 200 Min:26 Max: 697 Err: > 0 (0.00%) > summary + 59323 in 00:00:30 = 1977.6/s Avg: 202 Min: 158 Max: 256 Err: > 0 (0.00%) Active: 400 Started: 400 Finished: 0 > > > Testing: tier1, tier2 Dongbo He has updated the pull request incrementally with one additional commit since the last revision: Replace HashSet with TreeSet - Changes: - all: https://git.openjdk.java.net/jdk/pull/4424/files - new: https://git.openjdk.java.net/jdk/pull/4424/files/dde672f6..0253fdb2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4424=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4424=01-02 Stats: 17 lines in 2 files changed: 2 ins; 1 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/4424.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4424/head:pull/4424 PR: https://git.openjdk.java.net/jdk/pull/4424
