On Fri, 6 Mar 2026 14:18:20 GMT, Sean Mullan <[email protected]> wrote:
>> I also recommend updating the `@throws InvalidKeyException` to state that it >> is also thrown if the offset is negative. For example: >> >> "if offset is negative, or the given key material is <code>null</code>, or >> starting at <code>offset</code> inclusive, is shorter than 8 bytes." >> >> Arguably, throwing `IllegalArgumentException` is more appropriate, but I >> think that would have a higher compatibility risk since it would be a new >> exception thrown (even though DES/DESede really shouldn't be used much >> anymore). > > Actually, on second thought, these methods already throw > `ArrayIndexOutOfBoundsException` when the offset is negative, right? > > Therefore I think it is better to throw that instead of `InvalidKeyException` > and add that to the javadoc. This would also align it with `SecretKeySpec` > which throws AIOBE for negative offsets. This is the existing behavior of > this class so the compatibility risk should be low. > @seanjmullan thank you for your review. I've updated the PR to switch to > `AIOBE` instead of `IKE` for both DES and 3DES in all relevant methods. In > order to make the interfaces consistent I did have to explicitly check for > null `key` arguments given the ordering with implicit throws. As a result, I > needed to update the null `key` checks in DES and 3DES, along with the unit > test for null `key` checks (where I test for both ciphers now). AFAIK, there isn't any ordering implied by the order of the specified exceptions - if the parameters violate more than one condition, the implementation is compliant as long as one of the exceptions is thrown. But it is better to specifically check for null rather than letting an NPE be thrown when dereferencing, so I am ok with this. > Couple of followup questions based on these changes: i) Is a CSR no longer > required given that this is what is thrown implicitly given negative `offset` > parameters? ii) Should I change the bug ID synopsis to something like: > "DES/DES-EDE should consistently check 'key' and 'offset' arguments"? i) Even though the compatibility risk should be small, you still need a CSR because this is technically a change to the specification to document long-standing implementation behavior. ii) I think it would be good to add the other affected classes to the synopsis. I don't think you have to mention the other parameters. I'll try to catch up with the code review later. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30069#discussion_r2905298545
