Integrated: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet
On Tue, 19 Apr 2022 18:00:19 GMT, XenoAmess wrote: > as title. This pull request has now been integrated. Changeset: e01cd7c3 Author:XenoAmess Committer: Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/e01cd7c3ed923cd19509fc972ba6e4aa2991289f Stats: 154 lines in 29 files changed: 107 ins; 7 del; 40 mod 8284780: Need methods to create pre-sized HashSet and LinkedHashSet Reviewed-by: naoto, bpb, dfuchs, ascarpino - PR: https://git.openjdk.java.net/jdk/pull/8302
RFR: 8224768: Test ActalisCA.java fails
Updated with new test artifacts from CA. - Commit messages: - 8224768: Test ActalisCA.java fails Changes: https://git.openjdk.java.net/jdk/pull/9097/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9097=00 Issue: https://bugs.openjdk.org/browse/JDK-8224768 Stats: 199 lines in 2 files changed: 9 ins; 36 del; 154 mod Patch: https://git.openjdk.java.net/jdk/pull/9097.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9097/head:pull/9097 PR: https://git.openjdk.java.net/jdk/pull/9097
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
On Wed, 8 Jun 2022 16:12:36 GMT, Weijun Wang wrote:
>> Mark Powers has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 11 commits:
>>
>> - Merge
>> - fourth iteration
>> - Merge
>> - Merge
>> - third iteration
>> - Merge
>> - Merge
>> - second iteration
>> - Merge
>> - Merge
>> - ... and 1 more:
>> https://git.openjdk.java.net/jdk/compare/4d6fb515...44140a01
>
> src/java.base/share/classes/java/security/cert/PKIXBuilderParameters.java
> line 193:
>
>> 191: public String toString() {
>> 192: return "[\n" +
>> 193: super.toString() +
>
> Is `toString` necessary?
No it's not necessary. I had some discussion with Brad and others about boxing
vs unboxing. The same byte code is generated either way, so to box or unbox is
a matter of personal preference.
> src/java.base/share/classes/java/security/spec/ECPoint.java line 103:
>
>> 101: return obj instanceof ECPoint other
>> 102: && ((Objects.equals(x, other.x))
>> 103: && (Objects.equals(y, other.y)));
>
> `x` and `y` will not be null unless this is `POINT_INFINITY`, but this is
> already checked in both `equals` and `hashCode`.
I'm reversing IntelliJ's suggested fix since it really doesn't fix anything.
-
PR: https://git.openjdk.java.net/jdk/pull/8319
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
On Wed, 8 Jun 2022 16:12:05 GMT, Weijun Wang wrote:
>> Mark Powers has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 11 commits:
>>
>> - Merge
>> - fourth iteration
>> - Merge
>> - Merge
>> - third iteration
>> - Merge
>> - Merge
>> - second iteration
>> - Merge
>> - Merge
>> - ... and 1 more:
>> https://git.openjdk.java.net/jdk/compare/4d6fb515...44140a01
>
> src/java.base/share/classes/java/security/cert/LDAPCertStoreParameters.java
> line 34:
>
>> 32: * name and port number) to implementations of the LDAP {@code CertStore}
>> 33: * algorithm. However, if you are retrieving certificates or CRLs from
>> 34: * a ldap URI as specified by RFC 5280, use the
>
> Isn't `ldap` pronounced `L-Dap`?
You are right. IntelliJ was wrong and I didn't catch it.
-
PR: https://git.openjdk.java.net/jdk/pull/8319
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
On Wed, 8 Jun 2022 15:50:38 GMT, Weijun Wang wrote: >> Mark Powers has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 11 commits: >> >> - Merge >> - fourth iteration >> - Merge >> - Merge >> - third iteration >> - Merge >> - Merge >> - second iteration >> - Merge >> - Merge >> - ... and 1 more: >> https://git.openjdk.java.net/jdk/compare/4d6fb515...44140a01 > > src/java.base/share/classes/java/security/ProtectionDomain.java line 492: > >> 490: Policy p = Policy.getPolicyNoCheck(); >> 491: return p.getPermissions(ProtectionDomain.this); >> 492: }); > > First, indent these lines. (or maybe my GitHub view has not reveals the > indentation?). > > Second, inline `p` then there is no need to `return`. Simply `() -> P.g.g(P)`. I made your suggested change. My viewer shows the indented lines. Go figure. - PR: https://git.openjdk.java.net/jdk/pull/8319
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
On Wed, 8 Jun 2022 15:46:28 GMT, Weijun Wang wrote:
>> Mark Powers has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 11 commits:
>>
>> - Merge
>> - fourth iteration
>> - Merge
>> - Merge
>> - third iteration
>> - Merge
>> - Merge
>> - second iteration
>> - Merge
>> - Merge
>> - ... and 1 more:
>> https://git.openjdk.java.net/jdk/compare/4d6fb515...44140a01
>
> src/java.base/share/classes/java/security/MessageDigest.java line 306:
>
>> 304: } else {
>> 305: return Delegate.of((MessageDigestSpi)objs[0], algorithm,
>> 306: (Provider)objs[1]);
>
> Indent this line.
done
> src/java.base/share/classes/java/security/ProtectionDomain.java line 474:
>
>> 472: return true;
>> 473: } catch (SecurityException se) {
>> 474: // fall through and return false
>
> How about we just `return false` here and then there is no need for the
> `return false` at the end?
done
-
PR: https://git.openjdk.java.net/jdk/pull/8319
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v19]
On Wed, 8 Jun 2022 17:49:38 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > clean up Calendar I gave a quick look at the security files touched and seems straightforward. I didn't see any problems - Marked as reviewed by ascarpino (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v19]
On Wed, 8 Jun 2022 17:49:38 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > clean up Calendar Running tests and awaiting review from security team. Our internal test system is backlogged and tests might not complete in time to get into JDK 19. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Wed, 1 Jun 2022 18:26:17 GMT, Naoto Sato wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> do it as naotoj said
>
> src/java.base/share/classes/java/util/Calendar.java line 2648:
>
>> 2646: set.add("gregory");
>> 2647: set.add("buddhist");
>> 2648: set.add("japanese");
>
> This can be replaced with `SET = Set.of("gregory", "buddhist", "japanese");`.
@naotoj Yes it can. I did a further clean up to it, please have a look.
-
PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v19]
> as title. XenoAmess has updated the pull request incrementally with one additional commit since the last revision: clean up Calendar - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/bacc9ca8..95d59b97 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8302=18 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8302=17-18 Stats: 8 lines in 1 file changed: 0 ins; 7 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v17]
On Wed, 1 Jun 2022 17:34:04 GMT, Stuart Marks wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> do it as naotoj said > > test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 441: > >> 439: } >> 440: } >> 441: > > This unifies the test cases between the Set and Map factories, which > accomplishes the primary goal. Good. > > The unification is achieved through classic object-oriented polymorphism, > which works fine, though which is rather verbose. This could probably be > reduced with some tinkering on the model, but it's probably reaching the > point where additional tinkering on the model isn't worth it. I'm ok with > sticking with this approach for now. Maybe we can clean it up later, or maybe > not -- it's at least fairly straightforward. > > One issue that contributes to the verbosity is the repeated null checking. > The null checking enables the test code to proceed and end up with -1 as the > capacity if there's a null in there somewhere. This will cause the assertion > to fail. This is good in that it will call attention to itself (as opposed to > silently passing or something). However, if the test cases are set up > properly, they should never run into null. If the null checking weren't done, > an unexpected null will throw NPE, which will be caught be the framework and > reported as an error. > > That seems perfectly fine to me, so I'd suggest simply removing the null > checking. That would also reduce the bulkiness of infrastructure. @stuart-marks done. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v18]
> as title. XenoAmess has updated the pull request incrementally with two additional commits since the last revision: - remove null check for Capacitiable in WhiteBoxResizeTest - Rename type variable per CSR request; minor spec wording change. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/98bfb0e1..bacc9ca8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8302=17 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8302=16-17 Stats: 19 lines in 3 files changed: 0 ins; 13 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
On Tue, 7 Jun 2022 15:37:02 GMT, Mark Powers wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8285263 Minor cleanup could be done
>> in java.security
>>
>> JDK-8273046 is the umbrella bug for this bug. The changes were too large for
>> a single code review, so it was decided to split into smaller chunks. This
>> is one such chunk:
>>
>> open/src/java.base/share/classes/java/security
>
> Mark Powers has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 11 commits:
>
> - Merge
> - fourth iteration
> - Merge
> - Merge
> - third iteration
> - Merge
> - Merge
> - second iteration
> - Merge
> - Merge
> - ... and 1 more:
> https://git.openjdk.java.net/jdk/compare/4d6fb515...44140a01
Some comments. Thanks for the hard work.
src/java.base/share/classes/java/security/MessageDigest.java line 306:
> 304: } else {
> 305: return Delegate.of((MessageDigestSpi)objs[0], algorithm,
> 306: (Provider)objs[1]);
Indent this line.
src/java.base/share/classes/java/security/ProtectionDomain.java line 474:
> 472: return true;
> 473: } catch (SecurityException se) {
> 474: // fall through and return false
How about we just `return false` here and then there is no need for the `return
false` at the end?
src/java.base/share/classes/java/security/ProtectionDomain.java line 474:
> 472: return true;
> 473: } catch (SecurityException se) {
> 474: // fall through and return false
How about we just `return false` here and then there is no need for the `return
false` at the end?
src/java.base/share/classes/java/security/ProtectionDomain.java line 492:
> 490: Policy p = Policy.getPolicyNoCheck();
> 491: return p.getPermissions(ProtectionDomain.this);
> 492: });
First, indent these lines. (or maybe my GitHub view has not reveals the
indentation?).
Second, inline `p` then there is no need to `return`. Simply `() -> P.g.g(P)`.
src/java.base/share/classes/java/security/ProtectionDomain.java line 492:
> 490: Policy p = Policy.getPolicyNoCheck();
> 491: return p.getPermissions(ProtectionDomain.this);
> 492: });
First, indent these lines. (or maybe my GitHub view has not reveals the
indentation?).
Second, inline `p` then there is no need to `return`. Simply `() -> P.g.g(P)`.
src/java.base/share/classes/java/security/Provider.java line 1098:
> 1096: boolean matches(String type, String algorithm) {
> 1097: return (Objects.equals(this.type, type)) &&
> 1098: (Objects.equals(this.originalAlgorithm, algorithm));
In fact, I'm not sure why the original code is brave enough to use `==` instead
of `equals`. If you do believe it's a real bug (and can write a regression
test), let's fix it in a real bug fix. Otherwise, keep using `==` and add a
comment saying it's correct.
src/java.base/share/classes/java/security/Provider.java line 1098:
> 1096: boolean matches(String type, String algorithm) {
> 1097: return (Objects.equals(this.type, type)) &&
> 1098: (Objects.equals(this.originalAlgorithm, algorithm));
In fact, I'm not sure why the original code is brave enough to use `==` instead
of `equals`. If you do believe it's a real bug (and can write a regression
test), let's fix it in a real bug fix. Otherwise, keep using `==` and add a
comment saying it's correct.
src/java.base/share/classes/java/security/SecureClassLoader.java line 221:
> 219: // only), and the fragment is not considered.
> 220: CodeSourceKey key = new CodeSourceKey(cs);
> 221: /* not used */
What is not used? `key1`? How about just rename it to `unused`?
src/java.base/share/classes/java/security/SecureClassLoader.java line 221:
> 219: // only), and the fragment is not considered.
> 220: CodeSourceKey key = new CodeSourceKey(cs);
> 221: /* not used */
What is not used? `key1`? How about just rename it to `unused`?
src/java.base/share/classes/java/security/SecureClassLoader.java line 235:
> 233: }
> 234:
> 235: private record CodeSourceKey(CodeSource cs) {
Really necessary?
src/java.base/share/classes/java/security/SecureClassLoader.java line 235:
> 233: }
> 234:
> 235: private record CodeSourceKey(CodeSource cs) {
Really necessary?
src/java.base/share/classes/java/security/SecureRandom.java line 905:
> 903: private static final Pattern pattern =
> 904: Pattern.compile(
> 905: "\\s*([\\S&&[^:,]]*)(:([\\S&&[^,]]*))?\\s*(,(.*))?");
Don't quite understand this change. Can you explain?
src/java.base/share/classes/java/security/SecureRandom.java line 905:
> 903: private static final Pattern pattern =
> 904: Pattern.compile(
> 905:
RFR: 6522064: Aliases from Microsoft CryptoAPI has bad character encoding
Switch to wide char version of `CertGetNameString` to get the non-ASCII name. - Commit messages: - the fix Changes: https://git.openjdk.java.net/jdk/pull/9085/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9085=00 Issue: https://bugs.openjdk.org/browse/JDK-6522064 Stats: 89 lines in 2 files changed: 72 ins; 0 del; 17 mod Patch: https://git.openjdk.java.net/jdk/pull/9085.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/9085/head:pull/9085 PR: https://git.openjdk.java.net/jdk/pull/9085
Re: RFR: 8209935: Test to cover CodeSource.getCodeSigners() [v4]
> A Test updated to cover the getCodeSigners. Sibabrata Sahoo has updated the pull request incrementally with one additional commit since the last revision: 8209935: Test to cover CodeSource.getCodeSigners() - Changes: - all: https://git.openjdk.java.net/jdk/pull/8286/files - new: https://git.openjdk.java.net/jdk/pull/8286/files/c6318862..512cb615 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8286=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8286=02-03 Stats: 144 lines in 2 files changed: 144 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8286.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8286/head:pull/8286 PR: https://git.openjdk.java.net/jdk/pull/8286
Re: RFR: 8209935: Test to cover CodeSource.getCodeSigners() [v3]
> A Test updated to cover the getCodeSigners. Sibabrata Sahoo has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into JDK-8209935 - Update Implies.java - Update Implies.java - 8209935: Test to cover CodeSource.getCodeSigners() - Changes: - all: https://git.openjdk.java.net/jdk/pull/8286/files - new: https://git.openjdk.java.net/jdk/pull/8286/files/70618a17..c6318862 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8286=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8286=01-02 Stats: 359585 lines in 5089 files changed: 236066 ins; 93423 del; 30096 mod Patch: https://git.openjdk.java.net/jdk/pull/8286.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8286/head:pull/8286 PR: https://git.openjdk.java.net/jdk/pull/8286
Re: RFR: 8277307: Pre shared key sent under both session_ticket and pre_shared_key extensions [v2]
On Wed, 8 Jun 2022 05:05:13 GMT, Anthony Scarpino wrote: > The bug and the PR could have used a lot more description that the issue here > is that 1.2 and 1.3 are enabled at the same time. As far as I can tell, 1.2 and 1.3 are both enabled by default. > One could ask the reverse, if the resumption is from 1.2 should we be sending > a 1.3 pre_shared_key extension.. But that can be for another bug I suppose. We are not sending `pre_shared_key` when resuming TLS 1.2 > please make sure all jdk_security tests and tier1 tests pass before > integrating done. Thanks for reviewing! - PR: https://git.openjdk.java.net/jdk/pull/8922
Integrated: 8277307: Pre shared key sent under both session_ticket and pre_shared_key extensions
On Fri, 27 May 2022 13:20:24 GMT, Daniel Jeliński wrote: > Session ticket extension should only contain pre-TLS1.3 stateless session > tickets; it should not be used for sending TLS1.3 pre-shared keys. This pull request has now been integrated. Changeset: 4662e06b Author:Daniel Jeliński URL: https://git.openjdk.java.net/jdk/commit/4662e06bff2cef7425c194a9cdd7a6fe7469179e Stats: 19 lines in 2 files changed: 12 ins; 0 del; 7 mod 8277307: Pre shared key sent under both session_ticket and pre_shared_key extensions Reviewed-by: coffeys, ascarpino - PR: https://git.openjdk.java.net/jdk/pull/8922
