Code review request for 6687725 and 6848930

[Valerie (Yu-Ching) Peng]

On 11/04/10 12:32, Brad Wetmore wrote:
>
> I've been so focused on getting TLS 1.2 done, this has been really low 
> priority.
>
> > 6687725: Internal PKCS5Padding impl should throw
> > IllegalBlockSizeException and not BadPaddingException
> > Webrev - http://cr.openjdk.java.net/~valeriep/6687725
>
> Should you continuing to check for negative numbers?  I don't know 
> when/if a native impl C_DecryptFinal might actually return -1 (some 
> error condition?), but it seems this check might have been there for a 
> reason.
Error conditions should lead to Exception being thrown.
But who knows what buggy code underneath may return. I can add a check 
of (len > 0) at line 100 to be absolutely safe.

>
> While not specifically addressed in the JavaSoft Code Conventions, my 
> understanding of breaking lines has been to break method declarations 
> as such:
>
>     throw new IllegalBlockSizeException(
>         "Input length must be multiples of " + blockSize);
Well, since it's not specifically addressed in the JavaSoft Code 
Conventions, I'd prefer to adopt the same style as the rest of file so 
at least things look consistent.

>
> > 6848930: JSN security test jce/Global/Cipher/PKCS5Padding cannot thrown
> > expected BadPaddingException
> > Webrev - http://cr.openjdk.java.net/~valeriep/6848930
>
> Question, for the other pads such as CKM_RC2_CBC_PAD/CKM_RC5_CBC_PAD. 
> You're not including them here because the SunPKCS11 provider doesn't 
> provide access to them?
Correct.
Thanks,
Valerie

>
> Otherwise, looks fine.
>
> Brad
>
>
> On 8/31/2010 5:20 PM, Valerie (Yu-Ching) Peng wrote:
>> Hi, Brad,
>>
>> Do you have time to review these two PKCS11 fixes? They are straight
>> forward.
>>
>> 6687725: Internal PKCS5Padding impl should throw
>> IllegalBlockSizeException and not BadPaddingException
>> Webrev - http://cr.openjdk.java.net/~valeriep/6687725
>>
>> 6848930: JSN security test jce/Global/Cipher/PKCS5Padding cannot thrown
>> expected BadPaddingException
>> Webrev - http://cr.openjdk.java.net/~valeriep/6848930
>> Note: This is same as the changes made for 6U release, to workaround
>> different Solaris impls.
>>
>> Thanks!
>> Valerie