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

[Xuelei Fan]

Max (Weijun) Wang wrote:
>
> 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.
>
OK, I just realized line 605 is incorrect, which will treat the content 
as a TLV component, which is not always correct. For example, if the 
indefinite-length is "0x80 0x06 0x07 0x01 0x00 0x00", "0x06 0x07 0x01" 
is the content bytes, the 0x06 will be parsed as tag, the ox07 will be 
parsed as length, and then try to read 0x07 bytes from the stream.

If you want to support indefinite-length, I think you can simply keep 
reading until get two zero bytes.
>>
>> 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.
>
That's not a big issue, because the method is only used to parse a 
SEQUENCE tag.

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