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, e...@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
