[security-dev 00848]: Re: Code review request: 6813340: X509Factory should not depend on is.available()==0

[Max (Weijun) Wang]

On May 25, 2009, at 6:47 PM, Xuelei Fan wrote:

> 1.
> 116         byte[] buffer = new byte[length];
>
> I would like to use a fixed length, such as 1024, for memory saving  
> for large data. Just a very very personal preference.

I see what you mean. My attempt was read it in a single shot. Will  
think about it.

>
> 2. 562 ~ 567
> You assume that the end character of a line is the same for all  
> lines. That's OK, I just worry about the situation that a program  
> generated input steam, which would include more than one of CR, LF,  
> CR+LF as line end character. Hopefully, it is not a issue.

Guess it's even more less an issue if \n is tried.

>
> 3. 584 ~ EOF
> You assume that the tag occupy only one byte, that's incorrect, the  
> tag would occupy more than one byte when it is bigger than 30. The  
> assume would make the following length parser code incorrect.
>
> You assume that the end of indefinite length is only one zero byte,  
> that's incorrect, it is zero of two bytes.

readBERInternal() reads 2 bytes at EOC, on 588 and 595.

>
> You does not handle content longer than 10K(L>0x82). I don't think  
> 10K is enough, maybe you need to handle 0x83, even 0x84 before  
> regarding the content is too hug.

OK. I would support 0x83 and 0x84. The multiple byte tag is never  
supported by DerValue related classes, so I guess it's not necessary  
to add it here.

Thanks
Max

>
> Weijun Wang wrote:
>> Hi Sean and/or Andrew
>>
>> Can any of you take a review at this bug fix:
>>
>>  bug:
>>     http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6813340
>>  webrev:
>>     http://cr.openjdk.java.net/~weijun/6813340/webrev.02/
>>
>> The bug is about too many is.available() usage in
>> X509Factory.generateXXX() methods. This means a slow stream might  
>> not be
>> consumed at all.
>>
>> The fix introduces a new method readOneBlock() which reads a block of
>> data, either PEM or DER, in block mode. The method neither uses
>> available() nor performs any mark/reset actions.
>>
>> There might be two drawbacks for this code change:
>>
>> 1. In order to avoid mark/reset, it uses a heuristic method to detect
>> the line ending of a PEM file: If the -----BEGIN----- line ends with
>> '\r' (or \n), it assumes the -----END---- line also ends with it. I
>> don't know if there are files which is hybrid. I might make another
>> change if you want more safety:
>>
>> 564  -               if (next == -1 || next == end) {
>> 564  +               if (next == -1 || next == end || next == '\n') {
>>
>>
> See above, I would like the safer one.
>
> Andrew
>> 2. Since no buffering is made, the performance might hurt. However, I
>> simply browse the usage in JDK and find many callers actually use a
>> ByteArrayInputStream, so this is not a serious problem.
>>
>> Thanks
>> Max
>>