code review request: 8012971 PKCS11Test hiding exception failures

[Anthony Scarpino]

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
>>>>
>>>
>>
>