On Mon, 9 Mar 2026 13:03:39 GMT, Sean Mullan <[email protected]> wrote:
>> 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. It did occur to me that with the previous code, sometimes you could get `IKE` and sometimes `AIOBE` depending on the values. For example, for `DESEdeKeySpec`, a key of length 10 and an offset of -1 would throw `IKE` but a key length of 24 and offset of -1 would throw AIOBE. One of these is a checked exception, and the other unchecked, so applications could handle these differently. These classes should not be used much and these are invalid values, but just to be safe, I would probably keep the key length check before the negative offset check to preserve the above behavior. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30069#discussion_r2921057945
