On Fri, 14 Apr 2023 14:33:56 GMT, Weijun Wang <[email protected]> wrote:
>> src/java.base/share/classes/javax/crypto/KEM.java line 94:
>>
>>> 92: * @see KEM#newEncapsulator(PublicKey, AlgorithmParameterSpec,
>>> SecureRandom)
>>> 93: */
>>> 94: public record Encapsulated(SecretKey key, byte[] encapsulation,
>>> byte[] params) {
>>
>> We need to decide if the encapsulation and params arrays should be
>> defensively cloned. I would lean towards cloning it since immutability is a
>> feature of this API, and I think it would be surprising if this type was
>> not.
>>
>> We can potentially switch to frozen arrays later.
>
> If we need to clone defensively I'll switch back to normal class, and then we
> can clone in both the constructor and the getters. IMO records are meant to
> be deadly simple.
Rewrite in plain normal class. Clone on both sides.
>> src/java.base/share/classes/javax/crypto/KEMSpi.java line 211:
>>
>>> 209: * The caller of this method has already validated the parameters
>>> to
>>> 210: * ensure that {@code pk} is not {@code null}. Therefore an
>>> implementation
>>> 211: * of this method does not to validate it.
>>
>> s/not to/not need to/
>>
>> Also, suggest saying who the caller is, "The caller (KEM.newEncapsulator) of
>> this method ..."
>
> Fixed.
>
> Not sure if the caller needs to be written out. `KEM.newEncapsulator` is the
> ultimate caller, but for delayed provider selection, the direct caller is in
> `Delayed`. Also, this can be considered as implementation detail. If I write
> out the name, then we cannot change.
>
> `engineEncapsulate` and `engineDecapsulate` also use "the caller of this
> method".
Removed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1170288830
PR Review Comment: https://git.openjdk.org/jdk/pull/13256#discussion_r1170290779
