From a00240f2127b494aaeced2fffe44a6e599b7ea88 Mon Sep 17 00:00:00 2001 From: Dominik Stadler Date: Wed, 14 Oct 2015 14:53:41 +0000 Subject: [PATCH] Bug 58480: Work around problem where on Windows systems a Mapped Buffer can still lock a file even if the Channel was closed properly. Use reflection as DirectBuffer is in package sun.com and thus likely to go away with Java 9. git-svn-id: https://svn.apache.org/repos/asf/poi/trunk@1708609 13f79535-47bb-0310-9956-ffa450edef68 --- .../poi/hssf/usermodel/HSSFWorkbook.java | 58 +++--- .../poi/poifs/nio/FileBackedDataSource.java | 50 ++++- .../poi/hssf/usermodel/TestHSSFWorkbook.java | 49 +++++ .../apache/poi/poifs/nio/TestDataSource.java | 183 ++++++++++++++---- 4 files changed, 272 insertions(+), 68 deletions(-) diff --git a/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java b/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java index 09c306666..fe1eb26d1 100644 --- a/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java +++ b/src/java/org/apache/poi/hssf/usermodel/HSSFWorkbook.java @@ -1372,36 +1372,38 @@ public final class HSSFWorkbook extends POIDocument implements org.apache.poi.ss public void write(OutputStream stream) throws IOException { - byte[] bytes = getBytes(); NPOIFSFileSystem fs = new NPOIFSFileSystem(); - - // For tracking what we've written out, used if we're - // going to be preserving nodes - List excepts = new ArrayList(1); - - // Write out the Workbook stream - fs.createDocument(new ByteArrayInputStream(bytes), "Workbook"); - - // Write out our HPFS properties, if we have them - writeProperties(fs, excepts); - - if (preserveNodes) { - // Don't write out the old Workbook, we'll be doing our new one - // If the file had an "incorrect" name for the workbook stream, - // don't write the old one as we'll use the correct name shortly - excepts.addAll(Arrays.asList(WORKBOOK_DIR_ENTRY_NAMES)); - - // Copy over all the other nodes to our new poifs - EntryUtils.copyNodes( - new FilteringDirectoryNode(this.directory, excepts) - , new FilteringDirectoryNode(fs.getRoot(), excepts) - ); - - // YK: preserve StorageClsid, it is important for embedded workbooks, - // see Bugzilla 47920 - fs.getRoot().setStorageClsid(this.directory.getStorageClsid()); + try { + // For tracking what we've written out, used if we're + // going to be preserving nodes + List excepts = new ArrayList(1); + + // Write out the Workbook stream + fs.createDocument(new ByteArrayInputStream(getBytes()), "Workbook"); + + // Write out our HPFS properties, if we have them + writeProperties(fs, excepts); + + if (preserveNodes) { + // Don't write out the old Workbook, we'll be doing our new one + // If the file had an "incorrect" name for the workbook stream, + // don't write the old one as we'll use the correct name shortly + excepts.addAll(Arrays.asList(WORKBOOK_DIR_ENTRY_NAMES)); + + // Copy over all the other nodes to our new poifs + EntryUtils.copyNodes( + new FilteringDirectoryNode(this.directory, excepts) + , new FilteringDirectoryNode(fs.getRoot(), excepts) + ); + + // YK: preserve StorageClsid, it is important for embedded workbooks, + // see Bugzilla 47920 + fs.getRoot().setStorageClsid(this.directory.getStorageClsid()); + } + fs.writeFilesystem(stream); + } finally { + fs.close(); } - fs.writeFilesystem(stream); } /** diff --git a/src/java/org/apache/poi/poifs/nio/FileBackedDataSource.java b/src/java/org/apache/poi/poifs/nio/FileBackedDataSource.java index 2e4c7cd5f..424fad1ce 100644 --- a/src/java/org/apache/poi/poifs/nio/FileBackedDataSource.java +++ b/src/java/org/apache/poi/poifs/nio/FileBackedDataSource.java @@ -22,10 +22,14 @@ import java.io.FileNotFoundException; import java.io.IOException; import java.io.OutputStream; import java.io.RandomAccessFile; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.nio.ByteBuffer; import java.nio.channels.Channels; import java.nio.channels.FileChannel; import java.nio.channels.WritableByteChannel; +import java.util.ArrayList; +import java.util.List; import org.apache.poi.util.IOUtils; @@ -37,6 +41,14 @@ public class FileBackedDataSource extends DataSource { private boolean writable; // remember file base, which needs to be closed too private RandomAccessFile srcFile; + + // Buffers which map to a file-portion are not closed automatically when the Channel is closed + // therefore we need to keep the list of mapped buffers and do some ugly reflection to try to + // clean the buffer during close(). + // See https://bz.apache.org/bugzilla/show_bug.cgi?id=58480, + // http://stackoverflow.com/questions/3602783/file-access-synchronized-on-java-object and + // http://bugs.java.com/view_bug.do?bug_id=4724038 for related discussions + private List buffersToClean = new ArrayList(); public FileBackedDataSource(File file) throws FileNotFoundException { this(newSrcFile(file, "r"), true); @@ -91,6 +103,9 @@ public class FileBackedDataSource extends DataSource { // Ready it for reading dst.position(0); + // remember the buffer for cleanup if necessary + buffersToClean.add(dst); + // All done return dst; } @@ -115,7 +130,14 @@ public class FileBackedDataSource extends DataSource { @Override public void close() throws IOException { - if (srcFile != null) { + // also ensure that all buffers are unmapped so we do not keep files locked on Windows + // We consider it a bug if a Buffer is still in use now! + for(ByteBuffer buffer : buffersToClean) { + unmap(buffer); + } + buffersToClean.clear(); + + if (srcFile != null) { // see http://bugs.java.com/bugdatabase/view_bug.do?bug_id=4796385 srcFile.close(); } else { @@ -129,4 +151,30 @@ public class FileBackedDataSource extends DataSource { } return new RandomAccessFile(file, mode); } + + // need to use reflection to avoid depending on the sun.nio internal API + // unfortunately this might break silently with newer/other Java implementations, + // but we at least have unit-tests which will indicate this when run on Windows + private static void unmap(ByteBuffer bb) { + Class fcClass = bb.getClass(); + try { + // invoke bb.cleaner().clean(), but do not depend on sun.nio + // interfaces + Method cleanerMethod = fcClass.getDeclaredMethod("cleaner"); + cleanerMethod.setAccessible(true); + Object cleaner = cleanerMethod.invoke(bb); + Method cleanMethod = cleaner.getClass().getDeclaredMethod("clean"); + cleanMethod.invoke(cleaner); + } catch (NoSuchMethodException e) { + // e.printStackTrace(); + } catch (SecurityException e) { + // e.printStackTrace(); + } catch (IllegalAccessException e) { + // e.printStackTrace(); + } catch (IllegalArgumentException e) { + // e.printStackTrace(); + } catch (InvocationTargetException e) { + // e.printStackTrace(); + } + } } diff --git a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java index a2306d723..fec2a230a 100644 --- a/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java +++ b/src/testcases/org/apache/poi/hssf/usermodel/TestHSSFWorkbook.java @@ -56,6 +56,9 @@ import org.apache.poi.poifs.filesystem.POIFSFileSystem; import org.apache.poi.ss.formula.ptg.Area3DPtg; import org.apache.poi.ss.usermodel.BaseTestWorkbook; import org.apache.poi.ss.usermodel.Name; +import org.apache.poi.ss.usermodel.Row; +import org.apache.poi.ss.usermodel.Sheet; +import org.apache.poi.ss.usermodel.Workbook; import org.apache.poi.ss.util.CellRangeAddress; import org.apache.poi.util.LittleEndian; import org.apache.poi.util.TempFile; @@ -1194,4 +1197,50 @@ public final class TestHSSFWorkbook extends BaseTestWorkbook { assertTrue("Should find some images via Client or Child anchors, but did not find any at all", found); } + + @Test + public void testRewriteFileBug58480() throws Exception { + final File file = new File( + "build/HSSFWorkbookTest-testWriteScenario.xls"); + + // create new workbook + { + final Workbook workbook = new HSSFWorkbook(); + final Sheet sheet = workbook.createSheet("foo"); + final Row row = sheet.createRow(1); + row.createCell(1).setCellValue("bar"); + + writeAndCloseWorkbook(workbook, file); + } + + // edit the workbook + { + NPOIFSFileSystem fs = new NPOIFSFileSystem(file, false); + try { + DirectoryNode root = fs.getRoot(); + final Workbook workbook = new HSSFWorkbook(root, true); + final Sheet sheet = workbook.getSheet("foo"); + sheet.getRow(1).createCell(2).setCellValue("baz"); + + writeAndCloseWorkbook(workbook, file); + } finally { + fs.close(); + } + } + } + + private void writeAndCloseWorkbook(Workbook workbook, File file) + throws IOException, InterruptedException { + final ByteArrayOutputStream bytesOut = new ByteArrayOutputStream(); + workbook.write(bytesOut); + workbook.close(); + + final byte[] byteArray = bytesOut.toByteArray(); + bytesOut.close(); + + final FileOutputStream fileOut = new FileOutputStream(file); + fileOut.write(byteArray); + fileOut.close(); + + } } diff --git a/src/testcases/org/apache/poi/poifs/nio/TestDataSource.java b/src/testcases/org/apache/poi/poifs/nio/TestDataSource.java index 0155cf809..a7242662f 100644 --- a/src/testcases/org/apache/poi/poifs/nio/TestDataSource.java +++ b/src/testcases/org/apache/poi/poifs/nio/TestDataSource.java @@ -20,10 +20,17 @@ package org.apache.poi.poifs.nio; import java.io.File; +import java.io.FileNotFoundException; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; import java.nio.BufferUnderflowException; import java.nio.ByteBuffer; import org.apache.poi.POIDataSamples; +import org.apache.poi.util.IOUtils; +import org.apache.poi.util.TempFile; import junit.framework.TestCase; @@ -39,49 +46,147 @@ public class TestDataSource extends TestCase FileBackedDataSource ds = new FileBackedDataSource(f); try { - assertEquals(8192, ds.size()); - - // Start of file - ByteBuffer bs; - bs = ds.read(4, 0); - assertEquals(4, bs.capacity()); - assertEquals(0, bs.position()); - assertEquals(0xd0-256, bs.get(0)); - assertEquals(0xcf-256, bs.get(1)); - assertEquals(0x11-000, bs.get(2)); - assertEquals(0xe0-256, bs.get(3)); - assertEquals(0xd0-256, bs.get()); - assertEquals(0xcf-256, bs.get()); - assertEquals(0x11-000, bs.get()); - assertEquals(0xe0-256, bs.get()); - - // Mid way through - bs = ds.read(8, 0x400); - assertEquals(8, bs.capacity()); - assertEquals(0, bs.position()); - assertEquals((byte)'R', bs.get(0)); - assertEquals(0, bs.get(1)); - assertEquals((byte)'o', bs.get(2)); - assertEquals(0, bs.get(3)); - assertEquals((byte)'o', bs.get(4)); - assertEquals(0, bs.get(5)); - assertEquals((byte)'t', bs.get(6)); - assertEquals(0, bs.get(7)); - - // Can go to the end, but not past it - bs = ds.read(8, 8190); - assertEquals(0, bs.position()); // TODO How best to warn of a short read? - - // Can't go off the end - try { - bs = ds.read(4, 8192); - fail("Shouldn't be able to read off the end of the file"); - } catch(IllegalArgumentException e) {} + checkDataSource(ds, false); + } finally { + ds.close(); + } + + // try a second time + ds = new FileBackedDataSource(f); + try { + checkDataSource(ds, false); } finally { ds.close(); } } - + + public void testFileWritable() throws Exception { + File temp = TempFile.createTempFile("TestDataSource", ".test"); + try { + writeDataToFile(temp); + + FileBackedDataSource ds = new FileBackedDataSource(temp, false); + try { + checkDataSource(ds, true); + } finally { + ds.close(); + } + + // try a second time + ds = new FileBackedDataSource(temp, false); + try { + checkDataSource(ds, true); + } finally { + ds.close(); + } + + writeDataToFile(temp); + } finally { + assertTrue(temp.exists()); + assertTrue("Could not delete file " + temp, temp.delete()); + } + } + + + public void testRewritableFile() throws Exception { + File temp = TempFile.createTempFile("TestDataSource", ".test"); + try { + writeDataToFile(temp); + + FileBackedDataSource ds = new FileBackedDataSource(temp, true); + try { + ByteBuffer buf = ds.read(0, 10); + assertNotNull(buf); + buf = ds.read(8, 0x400); + assertNotNull(buf); + } finally { + ds.close(); + } + + // try a second time + ds = new FileBackedDataSource(temp, true); + try { + ByteBuffer buf = ds.read(0, 10); + assertNotNull(buf); + buf = ds.read(8, 0x400); + assertNotNull(buf); + } finally { + ds.close(); + } + + writeDataToFile(temp); + } finally { + assertTrue(temp.exists()); + assertTrue(temp.delete()); + } + } + + private void writeDataToFile(File temp) throws FileNotFoundException, IOException { + OutputStream str = new FileOutputStream(temp); + try { + InputStream in = data.openResourceAsStream("Notes.ole2"); + try { + IOUtils.copy(in, str); + } finally { + in.close(); + } + } finally { + str.close(); + } + } + + private void checkDataSource(FileBackedDataSource ds, boolean writeable) throws IOException { + assertEquals(writeable, ds.isWriteable()); + assertNotNull(ds.getChannel()); + + // rewriting changes the size + if(writeable) { + assertTrue("Had: " + ds.size(), ds.size() == 8192 || ds.size() == 8198); + } else { + assertEquals(8192, ds.size()); + } + + // Start of file + ByteBuffer bs; + bs = ds.read(4, 0); + assertEquals(4, bs.capacity()); + assertEquals(0, bs.position()); + assertEquals(0xd0 - 256, bs.get(0)); + assertEquals(0xcf - 256, bs.get(1)); + assertEquals(0x11 - 000, bs.get(2)); + assertEquals(0xe0 - 256, bs.get(3)); + assertEquals(0xd0 - 256, bs.get()); + assertEquals(0xcf - 256, bs.get()); + assertEquals(0x11 - 000, bs.get()); + assertEquals(0xe0 - 256, bs.get()); + + // Mid way through + bs = ds.read(8, 0x400); + assertEquals(8, bs.capacity()); + assertEquals(0, bs.position()); + assertEquals((byte) 'R', bs.get(0)); + assertEquals(0, bs.get(1)); + assertEquals((byte) 'o', bs.get(2)); + assertEquals(0, bs.get(3)); + assertEquals((byte) 'o', bs.get(4)); + assertEquals(0, bs.get(5)); + assertEquals((byte) 't', bs.get(6)); + assertEquals(0, bs.get(7)); + + // Can go to the end, but not past it + bs = ds.read(8, 8190); + assertEquals(0, bs.position()); // TODO How best to warn of a short read? + + // Can't go off the end + try { + bs = ds.read(4, 8192); + if(!writeable) { + fail("Shouldn't be able to read off the end of the file"); + } + } catch (IllegalArgumentException e) { + } + } + public void testByteArray() throws Exception { byte[] data = new byte[256]; byte b;