RFR: JDK-8285263 Minor cleanup could be done in java.security [v5]

[Weijun Wang]

On Tue, 7 Jun 2022 15:37:02 GMT, Mark Powers <duke at openjdk.java.net> 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:                 "\\s*([\\S&&[^:,]]*)(:([\\S&&[^,]]*))?\\s*(,(.*))?");

Don't quite understand this change. Can you explain?

src/java.base/share/classes/java/security/Security.java line 260:

> 258:         }
> 259: 
> 260:         return null;

If `entry` is always null, then we don't need to declare it at all.

src/java.base/share/classes/java/security/Security.java line 260:

> 258:         }
> 259: 
> 260:         return null;

If `entry` is always null, then we don't need to declare it at all.

src/java.base/share/classes/java/security/Signature.java line 271:

> 269:                 (algorithm + " Signature not available");
> 270:         }
> 271:         // try services until we find a Spi or a working Signature subclass

Is `Spi` pronounced `spy` or "S-P-I"?

src/java.base/share/classes/java/security/Signature.java line 271:

> 269:                 (algorithm + " Signature not available");
> 270:         }
> 271:         // try services until we find a Spi or a working Signature subclass

Is `Spi` pronounced `spy` or "S-P-I"?

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)`.

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)`.

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`?

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`?

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`?

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`?

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?

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?

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`.

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`.

-------------

PR: https://git.openjdk.java.net/jdk/pull/8319