Please review this PR to use modern APIs and language features to simplify 
`equals` and `hashCode` in security area.

I understand that security area is sensitive and a non-expert, such as myself, 
should tread carefully; so below are my notes to assist the review.

* Unlike `hashCode`, non-secure `equals` implementations are typically 
short-circuit. But because of "timing attacks", we seem to have specialized 
implementations, such as `java.security.MessageDigest.isEqual(byte[], byte[])` 
and a more general `sun.security.util.ByteArrays.isEqual(byte[], int, int, 
byte[], int, int)`. So while reviewing this PR, take an opportunity to audit 
the affected `equals` implementations: perhaps some of them need to become 
secure, not modern. I have no domain knowledge to tell those cases apart, I 
only note that those cases exist.

* This PR sacrifices compatibility for pragmatism: it changes some `hashCode` 
implementations to produce different values than before to allow more 
utilization of methods from `Objects` and `Arrays`. To my mind, those changes 
are **benign**. If you disagree, I'd be happy to discuss that and/or retract 
the concerning part of the change.

* BitArray could be a topic of its own, but I'll do my best to be concise.

    * Truth to be told, BitArray's `equals` and `hashCode` are not used 
anywhere in source, and `equals` is only used in one test. For that reason, I 
refrained from reimplementing internals of `BitArray` using more general 
`java.util.BitSet`: too much effort and risk for almost nothing.
    * Speaking of `BitSet`-powered `BitArray`. Such an implementation is not 
for the faint of heart: there's too much impedance mismatch between data 
structures that those classes use to store bits. That said, for the sake of 
testing that it is possible and that I understand the `BitArray` correctly, I 
actually implemented it using `BitSet`. While that implementation is **NOT** 
part of this PR, you can have a look at it 
[here](https://cr.openjdk.org/~prappo/8311170/BitArray.java).

* One suggestion to consider is to change this somewhat arcane piece in 
java.security.UnresolvedPermission.equals:

          // check certs
          if (this.certs == null && that.certs != null ||
              this.certs != null && that.certs == null ||
              this.certs != null &&
                 this.certs.length != that.certs.length) {
              return false;
          }
  
          int i,j;
          boolean match;
  
          for (i = 0; this.certs != null && i < this.certs.length; i++) {
              match = false;
              for (j = 0; j < that.certs.length; j++) {
                  if (this.certs[i].equals(that.certs[j])) {
                      match = true;
                      break;
                  }
              }
              if (!match) return false;
          }
  
          for (i = 0; that.certs != null && i < that.certs.length; i++) {
              match = false;
              for (j = 0; j < this.certs.length; j++) {
                  if (that.certs[i].equals(this.certs[j])) {
                      match = true;
                      break;
                  }
              }
              if (!match) return false;
          }
          return true;

  to this:

          // check certs
          if (this.certs == null && that.certs != null ||
              this.certs != null && that.certs == null)
              return false;
          
          if (this.certs == null) {
              assert that.certs == null;
              return false;
          }
          
          return Set.of(this.certs).equals(Set.of(that.certs));

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

Commit messages:
 - Initial commit

Changes: https://git.openjdk.org/jdk/pull/14738/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14738&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8311170
  Stats: 127 lines in 6 files changed: 16 ins; 73 del; 38 mod
  Patch: https://git.openjdk.org/jdk/pull/14738.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/14738/head:pull/14738

PR: https://git.openjdk.org/jdk/pull/14738

Reply via email to