On Fri, 25 Apr 2025 12:41:34 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Anthony Scarpino has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - javadoc updates
>> - code review comments
>
> src/java.base/share/classes/sun/security/ec/ECKeyFactory.java line 225:
>
>> 223: throws GeneralSecurityException {
>> 224: if (keySpec instanceof X509EncodedKeySpec) {
>> 225: return new
>> ECPublicKeyImpl(((X509EncodedKeySpec)keySpec).getEncoded());
>
> We can also use `instanceof` pattern matching here. Same on line 246.
If left it because there were other `instanceof` if-else matching that I didn't
want two different styles in the file. I dunno I'll think about it.
> src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 157:
>
>> 155: }
>> 156:
>> 157: public byte[] getArrayS() {
>
> Why remove `getArrayS0`? Not worth saving those bytes?
I just couldn't figure out why it needed to be S0 and not just S. There is no
S1 and it uses the existing arrayS. Is there some spec that calls it S0?
> src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 191:
>
>> 189: DerValue value = data.getDerValue();
>> 190: if (value.isContextSpecific((byte) 0)) {
>> 191: attributes = value.getDataBytes(); // Save DER
>> sequence
>
> At [0], it's `ECDomainParameters`. I don't think it's the same as
> `attributes` inside `OneAsymmetricKey`.
The variable name should be different, but I'm not too concerned about since
we've never supported them anyway. It's probably something that should be
added, but probably in the future.
> src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 195:
>
>> 193: return;
>> 194: }
>> 195: value = data.getDerValue();
>
> The original code does not care about whether [0] comes before [1] or if
> there are duplicates. You modified code ensures only [1] can be after [0] but
> still allows duplicates (For example, [0], [1], [1]). The `while` on line 188
> can be changed to `if` and we ensure `data.available() == 0` at the end.
Sure
> src/java.base/share/classes/sun/security/ec/ECPrivateKeyImpl.java line 201:
>
>> 199: BitArray bitArray =
>> bits.data.getUnalignedBitString();
>> 200: pubKeyEncoded = new X509Key(algid,
>> 201: bitArray).getEncoded();
>
> We could have 2 copies of the public key, one here and one inside
> `OneAsymmetricKey`. Do you want to compare them?
Have we seen such a DER? I'm sure is possible, but that sounds invalid to have
two binary formats in one bytestream. If we are seriously concerned about that
we should thrown an error rather than try to allow it.
> src/java.base/share/classes/sun/security/util/DerValue.java line 195:
>
>> 193:
>> 194: /**
>> 195: * Returns true if the CONTEXT SPECIFIC bit is set in the type tag.
>
> `iff` was used to mean `if and only if`. It's more precise than a simple `if`.
Ok
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2068976363
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2068971300
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2069653731
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2069653877
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2069393674
PR Review Comment: https://git.openjdk.org/jdk/pull/17543#discussion_r2069678187
