> 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 != nu...
Pavel Rappo has updated the pull request incrementally with one additional commit since the last revision: Feedback (part 3) ------------- Changes: - all: https://git.openjdk.org/jdk/pull/14738/files - new: https://git.openjdk.org/jdk/pull/14738/files/d9316270..608258ea Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=14738&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=14738&range=12-13 Stats: 3 lines in 2 files changed: 0 ins; 2 del; 1 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
