Discussion:
Resource size hints appear to be used incorrectly in JPMS
(too old to reply)
Luke Hutchison
2018-08-08 05:19:06 UTC
Permalink
The uncompressed size of a resource is used in
BasicImageReader#getResource(ImageLocation), which calls readBuffer(offset,
uncompressedSize) . The readBuffer method then does two or three things
that seem incorrect if the size is obtained from ZipEntry#getSize():

(1) size < 0 is rejected, but ZipEntry#getSize() returns "the uncompressed
size of the entry data, or -1 if not known":

if (size < 0 || Integer.MAX_VALUE <= size) {
throw new IndexOutOfBoundsException("Bad size: " + size);
}

(2) size is used directly without restriction, rather than as a size hint.
ZipEntry#getSize() may actually return a totally bogus value (you can write
anything you want for the uncompressed size in a zipfile entry). If the
requested size is large, e.g. something close to Integer.MAX_VALUE, large
allocations will occur every time a resource is read, which can be used as
a memory allocation attack vector.

ByteBuffer buffer = slice(memoryMap, (int)offset, (int)size);
// ...
ByteBuffer buffer = ImageBufferCache.getBuffer(size);

Instead, the initial buffer size should be set to something like
Math.min(16 * 1024 * 1024, size), and then grown as needed if the resource
is indeed larger than this initial allocation.

(3) The BasicImageReader#readBuffer(long offset, long size) checks if the
filesize is larger than Integer.MAX_VALUE. However, InputStream contains
the following max limit for byte array sizes:

/**
* The maximum size of array to allocate.
* Some VMs reserve some header words in an array.
* Attempts to allocate larger arrays may result in
* OutOfMemoryError: Requested array size exceeds VM limit
*/
private static final int MAX_BUFFER_SIZE = Integer.MAX_VALUE - 8;

If this limit applies to ByteBuffers (and it does apply to ByteBuffers if
they are array-backed), then the check of size against Integer.MAX_VALUE is
incorrect.
Alan Bateman
2018-08-08 12:22:08 UTC
Permalink
Post by Luke Hutchison
(2) size is used directly without restriction, rather than as a size hint.
ZipEntry#getSize() may actually return a totally bogus value (you can write
anything you want for the uncompressed size in a zipfile entry). If the
requested size is large, e.g. something close to Integer.MAX_VALUE, large
allocations will occur every time a resource is read, which can be used as
a memory allocation attack vector.
I've changed the subject line as the original subject line suggested an
issue with the module system. Instead, I think your mail is about
run-time images created with `jlink --compress <level>`. The JDK
run-time images do not use compression so the code paths in the jimage
code for compressed resources may not be tested as well as the
uncompressed case. If there are bugs or inefficiencies in the handling
of compressed resources then we should get them into JIRA.

-Alan
Luke Hutchison
2018-08-09 09:42:36 UTC
Permalink
Good info, I was wondering about the claims (in the ModuleReader javadoc)
about how ByteBuffer was being used for high-speed classloading, given that
in the past rt.jar has been compressed. This makes a lot more sense if the
system modules are not compressed.

So yes, the issues I raised are about the use of decompressed sizes in the
loading of resources from runtime images that were created with compression.
Post by Alan Bateman
Post by Luke Hutchison
(2) size is used directly without restriction, rather than as a size
hint.
Post by Luke Hutchison
ZipEntry#getSize() may actually return a totally bogus value (you can
write
Post by Luke Hutchison
anything you want for the uncompressed size in a zipfile entry). If the
requested size is large, e.g. something close to Integer.MAX_VALUE, large
allocations will occur every time a resource is read, which can be used
as
Post by Luke Hutchison
a memory allocation attack vector.
I've changed the subject line as the original subject line suggested an
issue with the module system. Instead, I think your mail is about
run-time images created with `jlink --compress <level>`. The JDK
run-time images do not use compression so the code paths in the jimage
code for compressed resources may not be tested as well as the
uncompressed case. If there are bugs or inefficiencies in the handling
of compressed resources then we should get them into JIRA.
-Alan
Alan Bateman
2018-08-09 10:06:46 UTC
Permalink
Post by Luke Hutchison
Good info, I was wondering about the claims (in the ModuleReader
javadoc) about how ByteBuffer was being used for high-speed
classloading, given that in the past rt.jar has been compressed. This
makes a lot more sense if the system modules are not compressed.
So yes, the issues I raised are about the use of decompressed sizes in
the loading of resources from runtime images that were created with
compression.
I don't think rt.jar was compressed in JDK 8 or older, at least not
unless you are looking at embedded builds where it helped with the
static footprint (and depending on processor/environment, it may help
with class loading performance too).

The API note (and it's just a note, not normative text) about class
loading in the ModuleReader javadoc was to document the rational for the
read/release methods. The built-in class loaders make heavy use of it
when loading classes/resources from modules in the run-time image and
significant work was done to ensure that the buffers are views on the
underlying memory mapped file.

The support for compressing resources in the jimage is something that
does need attention so I created JDK-8209176 to track that. As I said,
we haven't done as much performance testing with compression enabled.
I'll also add that your comments about malicious values need to be
viewed in the context of the tool that creates the run-time image -
there is no support whatsoever for using jimage files that have not been
created by the jlink tool.

-Alan

Loading...