Hi Florian,
On 10/28/17 16:16, Florian Weimer wrote:
* Xueming Shen:
https://bugs.openjdk.java.net/browse/JDK-8187485
http://cr.openjdk.java.net/~sherman/8185582/webrev
In ZipFile:
387 Inflater inf = getInflater();
388 InputStream is = new ZipFileInflaterInputStream(in, inf,
(int)size,
389 () -> releaseInflater(inf));
390 synchronized (streams) {
391 streams.add(is);
392 }
Doesn't this leak the inflater if Cleaner.register() and thus the
ZipFileInflaterInputStream constructor throw?
Thanks for being alert. I think that all that can be thrown between
getInfalter() call and streams.add() successfully returning is
OutOfMemoryError or StackOverflowError. OOME can be thrown anywhere, not
only as a consequence of Cleaner.register(). I see following potential
places where allocation may be taking place:
- constructing the lambda instance (line 389)
- allocating ZipFileInflaterInputStream object
- multiple places in ZipFileInflaterInputStream.<init> and
InflaterInputStream.<init> (including but not limited to Cleaner.register())
- streams.add() (line 391)
Can we do anything about it? Let's see. For example:
Inflater inf = getInflater();
InputStream is;
try {
is = new ZipFileInflaterInputStream(in, inf, (int)size, () ->
releaseInflater(inf));
} catch (OutOfMemoryError e) {
releaseInflater(inf);
throw e;
}
try {
synchronized (streams) {
streams.add(is);
}
} catch (OutOfMemoryError e) {
try { is.close(); } catch (IOException ignore) {}
}
...but, even releaseInflater(inf) may throw OOME (in line 470):
467 private void releaseInflater(Inflater inf) {
468 inf.reset();
469 synchronized (inflaterCache) {
470 inflaterCache.add(inf);
471 }
472 }
So we may change it to the following, which would make it more robust
for other uses too:
private void releaseInflater(Inflater inf) {
inf.reset();
try {
synchronized (inflaterCache) {
inflaterCache.add(inf);
}
} catch (OutOfMemoryError e) {
inf.end();
throw e;
}
}
...and that's it for OOME, hopefully.
But what about StackOverflowError? If we want to go that far, we may
need to create a special minimal critical method which encapsulates the
logic of obtaining Inflater and creating ZipFileInflaterInputStream with
it and then put a @ReservedStackAccess annotation on it. Like the following:
@ReservedStackAccess
private ZipFileInflaterInputStream createZipFileInflaterInputStream(
ZipFileInputStream zfin, int size
) {
Inflater inf = getInflater();
ZipFileInflaterInputStream is;
try {
is = new ZipFileInflaterInputStream(zfin, inf, size,
() ->
releaseInflater(inf));
} catch (OutOfMemoryError e) {
releaseInflater(inf);
throw e;
}
try {
synchronized (streams) {
streams.add(is);
}
} catch (OutOfMemoryError e) {
try {
is.close();
} catch (IOException ignore) {
// not really possible - just making javac happy
}
throw e;
}
return is;
}
What do you think, Sherman?
Regards, Peter
