From 04f27a16946dc0f1ce4d42d404d065a39082173b Mon Sep 17 00:00:00 2001 From: Harald Kuhr Date: Sat, 8 Sep 2018 16:21:01 +0200 Subject: [PATCH] #438 CompoundDocument file descriptor fix --- .../io/AbstractCachedSeekableStream.java | 8 ++- .../io/FileCacheSeekableStream.java | 26 ++++--- .../io/LittleEndianDataInputStream.java | 1 - .../io/LittleEndianDataOutputStream.java | 1 - .../io/LittleEndianRandomAccessFile.java | 1 - .../io/MemoryCacheSeekableStream.java | 7 +- .../io/ole2/CompoundDocument.java | 54 +++++++++++--- .../io/ole2/CompoundDocumentTest.java | 70 +++++++++---------- .../io/ole2/CompoundDocument_StreamTest.java | 32 +-------- 9 files changed, 107 insertions(+), 93 deletions(-) diff --git a/common/common-io/src/main/java/com/twelvemonkeys/io/AbstractCachedSeekableStream.java b/common/common-io/src/main/java/com/twelvemonkeys/io/AbstractCachedSeekableStream.java index 17d9038f..418760c5 100755 --- a/common/common-io/src/main/java/com/twelvemonkeys/io/AbstractCachedSeekableStream.java +++ b/common/common-io/src/main/java/com/twelvemonkeys/io/AbstractCachedSeekableStream.java @@ -186,7 +186,7 @@ abstract class AbstractCachedSeekableStream extends SeekableInputStream { } protected void closeImpl() throws IOException { - cache.flush(position); + cache.close(); cache = null; stream.close(); } @@ -198,12 +198,12 @@ abstract class AbstractCachedSeekableStream extends SeekableInputStream { * @author last modified by $Author: haku $ * @version $Id: //depot/branches/personal/haraldk/twelvemonkeys/release-2/twelvemonkeys-core/src/main/java/com/twelvemonkeys/io/AbstractCachedSeekableStream.java#2 $ */ - public static abstract class StreamCache { + static abstract class StreamCache { /** * Creates a {@code StreamCache}. */ - protected StreamCache() { + StreamCache() { } /** @@ -303,5 +303,7 @@ abstract class AbstractCachedSeekableStream extends SeekableInputStream { * @throws IOException if the position can't be determined because of a problem in the cache backing mechanism. */ abstract long getPosition() throws IOException; + + abstract void close() throws IOException; } } diff --git a/common/common-io/src/main/java/com/twelvemonkeys/io/FileCacheSeekableStream.java b/common/common-io/src/main/java/com/twelvemonkeys/io/FileCacheSeekableStream.java index bfdbaed8..a857b817 100755 --- a/common/common-io/src/main/java/com/twelvemonkeys/io/FileCacheSeekableStream.java +++ b/common/common-io/src/main/java/com/twelvemonkeys/io/FileCacheSeekableStream.java @@ -124,7 +124,9 @@ public final class FileCacheSeekableStream extends AbstractCachedSeekableStream @Override protected void closeImpl() throws IOException { + // TODO: Close cache file super.closeImpl(); + buffer = null; } @@ -194,39 +196,45 @@ public final class FileCacheSeekableStream extends AbstractCachedSeekableStream return length; } + // TODO: We need to close the cache file!!! Otherwise we are leaking file descriptors + final static class FileCache extends StreamCache { - private RandomAccessFile mCacheFile; + private RandomAccessFile cacheFile; public FileCache(final File pFile) throws FileNotFoundException { Validate.notNull(pFile, "file"); - mCacheFile = new RandomAccessFile(pFile, "rw"); + cacheFile = new RandomAccessFile(pFile, "rw"); } public void write(final int pByte) throws IOException { - mCacheFile.write(pByte); + cacheFile.write(pByte); } @Override public void write(final byte[] pBuffer, final int pOffset, final int pLength) throws IOException { - mCacheFile.write(pBuffer, pOffset, pLength); + cacheFile.write(pBuffer, pOffset, pLength); } public int read() throws IOException { - return mCacheFile.read(); + return cacheFile.read(); } @Override public int read(final byte[] pBuffer, final int pOffset, final int pLength) throws IOException { - return mCacheFile.read(pBuffer, pOffset, pLength); + return cacheFile.read(pBuffer, pOffset, pLength); } public void seek(final long pPosition) throws IOException { - mCacheFile.seek(pPosition); + cacheFile.seek(pPosition); } public long getPosition() throws IOException { - return mCacheFile.getFilePointer(); + return cacheFile.getFilePointer(); + } + + @Override + void close() throws IOException { + cacheFile.close(); } } - } diff --git a/common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianDataInputStream.java b/common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianDataInputStream.java index 401b92dd..5f9276f9 100755 --- a/common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianDataInputStream.java +++ b/common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianDataInputStream.java @@ -53,7 +53,6 @@ import java.io.*; * which this class imitates reads big endian quantities. *

* Warning: - * * The {@code DataInput} and {@code DataOutput} interfaces * specifies big endian byte order in their documentation. * This means that this class is, strictly speaking, not a proper diff --git a/common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianDataOutputStream.java b/common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianDataOutputStream.java index 84d6469f..23c112f7 100755 --- a/common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianDataOutputStream.java +++ b/common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianDataOutputStream.java @@ -52,7 +52,6 @@ import java.io.*; * imitates uses big endian integers. *

* Warning: - * * The {@code DataInput} and {@code DataOutput} interfaces * specifies big endian byte order in their documentation. * This means that this class is, strictly speaking, not a proper diff --git a/common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianRandomAccessFile.java b/common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianRandomAccessFile.java index 73b222f5..de2b32d0 100755 --- a/common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianRandomAccessFile.java +++ b/common/common-io/src/main/java/com/twelvemonkeys/io/LittleEndianRandomAccessFile.java @@ -38,7 +38,6 @@ import java.nio.channels.FileChannel; * and writing data in little endian byte order. *

* Warning: - * * The {@code DataInput} and {@code DataOutput} interfaces * specifies big endian byte order in their documentation. * This means that this class is, strictly speaking, not a proper diff --git a/common/common-io/src/main/java/com/twelvemonkeys/io/MemoryCacheSeekableStream.java b/common/common-io/src/main/java/com/twelvemonkeys/io/MemoryCacheSeekableStream.java index a60bc8a4..d17546b6 100755 --- a/common/common-io/src/main/java/com/twelvemonkeys/io/MemoryCacheSeekableStream.java +++ b/common/common-io/src/main/java/com/twelvemonkeys/io/MemoryCacheSeekableStream.java @@ -67,7 +67,7 @@ public final class MemoryCacheSeekableStream extends AbstractCachedSeekableStrea final static class MemoryCache extends StreamCache { final static int BLOCK_SIZE = 1 << 13; - private final List cache = new ArrayList(); + private final List cache = new ArrayList<>(); private long length; private long position; private long start; @@ -186,6 +186,11 @@ public final class MemoryCacheSeekableStream extends AbstractCachedSeekableStrea start = pPosition; } + @Override + void close() throws IOException { + cache.clear(); + } + public long getPosition() { return position; } diff --git a/common/common-io/src/main/java/com/twelvemonkeys/io/ole2/CompoundDocument.java b/common/common-io/src/main/java/com/twelvemonkeys/io/ole2/CompoundDocument.java index 91cab4a5..f33d0c21 100755 --- a/common/common-io/src/main/java/com/twelvemonkeys/io/ole2/CompoundDocument.java +++ b/common/common-io/src/main/java/com/twelvemonkeys/io/ole2/CompoundDocument.java @@ -30,11 +30,14 @@ package com.twelvemonkeys.io.ole2; +import static com.twelvemonkeys.lang.Validate.notNull; + import com.twelvemonkeys.io.*; import com.twelvemonkeys.lang.StringUtil; import javax.imageio.stream.ImageInputStream; import java.io.*; +import java.nio.ByteOrder; import java.util.Arrays; import java.util.SortedSet; import java.util.TreeSet; @@ -52,7 +55,7 @@ import java.util.UUID; * @author last modified by $Author: haku $ * @version $Id: //depot/branches/personal/haraldk/twelvemonkeys/release-2/twelvemonkeys-core/src/main/java/com/twelvemonkeys/io/ole2/CompoundDocument.java#4 $ */ -public final class CompoundDocument { +public final class CompoundDocument implements AutoCloseable { // TODO: Write support... // TODO: Properties: http://support.microsoft.com/kb/186898 @@ -96,13 +99,18 @@ public final class CompoundDocument { /** * Creates a (for now) read only {@code CompoundDocument}. + *

+ * Warning! You must invoke {@link #close()} on the compound document + * created from this constructor when done, to avoid leaking file + * descriptors. * - * @param pFile the file to read from + * @param file the file to read from * * @throws IOException if an I/O exception occurs while reading the header */ - public CompoundDocument(final File pFile) throws IOException { - input = new LittleEndianRandomAccessFile(FileUtil.resolve(pFile), "r"); + public CompoundDocument(final File file) throws IOException { + // TODO: We need to close this (or it's underlying RAF)! Otherwise we're leaking file descriptors! + input = new LittleEndianRandomAccessFile(FileUtil.resolve(file), "r"); // TODO: Might be better to read header on first read operation?! // OTOH: It's also good to be fail-fast, so at least we should make @@ -113,17 +121,17 @@ public final class CompoundDocument { /** * Creates a read only {@code CompoundDocument}. * - * @param pInput the input to read from + * @param pInput the input to read from. * * @throws IOException if an I/O exception occurs while reading the header */ public CompoundDocument(final InputStream pInput) throws IOException { - this(new FileCacheSeekableStream(pInput)); + this(new MemoryCacheSeekableStream(pInput)); } // For testing only, consider exposing later - CompoundDocument(final SeekableInputStream pInput) throws IOException { - input = new SeekableLittleEndianDataInputStream(pInput); + CompoundDocument(final SeekableInputStream stream) throws IOException { + input = new SeekableLittleEndianDataInputStream(stream); // TODO: Might be better to read header on first read operation?! // OTOH: It's also good to be fail-fast, so at least we should make @@ -134,12 +142,15 @@ public final class CompoundDocument { /** * Creates a read only {@code CompoundDocument}. * - * @param pInput the input to read from + * @param input the input to read from * * @throws IOException if an I/O exception occurs while reading the header */ - public CompoundDocument(final ImageInputStream pInput) throws IOException { - input = pInput; + public CompoundDocument(final ImageInputStream input) throws IOException { + this.input = notNull(input, "input"); + + // This implementation only supports little endian (Intel) CompoundDocuments + input.setByteOrder(ByteOrder.LITTLE_ENDIAN); // TODO: Might be better to read header on first read operation?! // OTOH: It's also good to be fail-fast, so at least we should make @@ -147,6 +158,27 @@ public final class CompoundDocument { readHeader(); } + /** + * This method will close the underlying {@link RandomAccessFile} if any, + * but will leave any stream created outside the document open. + * + * @see #CompoundDocument(File) + * @see RandomAccessFile#close() + * + * @throws IOException if an I/O error occurs. + */ + @Override + public void close() throws IOException { + if (input instanceof RandomAccessFile) { + ((RandomAccessFile) input).close(); + } + else if (input instanceof LittleEndianRandomAccessFile) { + ((LittleEndianRandomAccessFile) input).close(); + } + + // Other streams are left open + } + public static boolean canRead(final DataInput pInput) { return canRead(pInput, true); } diff --git a/common/common-io/src/test/java/com/twelvemonkeys/io/ole2/CompoundDocumentTest.java b/common/common-io/src/test/java/com/twelvemonkeys/io/ole2/CompoundDocumentTest.java index cef8106f..a9efc698 100755 --- a/common/common-io/src/test/java/com/twelvemonkeys/io/ole2/CompoundDocumentTest.java +++ b/common/common-io/src/test/java/com/twelvemonkeys/io/ole2/CompoundDocumentTest.java @@ -72,64 +72,64 @@ public class CompoundDocumentTest { @Test public void testRoot() throws IOException { - CompoundDocument document = createTestDocument(); + try (CompoundDocument document = createTestDocument()) { + Entry root = document.getRootEntry(); - Entry root = document.getRootEntry(); - - assertNotNull(root); - assertEquals("Root Entry", root.getName()); - assertTrue(root.isRoot()); - assertFalse(root.isFile()); - assertFalse(root.isDirectory()); - assertEquals(0, root.length()); - assertNull(root.getInputStream()); + assertNotNull(root); + assertEquals("Root Entry", root.getName()); + assertTrue(root.isRoot()); + assertFalse(root.isFile()); + assertFalse(root.isDirectory()); + assertEquals(0, root.length()); + assertNull(root.getInputStream()); + } } @Test public void testContents() throws IOException { - CompoundDocument document = createTestDocument(); + try (CompoundDocument document = createTestDocument()) { + Entry root = document.getRootEntry(); - Entry root = document.getRootEntry(); + assertNotNull(root); - assertNotNull(root); + SortedSet children = new TreeSet(root.getChildEntries()); + assertEquals(25, children.size()); - SortedSet children = new TreeSet(root.getChildEntries()); - assertEquals(25, children.size()); - - // Weirdness in the file format, name is *written backwards* 1-24 + Catalog - for (String name : "1,2,3,4,5,6,7,8,9,01,02,11,12,21,22,31,32,41,42,51,61,71,81,91,Catalog".split(",")) { - assertEquals(name, children.first().getName()); - children.remove(children.first()); + // Weirdness in the file format, name is *written backwards* 1-24 + Catalog + for (String name : "1,2,3,4,5,6,7,8,9,01,02,11,12,21,22,31,32,41,42,51,61,71,81,91,Catalog".split(",")) { + assertEquals(name, children.first().getName()); + children.remove(children.first()); + } } } @Test(expected = UnsupportedOperationException.class) public void testChildEntriesUnmodifiable() throws IOException { - CompoundDocument document = createTestDocument(); + try (CompoundDocument document = createTestDocument()) { + Entry root = document.getRootEntry(); - Entry root = document.getRootEntry(); + assertNotNull(root); - assertNotNull(root); + SortedSet children = root.getChildEntries(); - SortedSet children = root.getChildEntries(); - - // Should not be allowed, as it modifies the internal structure - children.remove(children.first()); + // Should not be allowed, as it modifies the internal structure + children.remove(children.first()); + } } @Test public void testReadThumbsCatalogFile() throws IOException { - CompoundDocument document = createTestDocument(); + try (CompoundDocument document = createTestDocument()) { + Entry root = document.getRootEntry(); - Entry root = document.getRootEntry(); + assertNotNull(root); + assertEquals(25, root.getChildEntries().size()); - assertNotNull(root); - assertEquals(25, root.getChildEntries().size()); + Entry catalog = root.getChildEntry("Catalog"); - Entry catalog = root.getChildEntry("Catalog"); - - assertNotNull(catalog); - assertNotNull("Input stream may not be null", catalog.getInputStream()); + assertNotNull(catalog); + assertNotNull("Input stream may not be null", catalog.getInputStream()); + } } @Test diff --git a/common/common-io/src/test/java/com/twelvemonkeys/io/ole2/CompoundDocument_StreamTest.java b/common/common-io/src/test/java/com/twelvemonkeys/io/ole2/CompoundDocument_StreamTest.java index be188623..95f733ad 100644 --- a/common/common-io/src/test/java/com/twelvemonkeys/io/ole2/CompoundDocument_StreamTest.java +++ b/common/common-io/src/test/java/com/twelvemonkeys/io/ole2/CompoundDocument_StreamTest.java @@ -51,35 +51,7 @@ import static org.junit.Assert.*; * @author last modified by $Author: haraldk$ * @version $Id: CompoundDocument_StreamTestCase.java,v 1.0 13.10.11 12:01 haraldk Exp$ */ -//@Ignore("Need proper in-memory creation of CompoundDocuments") public class CompoundDocument_StreamTest extends InputStreamAbstractTest { - private static final String SAMPLE_DATA = "/Thumbs-camera.db"; - - protected final CompoundDocument createTestDocument() throws IOException { - URL input = getClass().getResource(SAMPLE_DATA); - - assertNotNull("Missing test resource!", input); - assertEquals("Test resource not a file:// resource", "file", input.getProtocol()); - - try { - return new CompoundDocument(new File(input.toURI())); - } - catch (URISyntaxException e) { - throw new AssertionError(e); - } - } - - private SeekableInputStream createRealInputStream() { - try { - Entry first = createTestDocument().getRootEntry().getChildEntries().first(); - assertNotNull(first); - return first.getInputStream(); - } - catch (IOException e) { - throw new AssertionError(e); - } - } - @Override protected InputStream makeInputStream(byte[] data) { try { @@ -182,15 +154,13 @@ public class CompoundDocument_StreamTest extends InputStreamAbstractTest { return pad; } -// @Ignore @Test - public void testDev() throws IOException { + public void testStreamRead() throws IOException { InputStream stream = makeInputStream(makeOrderedArray(32)); int read; int count = 0; while ((read = stream.read()) >= 0) { -// System.out.printf("read %02d: 0x%02x%n", count, read & 0xFF); assertEquals(count, read); count++; }