RFR [9] 8139706: JarFile.getBytes could use InputStream.readNBytes

[Xueming Shen]

On 10/16/15 4:49 AM, Claes Redestad wrote:
> On 2015-10-16 04:09, Xueming Shen wrote:
>> On 10/15/15 3:08 PM, Claes Redestad wrote:
>>>
>>> On 2015-10-15 23:21, Chris Hegarty wrote:
>>>>> On 15 Oct 2015, at 21:59, ecki at zusammenkunft.net wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> This does change a bit the semantic of the length check. If the 
>>>>> stream would return more bytes than the zipentry says the new 
>>>>> version would ignore them, the  old version was consuming then and 
>>>>> then fail the check. However I am not sure if this is relevant.
>>>> Right, there are certainly some subtle differences resulting from
>>>> the proposed change. When working on JDK-8138978 I thought
>>>> about using readNBytes, but played it safe as IOUtils was growing
>>>> the bye[] lazily too, so no real perf difference.  In fact, I think 
>>>> I seen
>>>> a test failure with using readNBytes here. I’ll have to check.
>>>
>>> Seeing no jtreg test failures in java/util/zip nor java/util/jar 
>>> (apart from 2 ignored tests), but I can see a reason for the current 
>>> implementation being conservative: Corrupt/malicious jar files might 
>>> lie about the entry length and report very large values, which could 
>>> bring a down with OOME.
>>>
>>> I believe we could be both safe and faster than baseline by adding a 
>>> reasonable limit for when to use readNBytes, e.g., 32k would deal 
>>> with the majority of .class files.
>>>
>> getBytes should be used to read the meta-inf files only, not the 
>> .class files.
>
> Correct, but this is still enough to cause statistically significant 
> increases on our footprint measures.
>
> With a reasonable trust limit this change should be safe while 
> avoiding most temporary byte[] allocations when reading meta-inf 
> files. I've verified the startup and footprint numbers and ran it 
> through all java/util/jar and /zip tests without error.
>
> New webrev: http://cr.openjdk.java.net/~redestad/8139706/webrev.02/
>
Why do we no longer check the length of the returned byte[] from 
is.readAllBytes() against ze.getSize()?
I think the original IOUtils.readFully() throws EOFE if we don't get 
enough bytes.

-Sherman