On Wed, 11 Mar 2026 21:20:08 GMT, Sean Mullan <[email protected]> wrote:
>>> @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. Good point on preserving IKE behavior with invalid key length (with offset) vs. throwing AIOBE with invalid offsets. I've updated the PR accordingly. I believe that we can keep the null key check where they didn't exist before because of the following reasons: i) preserves existing IKE behavior for this scenario ii) good practice to always check and throw exception explicitly before dereferencing ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30069#discussion_r2921637646
