From fd201ea2c40945af73761d42b1a7e5ab102424d4 Mon Sep 17 00:00:00 2001 From: scottmitch Date: Tue, 27 Jan 2015 14:34:00 -0500 Subject: [PATCH] Possible leak in AbstractDiskHttpData Motivation: SonarQube (clinker.netty.io/sonar) reported a resource which may not have been properly closed in all situations in AbstractDiskHttpData. Modifications: - Ensure file channels are closed in the presence of exceptions. - Correct instances where local channels were created but potentially not closed. Result: Less leaks. Less SonarQube vulnerabilities. --- .../http/multipart/AbstractDiskHttpData.java | 111 ++++++++++++------ 1 file changed, 75 insertions(+), 36 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java index 1f19322e88..633d2815ab 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/AbstractDiskHttpData.java @@ -117,16 +117,18 @@ public abstract class AbstractDiskHttpData extends AbstractHttpData { return; } FileOutputStream outputStream = new FileOutputStream(file); - FileChannel localfileChannel = outputStream.getChannel(); - ByteBuffer byteBuffer = buffer.nioBuffer(); - int written = 0; - while (written < size) { - written += localfileChannel.write(byteBuffer); + try { + FileChannel localfileChannel = outputStream.getChannel(); + ByteBuffer byteBuffer = buffer.nioBuffer(); + int written = 0; + while (written < size) { + written += localfileChannel.write(byteBuffer); + } + buffer.readerIndex(buffer.readerIndex() + written); + localfileChannel.force(false); + } finally { + outputStream.close(); } - buffer.readerIndex(buffer.readerIndex() + written); - localfileChannel.force(false); - localfileChannel.close(); - outputStream.close(); completed = true; } finally { // Release the buffer as it was retained before and we not need a reference to it at all @@ -205,18 +207,21 @@ public abstract class AbstractDiskHttpData extends AbstractHttpData { } file = tempFile(); FileOutputStream outputStream = new FileOutputStream(file); - FileChannel localfileChannel = outputStream.getChannel(); - byte[] bytes = new byte[4096 * 4]; - ByteBuffer byteBuffer = ByteBuffer.wrap(bytes); - int read = inputStream.read(bytes); int written = 0; - while (read > 0) { - byteBuffer.position(read).flip(); - written += localfileChannel.write(byteBuffer); - read = inputStream.read(bytes); + try { + FileChannel localfileChannel = outputStream.getChannel(); + byte[] bytes = new byte[4096 * 4]; + ByteBuffer byteBuffer = ByteBuffer.wrap(bytes); + int read = inputStream.read(bytes); + while (read > 0) { + byteBuffer.position(read).flip(); + written += localfileChannel.write(byteBuffer); + read = inputStream.read(bytes); + } + localfileChannel.force(false); + } finally { + outputStream.close(); } - localfileChannel.force(false); - localfileChannel.close(); size = written; if (definedSize > 0 && definedSize < size) { file.delete(); @@ -269,7 +274,7 @@ public abstract class AbstractDiskHttpData extends AbstractHttpData { return EMPTY_BUFFER; } if (fileChannel == null) { - FileInputStream inputStream = new FileInputStream(file); + FileInputStream inputStream = new FileInputStream(file); fileChannel = inputStream.getChannel(); } int read = 0; @@ -327,20 +332,51 @@ public abstract class AbstractDiskHttpData extends AbstractHttpData { } if (!file.renameTo(dest)) { // must copy - FileInputStream inputStream = new FileInputStream(file); - FileOutputStream outputStream = new FileOutputStream(dest); - FileChannel in = inputStream.getChannel(); - FileChannel out = outputStream.getChannel(); + IOException exception = null; + FileInputStream inputStream = null; + FileOutputStream outputStream = null; long chunkSize = 8196; long position = 0; - while (position < size) { - if (chunkSize < size - position) { - chunkSize = size - position; + try { + inputStream = new FileInputStream(file); + outputStream = new FileOutputStream(dest); + FileChannel in = inputStream.getChannel(); + FileChannel out = outputStream.getChannel(); + while (position < size) { + if (chunkSize < size - position) { + chunkSize = size - position; + } + position += in.transferTo(position, chunkSize , out); + } + } catch (IOException e) { + exception = e; + } finally { + if (inputStream != null) { + try { + inputStream.close(); + } catch (IOException e) { + if (exception == null) { // Choose to report the first exception + exception = e; + } else { + logger.warn("Multiple exceptions detected, the following will be suppressed {}", e); + } + } + } + if (outputStream != null) { + try { + outputStream.close(); + } catch (IOException e) { + if (exception == null) { // Choose to report the first exception + exception = e; + } else { + logger.warn("Multiple exceptions detected, the following will be suppressed {}", e); + } + } } - position += in.transferTo(position, chunkSize , out); } - in.close(); - out.close(); + if (exception != null) { + throw exception; + } if (position == size) { file.delete(); file = dest; @@ -367,14 +403,17 @@ public abstract class AbstractDiskHttpData extends AbstractHttpData { "File too big to be loaded in memory"); } FileInputStream inputStream = new FileInputStream(src); - FileChannel fileChannel = inputStream.getChannel(); byte[] array = new byte[(int) srcsize]; - ByteBuffer byteBuffer = ByteBuffer.wrap(array); - int read = 0; - while (read < srcsize) { - read += fileChannel.read(byteBuffer); + try { + FileChannel fileChannel = inputStream.getChannel(); + ByteBuffer byteBuffer = ByteBuffer.wrap(array); + int read = 0; + while (read < srcsize) { + read += fileChannel.read(byteBuffer); + } + } finally { + inputStream.close(); } - fileChannel.close(); return array; }