RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]
Mark Powers
duke at openjdk.java.net
Fri Jun 10 20:56:10 UTC 2022
On Wed, 8 Jun 2022 15:53:06 GMT, Weijun Wang <weijun at openjdk.org> 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.org/jdk/compare/4d6fb515...44140a01
>
> 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.
The `ServiceKey` class has `equals` and `matches` methods. The `matches` method returns true if two objects contain identical String pointers. The `equals` method returns true if String pointers are different but have the same value. This is by design, so it would be a mistake to take IntelliJ's suggestion here.
Keeping the `==` and adding a comment.
Good catch.
> 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`?
Renamed `key1` to `unused`.
> src/java.base/share/classes/java/security/SecureClassLoader.java line 235:
>
>> 233: }
>> 234:
>> 235: private record CodeSourceKey(CodeSource cs) {
>
> Really necessary?
No. It was just an IntelliJ suggestion. I have no preference.
> 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?
The author must have thought `:` and `,` were metacharacters that needed to be escaped to turn them into ordinary characters. I verified with a small java program that` \:` and` \,` are equivalent to `;` and `,`.
The IntelliJ complaint is "Redundant character escape `'\:'`".
> src/java.base/share/classes/java/security/UnresolvedPermission.java line 369:
>
>> 367: this.certs != null && that.certs == null ||
>> 368: this.certs != null &&
>> 369: this.certs.length != that.certs.length) {
>
> I see you keep a lot of parentheses in other places. For example, `(!r)`.
This is an IntelliJ suggestion. I don't know why it complains about some cases and ignores others. Perhaps it has something to do with readability.
> src/java.base/share/classes/java/security/cert/CertPathValidator.java line 335:
>
>> 333: String cpvtype =
>> 334: AccessController.doPrivileged((PrivilegedAction<String>) () ->
>> 335: Security.getProperty(CPV_TYPE));
>
> See too many, maybe we need a dedicated method in `sun.security.util.SecurityProperties`?
A privilegedGetProperty method? That would be the subject of a new bug I think.
-------------
PR: https://git.openjdk.org/jdk/pull/8319
More information about the security-dev
mailing list