code review request: 8012971 PKCS11Test hiding exception failures

[Valerie (Yu-Ching) Peng]

Looks good~
Thanks,
Valerie

On 07/25/13 15:17, Anthony Scarpino wrote:
> On 07/24/2013 06:37 PM, Valerie (Yu-Ching) Peng wrote:
>> On 07/24/13 13:53, Anthony Scarpino wrote:
>>> On 07/23/2013 06:00 PM, Valerie (Yu-Ching) Peng wrote:
>>>>
>>>> <PKCS11Test.java>
>>>>
>>>> 263                 System.arraycopy(data, 900, data, 0, 100);
>>>> 264                 is.read(data, 0,  900);
>>>>
>>>> Do you really mean to overwrite the data[0..99] that you just 
>>>> copied on
>>>> line 263 with line 264?
>>>
>>>
>>> Good catch.. I would have thought all my testing would have tripped me
>>> up on this, but it appears it didn't.
>>>
>>>> In addition, don't you want to know how much is read in order to 
>>>> exclude
>>>> the data from earlier read(...) calls in case that the current read 
>>>> only
>>>> returns a few bytes?
>>>
>>> Yeah, the end of the file read is a bit sloppy, even though it should
>>> never see the end of the file as all the nss libraries have the header
>>> in it.
>>
>> Hmm, I think the String object should be constructed w/ the bytes that's
>> just read, i.e. new String(data, 0, read)
>>
>>   256                 s = new String(data);
>>
>> And line 272 should be update to "read = 100 + is.read(data, 100, 900);"
>> Also, the value of variable s is only set once on line 256 and yet the
>> content of 'data' may change on line 271-272. This may lead to
>> inconsistency between 's' and 'data' if the while-loop (line 255) is
>> exited due to read <= 0 on line 272.
>
> I wasn't overly concerned about what happens at the end of the file 
> because it shouldn't get to that point.  There should always be a 
> header and if one was not found, the check has for the most part failed.
> 'data' is not used outside the loop, so any inconsistency with the 
> String isn't a problem.
>
> That being said, I'm fine with changing it.  I couldn't add 100 to the 
> read because the while-loop looks for read to equal 0, and the 
> inconsistency on the read length made the original code troublesome, 
> so I rewrote it and I think it looks cleaner now.
>
> http://cr.openjdk.java.net/~ascarpino/8020424/webrev.02/
>
>>>>
>>>> After seeing all these checks and list of known bugs for testing 
>>>> against
>>>> NSS, I think we probably need a README or some wiki page to keep track
>>>> all this...
>>>
>>> I can throw a readme in the directory with the bug IDs and even my
>>> theory that the DER is 480280.
>>>
>> I didn't find the NSS pre-3.12 check in TestECDH.java?
>
> Yeah, that one has a different check.  I'll remove it from that comment.
>
>> Thanks,
>> Valerie
>>
>>> webrev updated at:
>>> http://cr.openjdk.java.net/~ascarpino/8020424/webrev.01/
>>>
>>>> Thanks,
>>>> Valerie
>>>>
>>>> On 07/17/13 13:51, Anthony Scarpino wrote:
>>>>> JDK-8020424 The NSS version should be detected before running crypto
>>>>> tests
>>>>> http://cr.openjdk.java.net/~ascarpino/8020424/webrev.00/
>>>>>
>>>>> Tony
>>>>>
>>>>
>>>
>>
>