From 8d6f3407c179c4787b02a75e13cc1c44af2720c8 Mon Sep 17 00:00:00 2001 From: jpai Date: Sun, 27 Jun 2021 20:52:28 +0530 Subject: [PATCH 01/14] 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream --- .../classes/jdk/nio/zipfs/ZipFileSystem.java | 4 +- .../jdk/jdk/nio/zipfs/LargeEntrySizeTest.java | 136 ++++++++++++++++++ 2 files changed, 139 insertions(+), 1 deletion(-) create mode 100644 test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java index e9154c0f4702c..5dc432c6253e8 100644 --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -105,6 +105,8 @@ class ZipFileSystem extends FileSystem { private static final String COMPRESSION_METHOD_DEFLATED = "DEFLATED"; // Value specified for compressionMethod property to not compress Zip entries private static final String COMPRESSION_METHOD_STORED = "STORED"; + // The maximum size of array to allocate. Some VMs reserve some header words in an array. + private static final int MAX_ARRAY_SIZE = Integer.MAX_VALUE - 8; private final ZipFileSystemProvider provider; private final Path zfpath; @@ -1948,7 +1950,7 @@ private OutputStream getOutputStream(Entry e) throws IOException { e.file = getTempPathForEntry(null); os = Files.newOutputStream(e.file, WRITE); } else { - os = new ByteArrayOutputStream((e.size > 0)? (int)e.size : 8192); + os = new ByteArrayOutputStream((e.size > 0 && e.size <= MAX_ARRAY_SIZE)? (int)e.size : 8192); } if (e.method == METHOD_DEFLATED) { return new DeflatingEntryOutputStream(e, os); diff --git a/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java b/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java new file mode 100644 index 0000000000000..1c27e83583483 --- /dev/null +++ b/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java @@ -0,0 +1,136 @@ +/* + * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + * + */ + +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import java.io.File; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.OutputStream; +import java.io.RandomAccessFile; +import java.nio.file.FileSystem; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.util.List; +import java.util.zip.ZipEntry; +import java.util.zip.ZipOutputStream; + +/** + * @test + * @bug 8190753 + * @summary Verify that opening an outputstream for a large zip entry doesn't run into "Negative initial size" exception + * @run testng LargeEntrySizeTest + */ +public class LargeEntrySizeTest { + + // a value which when cast to an integer, becomes a negative value + private static final long LARGE_FILE_SIZE = Integer.MAX_VALUE + 1L; + private static final long SMALL_FILE_SIZE = 0x100000L; // 1024L x 1024L; + private static final String LARGE_FILE_NAME = "LargeZipEntry.txt"; + // File that will be created with a size less than 0xFFFFFFFF + private static final String SMALL_FILE_NAME = "SmallZipEntry.txt"; + // List of files to be added to the ZIP file + private static final List ZIP_ENTRIES = List.of(LARGE_FILE_NAME, SMALL_FILE_NAME); + private static final String ZIP_FILE_NAME = "8190753-test.zip"; + + @BeforeMethod + public void setUp() throws IOException { + deleteFiles(); + } + + @AfterMethod + public void tearDown() throws IOException { + deleteFiles(); + } + + /** + * Delete the files created for use by the test + * + * @throws IOException if an error occurs deleting the files + */ + private static void deleteFiles() throws IOException { + Files.deleteIfExists(Path.of(ZIP_FILE_NAME)); + Files.deleteIfExists(Path.of(LARGE_FILE_NAME)); + Files.deleteIfExists(Path.of(SMALL_FILE_NAME)); + } + + + /** + * Verifies that large entry (whose size is greater than {@link Integer#MAX_VALUE}) in a zip file + * can be opened as an {@link OutputStream} using the zip filesystem + */ + @Test + public void testLargeEntryZipFSOutputStream() throws Exception { + final Path zipFile = Path.of(ZIP_FILE_NAME); + createZipFile(zipFile); + try (FileSystem fs = FileSystems.newFileSystem(zipFile)) { + for (String entryName : ZIP_ENTRIES) { + try (OutputStream os = Files.newOutputStream(fs.getPath(entryName), StandardOpenOption.WRITE)) { + // just a dummy write + os.write(0x01); + } + } + } + } + + /** + * Creates a zip file with an entry whose size is larger than {@link Integer#MAX_VALUE} + */ + private static void createZipFile(final Path zipFile) throws IOException { + createFiles(); + try (OutputStream os = Files.newOutputStream(zipFile); + ZipOutputStream zos = new ZipOutputStream(os)) { + System.out.println("Creating Zip file: " + zipFile.getFileName()); + for (String srcFile : ZIP_ENTRIES) { + File fileToZip = new File(srcFile); + long fileSize = fileToZip.length(); + System.out.println("Adding entry " + srcFile + " of size " + fileSize + " bytes"); + try (FileInputStream fis = new FileInputStream(fileToZip)) { + ZipEntry zipEntry = new ZipEntry(fileToZip.getName()); + zipEntry.setSize(fileSize); + zos.putNextEntry(zipEntry); + fis.transferTo(zos); + } + } + } + } + + /** + * Create the files that will be added to the ZIP file + */ + private static void createFiles() throws IOException { + try (RandomAccessFile largeFile = new RandomAccessFile(LARGE_FILE_NAME, "rw"); + RandomAccessFile smallFile = new RandomAccessFile(SMALL_FILE_NAME, "rw")) { + System.out.printf("Creating %s%n", LARGE_FILE_NAME); + largeFile.setLength(LARGE_FILE_SIZE); + System.out.printf("Creating %s%n", SMALL_FILE_NAME); + smallFile.setLength(SMALL_FILE_SIZE); + } + } + +} From 127acbcc4637fa040e0fae2969edc21200a461c2 Mon Sep 17 00:00:00 2001 From: jpai Date: Mon, 28 Jun 2021 14:37:49 +0530 Subject: [PATCH 02/14] add @requires to the new test to selectively decide where the test runs --- test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java b/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java index 1c27e83583483..ca40227fd3fe8 100644 --- a/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java +++ b/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java @@ -44,7 +44,8 @@ * @test * @bug 8190753 * @summary Verify that opening an outputstream for a large zip entry doesn't run into "Negative initial size" exception - * @run testng LargeEntrySizeTest + * @requires (sun.arch.data.model == "64" & os.maxMemory >= 5g) + * @run testng/othervm -Xmx4g LargeEntrySizeTest */ public class LargeEntrySizeTest { From 19cc9d437293d462a7f5f06421a7b059a5b6b512 Mon Sep 17 00:00:00 2001 From: jpai Date: Wed, 30 Jun 2021 21:22:17 +0530 Subject: [PATCH 03/14] minor summary update on test --- test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java b/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java index ca40227fd3fe8..40b31b080f7e0 100644 --- a/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java +++ b/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java @@ -43,7 +43,8 @@ /** * @test * @bug 8190753 - * @summary Verify that opening an outputstream for a large zip entry doesn't run into "Negative initial size" exception + * @summary Verify that using zip filesystem for opening an outputstream for a large zip entry doesn't + * run into "Negative initial size" exception * @requires (sun.arch.data.model == "64" & os.maxMemory >= 5g) * @run testng/othervm -Xmx4g LargeEntrySizeTest */ From f519aa473cc5c78807f0141e433229db9137aa54 Mon Sep 17 00:00:00 2001 From: jpai Date: Wed, 30 Jun 2021 21:23:05 +0530 Subject: [PATCH 04/14] an initial proposed test for testing compressed size entry greater than 2gb --- .../zipfs/LargeCompressedEntrySizeTest.java | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java diff --git a/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java b/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java new file mode 100644 index 0000000000000..100356630ca7f --- /dev/null +++ b/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java @@ -0,0 +1,111 @@ +/* + * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + * + */ + +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import java.io.File; +import java.io.FileInputStream; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.io.RandomAccessFile; +import java.nio.ByteBuffer; +import java.nio.file.FileSystem; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.util.Collections; +import java.util.List; +import java.util.Random; +import java.util.zip.CRC32; +import java.util.zip.DeflaterOutputStream; +import java.util.zip.ZipEntry; +import java.util.zip.ZipInputStream; +import java.util.zip.ZipOutputStream; + +/** + * This is intentionally a manual test. The (jtreg) configurations below are here only + * for reference about runtime expectations of this test. + * + * @bug 8190753 + * @summary Verify that using zip filesystem for opening an outputstream for a zip entry whose + * compressed size is large, doesn't run into "Negative initial size" exception + * @requires (sun.arch.data.model == "64" & os.maxMemory >= 8g) + * @run testng/othervm -Xmx6g LargeCompressedEntrySizeTest + */ +public class LargeCompressedEntrySizeTest { + + private static final String LARGE_FILE_NAME = "LargeZipEntry.txt"; + private static final String ZIP_FILE_NAME = "8190753-test-compressed-size.zip"; + + @BeforeMethod + public void setUp() throws IOException { + deleteFiles(); + } + + @AfterMethod + public void tearDown() throws IOException { + deleteFiles(); + } + + /** + * Delete the files created for use by the test + * + * @throws IOException if an error occurs deleting the files + */ + private static void deleteFiles() throws IOException { + Files.deleteIfExists(Path.of(ZIP_FILE_NAME)); + Files.deleteIfExists(Path.of(LARGE_FILE_NAME)); + } + + + /** + * Using zip filesystem, creates a zip file and writes out a zip entry whose compressed size is + * expected to be greater than 2gb. + */ + @Test + public void testLargeCompressedSizeWithZipFS() throws Exception { + final Path zipFile = Path.of(ZIP_FILE_NAME); + final long largeFileSize = (2L * 1024L * 1024L * 1024L) + 1L; + final Random random = new Random(); + try (FileSystem fs = FileSystems.newFileSystem(zipFile, Collections.singletonMap("create", "true"))) { + try (OutputStream os = Files.newOutputStream(fs.getPath(LARGE_FILE_NAME))) { + long remaining = largeFileSize; + final int chunkSize = 102400; + for (long l = 0; l < largeFileSize; l+=chunkSize) { + final int numToWrite = (int) Math.min(remaining, chunkSize); + final byte[] b = new byte[numToWrite]; + // fill with random bytes + random.nextBytes(b); + os.write(b); + remaining -= b.length; + } + } + } + } + +} From 5c5f2694bd7e9a0f556b3e437e2f10aa3fa87c87 Mon Sep 17 00:00:00 2001 From: jpai Date: Fri, 2 Jul 2021 15:28:57 +0530 Subject: [PATCH 05/14] Introduce a way to allow outputstream returned by ZipOutputStream to rollover into a temporary file during writes, if the data size exceeds a threshold --- .../classes/jdk/nio/zipfs/ZipFileSystem.java | 150 +++++++++++++- .../zipfs/LargeCompressedEntrySizeTest.java | 37 ++-- .../jdk/jdk/nio/zipfs/LargeEntrySizeTest.java | 3 +- .../jdk/nio/zipfs/ZipFSOutputStreamTest.java | 186 ++++++++++++++++++ 4 files changed, 344 insertions(+), 32 deletions(-) create mode 100644 test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java index 5dc432c6253e8..b98688580abd3 100644 --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -33,6 +33,7 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.io.UncheckedIOException; import java.lang.Runtime.Version; import java.nio.ByteBuffer; import java.nio.MappedByteBuffer; @@ -121,6 +122,13 @@ class ZipFileSystem extends FileSystem { private final boolean noExtt; // see readExtra() private final boolean useTempFile; // use a temp file for newOS, default // is to use BAOS for better performance + + private static final int DEFAULT_TEMP_FILE_CREATION_THRESHOLD = 10 * 1024 * 1024; // 10 MB + // a threshold, in bytes, to decide whether to create a temp file + // for outputstream of a zip entry, when useTempFile + // isn't explicitly enabled + private final int tempFileCreationThreshold; + private final boolean forceEnd64; private final int defaultCompressionMethod; // METHOD_STORED if "noCompression=true" // METHOD_DEFLATED otherwise @@ -145,6 +153,7 @@ class ZipFileSystem extends FileSystem { (String)env.get("encoding") : "UTF-8"; this.noExtt = "false".equals(env.get("zipinfo-time")); this.useTempFile = isTrue(env, "useTempFile"); + this.tempFileCreationThreshold = initTempFileThreshold(env); this.forceEnd64 = isTrue(env, "forceZIP64End"); this.defaultCompressionMethod = getDefaultCompressionMethod(env); this.supportPosix = isTrue(env, PROPERTY_POSIX); @@ -237,6 +246,24 @@ private static boolean isTrue(Map env, String name) { return "true".equals(env.get(name)) || TRUE.equals(env.get(name)); } + private static int initTempFileThreshold(Map env) { + final Object val = env.get("tempFileThreshold"); + if (val == null) { + return DEFAULT_TEMP_FILE_CREATION_THRESHOLD; + } + if (val instanceof String v) { + try { + return Math.min(Integer.parseInt(v), MAX_ARRAY_SIZE); + } catch (NumberFormatException nfe) { + return DEFAULT_TEMP_FILE_CREATION_THRESHOLD; + } + } + if (val instanceof Integer v) { + return Math.min(v, MAX_ARRAY_SIZE); + } + return DEFAULT_TEMP_FILE_CREATION_THRESHOLD; + } + // Initialize the default owner for files inside the zip archive. // If not specified in env, it is the owner of the archive. If no owner can // be determined, we try to go with system property "user.name". If that's not @@ -1946,11 +1973,11 @@ private OutputStream getOutputStream(Entry e) throws IOException { if (zc.isUTF8()) e.flag |= FLAG_USE_UTF8; OutputStream os; - if (useTempFile) { + if (useTempFile || (tempFileCreationThreshold > 0 && e.size >= tempFileCreationThreshold)) { e.file = getTempPathForEntry(null); os = Files.newOutputStream(e.file, WRITE); } else { - os = new ByteArrayOutputStream((e.size > 0 && e.size <= MAX_ARRAY_SIZE)? (int)e.size : 8192); + os = new FileRolloverOutputStream(e); } if (e.method == METHOD_DEFLATED) { return new DeflatingEntryOutputStream(e, os); @@ -1990,8 +2017,9 @@ public synchronized void close() throws IOException { } isClosed = true; e.size = written; - if (out instanceof ByteArrayOutputStream) - e.bytes = ((ByteArrayOutputStream)out).toByteArray(); + if (out instanceof FileRolloverOutputStream fros && fros.tmpFileOS == null) { + e.bytes = fros.toByteArray(); + } super.close(); update(e); } @@ -2026,8 +2054,9 @@ public synchronized void close() throws IOException { e.size = def.getBytesRead(); e.csize = def.getBytesWritten(); e.crc = crc.getValue(); - if (out instanceof ByteArrayOutputStream) - e.bytes = ((ByteArrayOutputStream)out).toByteArray(); + if (out instanceof FileRolloverOutputStream fros && fros.tmpFileOS == null) { + e.bytes = fros.toByteArray(); + } super.close(); update(e); releaseDeflater(def); @@ -2109,6 +2138,115 @@ public void close() throws IOException { } } + // A wrapper around the ByteArrayOutputStream. This FileRolloverOutputStream + // uses a threshold size to decide if the contents being written need to be + // rolled over into a temporary file. Until the threshold is reached, writes + // on this outputstream just write it to the internal in-memory byte array + // held by the ByteArrayOutputStream. Once the threshold is reached, the + // write operation on this outputstream first (and only once) creates a temporary file + // and transfers the data that has so far been written in the internal + // byte array, to that newly created file. The temp file is then opened + // in append mode and any subsequent writes, including the one which triggered + // the temporary file creation, will be written to the file. + // A threshold value of <= 0 implies rollover is disabled, in which case this + // FileRolloverOutputStream behaves just like a ByteArrayOutputStream and just passes + // on the write operations to the ByteArrayOutputStream implementation. + private class FileRolloverOutputStream extends ByteArrayOutputStream { + private final Entry entry; + private long totalWritten = 0; + private OutputStream tmpFileOS; + + FileRolloverOutputStream(final Entry e) { + super(tempFileCreationThreshold <= 0 + ? ((e.size > 0 && e.size <= MAX_ARRAY_SIZE) ? (int) e.size : 8192) + : ((e.size > 0 && e.size <= tempFileCreationThreshold) ? (int) e.size : 8192)); + this.entry = e; + } + + @Override + public synchronized void write(int b) { + if (tmpFileOS != null) { + // already rolled over, write to the file that has been created previously + writeToFile(b); + return; + } + if (tempFileCreationThreshold <= 0 || (totalWritten + 1 < tempFileCreationThreshold)) { + // write to our in-memory byte array + super.write(b); + totalWritten++; + return; + } + // rollover into a file + try { + transferToFile(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + writeToFile(b); + } + + @Override + public synchronized void write(byte[] b, int off, int len) { + if (tmpFileOS != null) { + // already rolled over, write to the file that has been created previously + writeToFile(b, off, len); + return; + } + if (tempFileCreationThreshold <= 0 || (totalWritten + len < tempFileCreationThreshold)) { + // write to our in-memory byte array + super.write(b, off, len); + totalWritten += len; + return; + } + // rollover into a file + try { + transferToFile(); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + writeToFile(b, off, len); + } + + @Override + public void close() throws IOException { + if (tmpFileOS != null) { + tmpFileOS.close(); + } + } + + private void writeToFile(int b) { + try { + tmpFileOS.write(b); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + totalWritten++; + } + + private void writeToFile(byte[] b, int off, int len) { + try { + tmpFileOS.write(b, off, len); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + totalWritten += len; + } + + private void transferToFile() throws IOException { + // create a tempfile + entry.file = getTempPathForEntry(null); + // transfer the already written data from the byte array buffer into this tempfile + try (OutputStream os = new BufferedOutputStream(Files.newOutputStream(entry.file))) { + new ByteArrayInputStream(buf, 0, count).transferTo(os); + } + // clear the in-memory buffer and shrink the buffer + reset(); + buf = new byte[0]; + // append any further data to the file with buffering enabled + tmpFileOS = new BufferedOutputStream(Files.newOutputStream(entry.file, APPEND)); + } + } + private InputStream getInputStream(Entry e) throws IOException { diff --git a/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java b/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java index 100356630ca7f..3ceffc26bde3b 100644 --- a/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java +++ b/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java @@ -26,26 +26,15 @@ import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; -import java.io.File; -import java.io.FileInputStream; import java.io.IOException; -import java.io.InputStream; import java.io.OutputStream; -import java.io.RandomAccessFile; -import java.nio.ByteBuffer; import java.nio.file.FileSystem; import java.nio.file.FileSystems; import java.nio.file.Files; import java.nio.file.Path; -import java.nio.file.StandardOpenOption; import java.util.Collections; -import java.util.List; import java.util.Random; -import java.util.zip.CRC32; -import java.util.zip.DeflaterOutputStream; -import java.util.zip.ZipEntry; -import java.util.zip.ZipInputStream; -import java.util.zip.ZipOutputStream; +import java.util.concurrent.TimeUnit; /** * This is intentionally a manual test. The (jtreg) configurations below are here only @@ -54,8 +43,7 @@ * @bug 8190753 * @summary Verify that using zip filesystem for opening an outputstream for a zip entry whose * compressed size is large, doesn't run into "Negative initial size" exception - * @requires (sun.arch.data.model == "64" & os.maxMemory >= 8g) - * @run testng/othervm -Xmx6g LargeCompressedEntrySizeTest + * @run testng/othervm/timeout=300 LargeCompressedEntrySizeTest */ public class LargeCompressedEntrySizeTest { @@ -79,7 +67,6 @@ public void tearDown() throws IOException { */ private static void deleteFiles() throws IOException { Files.deleteIfExists(Path.of(ZIP_FILE_NAME)); - Files.deleteIfExists(Path.of(LARGE_FILE_NAME)); } @@ -90,20 +77,22 @@ private static void deleteFiles() throws IOException { @Test public void testLargeCompressedSizeWithZipFS() throws Exception { final Path zipFile = Path.of(ZIP_FILE_NAME); - final long largeFileSize = (2L * 1024L * 1024L * 1024L) + 1L; - final Random random = new Random(); + final long largeEntrySize = 6L * 1024L * 1024L * 1024L; // large value which exceeds Integer.MAX_VALUE try (FileSystem fs = FileSystems.newFileSystem(zipFile, Collections.singletonMap("create", "true"))) { try (OutputStream os = Files.newOutputStream(fs.getPath(LARGE_FILE_NAME))) { - long remaining = largeFileSize; + long remaining = largeEntrySize; + // create a chunk of random bytes which we keep writing out final int chunkSize = 102400; - for (long l = 0; l < largeFileSize; l+=chunkSize) { + final byte[] chunk = new byte[chunkSize]; + new Random().nextBytes(chunk); + final long start = System.currentTimeMillis(); + for (long l = 0; l < largeEntrySize; l += chunkSize) { final int numToWrite = (int) Math.min(remaining, chunkSize); - final byte[] b = new byte[numToWrite]; - // fill with random bytes - random.nextBytes(b); - os.write(b); - remaining -= b.length; + os.write(chunk, 0, numToWrite); + remaining -= numToWrite; } + System.out.println("Took " + TimeUnit.SECONDS.toSeconds(System.currentTimeMillis() - start) + + " seconds to generate entry of size " + largeEntrySize); } } } diff --git a/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java b/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java index 40b31b080f7e0..33530f94fff14 100644 --- a/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java +++ b/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java @@ -45,8 +45,7 @@ * @bug 8190753 * @summary Verify that using zip filesystem for opening an outputstream for a large zip entry doesn't * run into "Negative initial size" exception - * @requires (sun.arch.data.model == "64" & os.maxMemory >= 5g) - * @run testng/othervm -Xmx4g LargeEntrySizeTest + * @run testng LargeEntrySizeTest */ public class LargeEntrySizeTest { diff --git a/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java b/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java new file mode 100644 index 0000000000000..994b72c2387f2 --- /dev/null +++ b/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java @@ -0,0 +1,186 @@ +/* + * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + * + */ + +import org.testng.Assert; +import org.testng.annotations.AfterMethod; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.FileSystem; +import java.nio.file.FileSystems; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; +import java.util.Random; + + +/** + * @test + * @summary Verify that the outputstream created for zip file entries, through the ZipFileSystem + * works fine for varying sizes of the zip file entries and various different values of tempFileThreshold + * @bug 8190753 + * @run testng ZipFSOutputStreamTest + */ +public class ZipFSOutputStreamTest { + // List of files to be added to the ZIP file along with their sizes in bytes + private static final Map ZIP_ENTRIES = Map.of( + "LargeSize", 25L * 1024L * 1024L, // 25 MB + "d1/SmallSize", 1234L, + "d1/d2/ZeroSize", 0L); + + private static final Path ZIP_FILE = Path.of("zipfs-outputstream-test.zip"); + + @BeforeMethod + public void setUp() throws IOException { + deleteFiles(); + } + + @AfterMethod + public void tearDown() throws IOException { + deleteFiles(); + } + + private static void deleteFiles() throws IOException { + Files.deleteIfExists(ZIP_FILE); + } + + @DataProvider(name = "zipfsThresholdValueParsing") + private Object[][] zipfsThresholdValueParsing() { + // various different values passed to the "tempFileThreshold", some valid some invalid + return new Object[][]{ + {Map.of("create", "true", "tempFileThreshold", "1024")}, + {Map.of("create", "true", "tempFileThreshold", "1")}, + {Map.of("create", "true", "tempFileThreshold", "-1")}, + {Map.of("create", "true", "tempFileThreshold", "-2")}, + {Map.of("create", "true", "tempFileThreshold", "0")}, + {Map.of("create", "true", "tempFileThreshold", Integer.MAX_VALUE)}, + {Map.of("create", "true", "tempFileThreshold", Long.MAX_VALUE)}, + {Map.of("create", "true", "tempFileThreshold", "helloworld")}, + {Map.of("create", "true", "tempFileThreshold", 124232.232d)}, + {Map.of("create", "true", "tempFileThreshold", 122.232f)} + }; + } + + /** + * Create a zip filesystem with various different values of tempFileThreshold, some valid and + * some invalid. Make sure that in all these cases, the zip filesystem creation works fine + * and entries can be added to the zip file and read from and the content is as expected. + */ + @Test(dataProvider = "zipfsThresholdValueParsing") + public void testThresholdValueParsing(final Map env) throws Exception { + try (final FileSystem zipfs = FileSystems.newFileSystem(ZIP_FILE, env)) { + // create the zip file with just a simple entry + try (final OutputStream os = Files.newOutputStream(zipfs.getPath("helloworld.txt"))) { + os.write("hi".getBytes(StandardCharsets.UTF_8)); + } + // now verify the written content + try (final InputStream is = Files.newInputStream(zipfs.getPath("helloworld.txt"))) { + Assert.assertEquals(is.readAllBytes(), "hi".getBytes(StandardCharsets.UTF_8), + "Unexpected content in zip entry"); + } + } + } + + @DataProvider(name = "zipfsVaryingThreshold") + private Object[][] zipfsVaryingThreshold() { + return new Object[][]{ + // default tempfile threshold, for both DEFLATED and STORED compression modes + {Map.of("create", "true", "noCompression", "true")}, + {Map.of("create", "true", "noCompression", "false")}, + // specific threshold for both DEFLATED and STORED compression modes + {Map.of("create", "true", "noCompression", "true", "tempFileThreshold", "1024")}, + {Map.of("create", "true", "noCompression", "false", "tempFileThreshold", "1024")}, + // threshold disabled + {Map.of("create", "true", "noCompression", "true", "tempFileThreshold", "-1")}, + {Map.of("create", "true", "noCompression", "false", "tempFileThreshold", "-1")}, + {Map.of("create", "true", "noCompression", "true", "tempFileThreshold", "0")}, + {Map.of("create", "true", "noCompression", "false", "tempFileThreshold", "0")}, + + }; + } + + /** + * Create a zip filesystem using different outputstream temp file creation threshold and write + * out entries of varying sizes. Then verify that the generated zip file entries are as expected, + * both in size and content + */ + @Test(dataProvider = "zipfsVaryingThreshold") + public void testOutputStreamWithDifferentThreshold(final Map env) throws Exception { + final byte[] chunk = new byte[1024]; + new Random().nextBytes(chunk); + try (final FileSystem zipfs = FileSystems.newFileSystem(ZIP_FILE, env)) { + // create the zip with varying sized entries + for (final Map.Entry entry : ZIP_ENTRIES.entrySet()) { + final Path entryPath = zipfs.getPath(entry.getKey()); + if (entryPath.getParent() != null) { + Files.createDirectories(entryPath.getParent()); + } + try (final OutputStream os = Files.newOutputStream(entryPath)) { + writeAsChunks(os, chunk, entry.getValue()); + } + } + } + // now verify the written content + try (final FileSystem zipfs = FileSystems.newFileSystem(ZIP_FILE)) { + for (final Map.Entry entry : ZIP_ENTRIES.entrySet()) { + final Path entryPath = zipfs.getPath(entry.getKey()); + try (final InputStream is = Files.newInputStream(entryPath)) { + final byte[] buf = new byte[chunk.length]; + int numRead; + long totalRead = 0; + while ((numRead = is.read(buf)) != -1) { + totalRead += numRead; + // verify the content + for (int i = 0, chunkoffset = (int) ((totalRead - numRead) % chunk.length); + i < numRead; i++, chunkoffset++) { + Assert.assertEquals(buf[i], chunk[chunkoffset % chunk.length], + "Unexpected content in " + entryPath); + } + } + Assert.assertEquals(totalRead, (long) entry.getValue(), + "Unexpected number of bytes read from zip entry " + entryPath); + } + } + } + } + + /** + * Repeatedly writes out to the outputstream, the chunk of data, till the number of bytes + * written to the stream equals the totalSize + */ + private static void writeAsChunks(final OutputStream os, final byte[] chunk, + final long totalSize) throws IOException { + long remaining = totalSize; + for (long l = 0; l < totalSize; l += chunk.length) { + final int numToWrite = (int) Math.min(remaining, chunk.length); + os.write(chunk, 0, numToWrite); + remaining -= numToWrite; + } + } +} From a9dfd8cddf6ce9af35af245ad7fb0d4d5fe17c2d Mon Sep 17 00:00:00 2001 From: jpai Date: Sun, 4 Jul 2021 12:43:07 +0530 Subject: [PATCH 06/14] propagate back the original checked IOException to the callers --- .../classes/jdk/nio/zipfs/ZipFileSystem.java | 26 ++++++++++++++----- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java index b98688580abd3..42b79ecb24660 100644 --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -1999,14 +1999,22 @@ private class EntryOutputStream extends FilterOutputStream { @Override public synchronized void write(int b) throws IOException { - out.write(b); + try { + out.write(b); + } catch (UncheckedIOException uioe) { + throw uioe.getCause(); + } written += 1; } @Override public synchronized void write(byte[] b, int off, int len) throws IOException { - out.write(b, off, len); + try { + out.write(b, off, len); + } catch (UncheckedIOException uioe) { + throw uioe.getCause(); + } written += len; } @@ -2041,7 +2049,11 @@ private class DeflatingEntryOutputStream extends DeflaterOutputStream { @Override public synchronized void write(byte[] b, int off, int len) throws IOException { - super.write(b, off, len); + try { + super.write(b, off, len); + } catch (UncheckedIOException uioe) { + throw uioe.getCause(); + } crc.update(b, off, len); } @@ -2164,7 +2176,7 @@ private class FileRolloverOutputStream extends ByteArrayOutputStream { } @Override - public synchronized void write(int b) { + public synchronized void write(int b) throws UncheckedIOException { if (tmpFileOS != null) { // already rolled over, write to the file that has been created previously writeToFile(b); @@ -2186,7 +2198,7 @@ public synchronized void write(int b) { } @Override - public synchronized void write(byte[] b, int off, int len) { + public synchronized void write(byte[] b, int off, int len) throws UncheckedIOException { if (tmpFileOS != null) { // already rolled over, write to the file that has been created previously writeToFile(b, off, len); @@ -2214,7 +2226,7 @@ public void close() throws IOException { } } - private void writeToFile(int b) { + private void writeToFile(int b) throws UncheckedIOException { try { tmpFileOS.write(b); } catch (IOException e) { @@ -2223,7 +2235,7 @@ private void writeToFile(int b) { totalWritten++; } - private void writeToFile(byte[] b, int off, int len) { + private void writeToFile(byte[] b, int off, int len) throws UncheckedIOException { try { tmpFileOS.write(b, off, len); } catch (IOException e) { From c27a0a445536ff327a2830ab1e2a919c15e74ff9 Mon Sep 17 00:00:00 2001 From: jpai Date: Mon, 5 Jul 2021 09:39:05 +0530 Subject: [PATCH 07/14] Implement Alan's review suggestion - don't expose the threshold as a configuration property, instead set it internally to a specific value --- .../classes/jdk/nio/zipfs/ZipFileSystem.java | 35 ++++--------------- 1 file changed, 6 insertions(+), 29 deletions(-) diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java index 42b79ecb24660..1f4125d4bf665 100644 --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -123,11 +123,9 @@ class ZipFileSystem extends FileSystem { private final boolean useTempFile; // use a temp file for newOS, default // is to use BAOS for better performance - private static final int DEFAULT_TEMP_FILE_CREATION_THRESHOLD = 10 * 1024 * 1024; // 10 MB // a threshold, in bytes, to decide whether to create a temp file - // for outputstream of a zip entry, when useTempFile - // isn't explicitly enabled - private final int tempFileCreationThreshold; + // for outputstream of a zip entry + private final int tempFileCreationThreshold = 10 * 1024 * 1024; // 10 MB private final boolean forceEnd64; private final int defaultCompressionMethod; // METHOD_STORED if "noCompression=true" @@ -153,7 +151,6 @@ class ZipFileSystem extends FileSystem { (String)env.get("encoding") : "UTF-8"; this.noExtt = "false".equals(env.get("zipinfo-time")); this.useTempFile = isTrue(env, "useTempFile"); - this.tempFileCreationThreshold = initTempFileThreshold(env); this.forceEnd64 = isTrue(env, "forceZIP64End"); this.defaultCompressionMethod = getDefaultCompressionMethod(env); this.supportPosix = isTrue(env, PROPERTY_POSIX); @@ -246,24 +243,6 @@ private static boolean isTrue(Map env, String name) { return "true".equals(env.get(name)) || TRUE.equals(env.get(name)); } - private static int initTempFileThreshold(Map env) { - final Object val = env.get("tempFileThreshold"); - if (val == null) { - return DEFAULT_TEMP_FILE_CREATION_THRESHOLD; - } - if (val instanceof String v) { - try { - return Math.min(Integer.parseInt(v), MAX_ARRAY_SIZE); - } catch (NumberFormatException nfe) { - return DEFAULT_TEMP_FILE_CREATION_THRESHOLD; - } - } - if (val instanceof Integer v) { - return Math.min(v, MAX_ARRAY_SIZE); - } - return DEFAULT_TEMP_FILE_CREATION_THRESHOLD; - } - // Initialize the default owner for files inside the zip archive. // If not specified in env, it is the owner of the archive. If no owner can // be determined, we try to go with system property "user.name". If that's not @@ -1973,7 +1952,7 @@ private OutputStream getOutputStream(Entry e) throws IOException { if (zc.isUTF8()) e.flag |= FLAG_USE_UTF8; OutputStream os; - if (useTempFile || (tempFileCreationThreshold > 0 && e.size >= tempFileCreationThreshold)) { + if (useTempFile || e.size >= tempFileCreationThreshold) { e.file = getTempPathForEntry(null); os = Files.newOutputStream(e.file, WRITE); } else { @@ -2169,9 +2148,7 @@ private class FileRolloverOutputStream extends ByteArrayOutputStream { private OutputStream tmpFileOS; FileRolloverOutputStream(final Entry e) { - super(tempFileCreationThreshold <= 0 - ? ((e.size > 0 && e.size <= MAX_ARRAY_SIZE) ? (int) e.size : 8192) - : ((e.size > 0 && e.size <= tempFileCreationThreshold) ? (int) e.size : 8192)); + super(8192); this.entry = e; } @@ -2182,7 +2159,7 @@ public synchronized void write(int b) throws UncheckedIOException { writeToFile(b); return; } - if (tempFileCreationThreshold <= 0 || (totalWritten + 1 < tempFileCreationThreshold)) { + if (totalWritten + 1 < tempFileCreationThreshold) { // write to our in-memory byte array super.write(b); totalWritten++; @@ -2204,7 +2181,7 @@ public synchronized void write(byte[] b, int off, int len) throws UncheckedIOExc writeToFile(b, off, len); return; } - if (tempFileCreationThreshold <= 0 || (totalWritten + len < tempFileCreationThreshold)) { + if (totalWritten + len < tempFileCreationThreshold) { // write to our in-memory byte array super.write(b, off, len); totalWritten += len; From 9de70ef10307bb92a153acd3889ea9640855b9bc Mon Sep 17 00:00:00 2001 From: jpai Date: Mon, 5 Jul 2021 09:50:51 +0530 Subject: [PATCH 08/14] use jtreg's construct of manual test --- test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java b/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java index 3ceffc26bde3b..0bb747dcf5c1c 100644 --- a/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java +++ b/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java @@ -40,10 +40,11 @@ * This is intentionally a manual test. The (jtreg) configurations below are here only * for reference about runtime expectations of this test. * + * @test * @bug 8190753 * @summary Verify that using zip filesystem for opening an outputstream for a zip entry whose * compressed size is large, doesn't run into "Negative initial size" exception - * @run testng/othervm/timeout=300 LargeCompressedEntrySizeTest + * @run testng/manual/othervm LargeCompressedEntrySizeTest */ public class LargeCompressedEntrySizeTest { From 50e64d7402238f1f06fee3c2ff4a284c0d5ff518 Mon Sep 17 00:00:00 2001 From: jpai Date: Mon, 5 Jul 2021 09:54:37 +0530 Subject: [PATCH 09/14] remove no longer used constant --- src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java index 1f4125d4bf665..3d4070002fde4 100644 --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -106,8 +106,6 @@ class ZipFileSystem extends FileSystem { private static final String COMPRESSION_METHOD_DEFLATED = "DEFLATED"; // Value specified for compressionMethod property to not compress Zip entries private static final String COMPRESSION_METHOD_STORED = "STORED"; - // The maximum size of array to allocate. Some VMs reserve some header words in an array. - private static final int MAX_ARRAY_SIZE = Integer.MAX_VALUE - 8; private final ZipFileSystemProvider provider; private final Path zfpath; From 0fdbcea63e4e1384f6c0b7e1899fd0e885bdf158 Mon Sep 17 00:00:00 2001 From: jpai Date: Mon, 5 Jul 2021 10:03:12 +0530 Subject: [PATCH 10/14] minor update to comment on FileRolloverOutputStream class --- src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java index 3d4070002fde4..bfe5c50f4b700 100644 --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -2137,9 +2137,6 @@ public void close() throws IOException { // byte array, to that newly created file. The temp file is then opened // in append mode and any subsequent writes, including the one which triggered // the temporary file creation, will be written to the file. - // A threshold value of <= 0 implies rollover is disabled, in which case this - // FileRolloverOutputStream behaves just like a ByteArrayOutputStream and just passes - // on the write operations to the ByteArrayOutputStream implementation. private class FileRolloverOutputStream extends ByteArrayOutputStream { private final Entry entry; private long totalWritten = 0; From c1ec9e129295bfc7d8e64d7e75ff4ab628874b72 Mon Sep 17 00:00:00 2001 From: jpai Date: Mon, 5 Jul 2021 13:06:09 +0530 Subject: [PATCH 11/14] reorganize the tests now that the temp file creation threshold isn't exposed as a user configurable value --- .../zipfs/LargeCompressedEntrySizeTest.java | 2 +- .../jdk/jdk/nio/zipfs/LargeEntrySizeTest.java | 137 ------------------ .../jdk/nio/zipfs/ZipFSOutputStreamTest.java | 76 ++-------- 3 files changed, 16 insertions(+), 199 deletions(-) delete mode 100644 test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java diff --git a/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java b/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java index 0bb747dcf5c1c..7132195e8fd7d 100644 --- a/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java +++ b/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java @@ -41,7 +41,7 @@ * for reference about runtime expectations of this test. * * @test - * @bug 8190753 + * @bug 8190753 8011146 * @summary Verify that using zip filesystem for opening an outputstream for a zip entry whose * compressed size is large, doesn't run into "Negative initial size" exception * @run testng/manual/othervm LargeCompressedEntrySizeTest diff --git a/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java b/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java deleted file mode 100644 index 33530f94fff14..0000000000000 --- a/test/jdk/jdk/nio/zipfs/LargeEntrySizeTest.java +++ /dev/null @@ -1,137 +0,0 @@ -/* - * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. - * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. - * - * This code is free software; you can redistribute it and/or modify it - * under the terms of the GNU General Public License version 2 only, as - * published by the Free Software Foundation. - * - * This code is distributed in the hope that it will be useful, but WITHOUT - * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or - * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License - * version 2 for more details (a copy is included in the LICENSE file that - * accompanied this code). - * - * You should have received a copy of the GNU General Public License version - * 2 along with this work; if not, write to the Free Software Foundation, - * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. - * - * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA - * or visit www.oracle.com if you need additional information or have any - * questions. - * - */ - -import org.testng.annotations.AfterMethod; -import org.testng.annotations.BeforeMethod; -import org.testng.annotations.Test; - -import java.io.File; -import java.io.FileInputStream; -import java.io.IOException; -import java.io.OutputStream; -import java.io.RandomAccessFile; -import java.nio.file.FileSystem; -import java.nio.file.FileSystems; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.StandardOpenOption; -import java.util.List; -import java.util.zip.ZipEntry; -import java.util.zip.ZipOutputStream; - -/** - * @test - * @bug 8190753 - * @summary Verify that using zip filesystem for opening an outputstream for a large zip entry doesn't - * run into "Negative initial size" exception - * @run testng LargeEntrySizeTest - */ -public class LargeEntrySizeTest { - - // a value which when cast to an integer, becomes a negative value - private static final long LARGE_FILE_SIZE = Integer.MAX_VALUE + 1L; - private static final long SMALL_FILE_SIZE = 0x100000L; // 1024L x 1024L; - private static final String LARGE_FILE_NAME = "LargeZipEntry.txt"; - // File that will be created with a size less than 0xFFFFFFFF - private static final String SMALL_FILE_NAME = "SmallZipEntry.txt"; - // List of files to be added to the ZIP file - private static final List ZIP_ENTRIES = List.of(LARGE_FILE_NAME, SMALL_FILE_NAME); - private static final String ZIP_FILE_NAME = "8190753-test.zip"; - - @BeforeMethod - public void setUp() throws IOException { - deleteFiles(); - } - - @AfterMethod - public void tearDown() throws IOException { - deleteFiles(); - } - - /** - * Delete the files created for use by the test - * - * @throws IOException if an error occurs deleting the files - */ - private static void deleteFiles() throws IOException { - Files.deleteIfExists(Path.of(ZIP_FILE_NAME)); - Files.deleteIfExists(Path.of(LARGE_FILE_NAME)); - Files.deleteIfExists(Path.of(SMALL_FILE_NAME)); - } - - - /** - * Verifies that large entry (whose size is greater than {@link Integer#MAX_VALUE}) in a zip file - * can be opened as an {@link OutputStream} using the zip filesystem - */ - @Test - public void testLargeEntryZipFSOutputStream() throws Exception { - final Path zipFile = Path.of(ZIP_FILE_NAME); - createZipFile(zipFile); - try (FileSystem fs = FileSystems.newFileSystem(zipFile)) { - for (String entryName : ZIP_ENTRIES) { - try (OutputStream os = Files.newOutputStream(fs.getPath(entryName), StandardOpenOption.WRITE)) { - // just a dummy write - os.write(0x01); - } - } - } - } - - /** - * Creates a zip file with an entry whose size is larger than {@link Integer#MAX_VALUE} - */ - private static void createZipFile(final Path zipFile) throws IOException { - createFiles(); - try (OutputStream os = Files.newOutputStream(zipFile); - ZipOutputStream zos = new ZipOutputStream(os)) { - System.out.println("Creating Zip file: " + zipFile.getFileName()); - for (String srcFile : ZIP_ENTRIES) { - File fileToZip = new File(srcFile); - long fileSize = fileToZip.length(); - System.out.println("Adding entry " + srcFile + " of size " + fileSize + " bytes"); - try (FileInputStream fis = new FileInputStream(fileToZip)) { - ZipEntry zipEntry = new ZipEntry(fileToZip.getName()); - zipEntry.setSize(fileSize); - zos.putNextEntry(zipEntry); - fis.transferTo(zos); - } - } - } - } - - /** - * Create the files that will be added to the ZIP file - */ - private static void createFiles() throws IOException { - try (RandomAccessFile largeFile = new RandomAccessFile(LARGE_FILE_NAME, "rw"); - RandomAccessFile smallFile = new RandomAccessFile(SMALL_FILE_NAME, "rw")) { - System.out.printf("Creating %s%n", LARGE_FILE_NAME); - largeFile.setLength(LARGE_FILE_SIZE); - System.out.printf("Creating %s%n", SMALL_FILE_NAME); - smallFile.setLength(SMALL_FILE_SIZE); - } - } - -} diff --git a/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java b/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java index 994b72c2387f2..fe59857879b3d 100644 --- a/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java +++ b/test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java @@ -31,7 +31,6 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; -import java.nio.charset.StandardCharsets; import java.nio.file.FileSystem; import java.nio.file.FileSystems; import java.nio.file.Files; @@ -43,16 +42,17 @@ /** * @test * @summary Verify that the outputstream created for zip file entries, through the ZipFileSystem - * works fine for varying sizes of the zip file entries and various different values of tempFileThreshold - * @bug 8190753 - * @run testng ZipFSOutputStreamTest + * works fine for varying sizes of the zip file entries + * @bug 8190753 8011146 + * @run testng/timeout=300 ZipFSOutputStreamTest */ public class ZipFSOutputStreamTest { // List of files to be added to the ZIP file along with their sizes in bytes private static final Map ZIP_ENTRIES = Map.of( - "LargeSize", 25L * 1024L * 1024L, // 25 MB - "d1/SmallSize", 1234L, - "d1/d2/ZeroSize", 0L); + "f1", Integer.MAX_VALUE + 1L, // a value which when cast to an integer, becomes a negative value + "f2", 25L * 1024L * 1024L, // 25 MB + "d1/f3", 1234L, + "d1/d2/f4", 0L); private static final Path ZIP_FILE = Path.of("zipfs-outputstream-test.zip"); @@ -70,68 +70,22 @@ private static void deleteFiles() throws IOException { Files.deleteIfExists(ZIP_FILE); } - @DataProvider(name = "zipfsThresholdValueParsing") - private Object[][] zipfsThresholdValueParsing() { - // various different values passed to the "tempFileThreshold", some valid some invalid + @DataProvider(name = "zipFSCreationEnv") + private Object[][] zipFSCreationEnv() { return new Object[][]{ - {Map.of("create", "true", "tempFileThreshold", "1024")}, - {Map.of("create", "true", "tempFileThreshold", "1")}, - {Map.of("create", "true", "tempFileThreshold", "-1")}, - {Map.of("create", "true", "tempFileThreshold", "-2")}, - {Map.of("create", "true", "tempFileThreshold", "0")}, - {Map.of("create", "true", "tempFileThreshold", Integer.MAX_VALUE)}, - {Map.of("create", "true", "tempFileThreshold", Long.MAX_VALUE)}, - {Map.of("create", "true", "tempFileThreshold", "helloworld")}, - {Map.of("create", "true", "tempFileThreshold", 124232.232d)}, - {Map.of("create", "true", "tempFileThreshold", 122.232f)} - }; - } - - /** - * Create a zip filesystem with various different values of tempFileThreshold, some valid and - * some invalid. Make sure that in all these cases, the zip filesystem creation works fine - * and entries can be added to the zip file and read from and the content is as expected. - */ - @Test(dataProvider = "zipfsThresholdValueParsing") - public void testThresholdValueParsing(final Map env) throws Exception { - try (final FileSystem zipfs = FileSystems.newFileSystem(ZIP_FILE, env)) { - // create the zip file with just a simple entry - try (final OutputStream os = Files.newOutputStream(zipfs.getPath("helloworld.txt"))) { - os.write("hi".getBytes(StandardCharsets.UTF_8)); - } - // now verify the written content - try (final InputStream is = Files.newInputStream(zipfs.getPath("helloworld.txt"))) { - Assert.assertEquals(is.readAllBytes(), "hi".getBytes(StandardCharsets.UTF_8), - "Unexpected content in zip entry"); - } - } - } - - @DataProvider(name = "zipfsVaryingThreshold") - private Object[][] zipfsVaryingThreshold() { - return new Object[][]{ - // default tempfile threshold, for both DEFLATED and STORED compression modes - {Map.of("create", "true", "noCompression", "true")}, - {Map.of("create", "true", "noCompression", "false")}, - // specific threshold for both DEFLATED and STORED compression modes - {Map.of("create", "true", "noCompression", "true", "tempFileThreshold", "1024")}, - {Map.of("create", "true", "noCompression", "false", "tempFileThreshold", "1024")}, - // threshold disabled - {Map.of("create", "true", "noCompression", "true", "tempFileThreshold", "-1")}, - {Map.of("create", "true", "noCompression", "false", "tempFileThreshold", "-1")}, - {Map.of("create", "true", "noCompression", "true", "tempFileThreshold", "0")}, - {Map.of("create", "true", "noCompression", "false", "tempFileThreshold", "0")}, + {Map.of("create", "true", "noCompression", "true")}, // STORED + {Map.of("create", "true", "noCompression", "false")} // DEFLATED }; } /** - * Create a zip filesystem using different outputstream temp file creation threshold and write - * out entries of varying sizes. Then verify that the generated zip file entries are as expected, + * Create a zip filesystem and write out entries of varying sizes using the outputstream returned + * by the ZipFileSystem. Then verify that the generated zip file entries are as expected, * both in size and content */ - @Test(dataProvider = "zipfsVaryingThreshold") - public void testOutputStreamWithDifferentThreshold(final Map env) throws Exception { + @Test(dataProvider = "zipFSCreationEnv") + public void testOutputStream(final Map env) throws Exception { final byte[] chunk = new byte[1024]; new Random().nextBytes(chunk); try (final FileSystem zipfs = FileSystems.newFileSystem(ZIP_FILE, env)) { From b6d453ff27f6a129981e3e954c10e44ee8caae8e Mon Sep 17 00:00:00 2001 From: jpai Date: Thu, 22 Jul 2021 21:16:14 +0530 Subject: [PATCH 12/14] review suggestion - wrap ByteArrayOutputStream instead of extending it --- .../classes/jdk/nio/zipfs/ZipFileSystem.java | 87 ++++++++----------- 1 file changed, 37 insertions(+), 50 deletions(-) diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java index bfe5c50f4b700..071aa147193f4 100644 --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -33,7 +33,6 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; -import java.io.UncheckedIOException; import java.lang.Runtime.Version; import java.nio.ByteBuffer; import java.nio.MappedByteBuffer; @@ -1976,22 +1975,14 @@ private class EntryOutputStream extends FilterOutputStream { @Override public synchronized void write(int b) throws IOException { - try { - out.write(b); - } catch (UncheckedIOException uioe) { - throw uioe.getCause(); - } + out.write(b); written += 1; } @Override public synchronized void write(byte[] b, int off, int len) throws IOException { - try { - out.write(b, off, len); - } catch (UncheckedIOException uioe) { - throw uioe.getCause(); - } + out.write(b, off, len); written += len; } @@ -2026,11 +2017,7 @@ private class DeflatingEntryOutputStream extends DeflaterOutputStream { @Override public synchronized void write(byte[] b, int off, int len) throws IOException { - try { - super.write(b, off, len); - } catch (UncheckedIOException uioe) { - throw uioe.getCause(); - } + super.write(b, off, len); crc.update(b, off, len); } @@ -2137,18 +2124,18 @@ public void close() throws IOException { // byte array, to that newly created file. The temp file is then opened // in append mode and any subsequent writes, including the one which triggered // the temporary file creation, will be written to the file. - private class FileRolloverOutputStream extends ByteArrayOutputStream { + private class FileRolloverOutputStream extends OutputStream { + private ByteArrayOutputStream baos = new ByteArrayOutputStream(8192); private final Entry entry; - private long totalWritten = 0; private OutputStream tmpFileOS; + private long totalWritten = 0; - FileRolloverOutputStream(final Entry e) { - super(8192); + private FileRolloverOutputStream(final Entry e) { this.entry = e; } @Override - public synchronized void write(int b) throws UncheckedIOException { + public synchronized void write(final int b) throws IOException { if (tmpFileOS != null) { // already rolled over, write to the file that has been created previously writeToFile(b); @@ -2156,21 +2143,22 @@ public synchronized void write(int b) throws UncheckedIOException { } if (totalWritten + 1 < tempFileCreationThreshold) { // write to our in-memory byte array - super.write(b); + baos.write(b); totalWritten++; return; } // rollover into a file - try { - transferToFile(); - } catch (IOException e) { - throw new UncheckedIOException(e); - } + transferToFile(); writeToFile(b); } @Override - public synchronized void write(byte[] b, int off, int len) throws UncheckedIOException { + public void write(final byte[] b) throws IOException { + write(b, 0, b.length); + } + + @Override + public synchronized void write(final byte[] b, final int off, final int len) throws IOException { if (tmpFileOS != null) { // already rolled over, write to the file that has been created previously writeToFile(b, off, len); @@ -2178,41 +2166,37 @@ public synchronized void write(byte[] b, int off, int len) throws UncheckedIOExc } if (totalWritten + len < tempFileCreationThreshold) { // write to our in-memory byte array - super.write(b, off, len); + baos.write(b, off, len); totalWritten += len; return; } // rollover into a file - try { - transferToFile(); - } catch (IOException e) { - throw new UncheckedIOException(e); - } + transferToFile(); writeToFile(b, off, len); } + @Override + public void flush() throws IOException { + if (tmpFileOS != null) { + tmpFileOS.flush(); + } + } + @Override public void close() throws IOException { + baos = null; if (tmpFileOS != null) { tmpFileOS.close(); } } - private void writeToFile(int b) throws UncheckedIOException { - try { - tmpFileOS.write(b); - } catch (IOException e) { - throw new UncheckedIOException(e); - } + private void writeToFile(int b) throws IOException { + tmpFileOS.write(b); totalWritten++; } - private void writeToFile(byte[] b, int off, int len) throws UncheckedIOException { - try { - tmpFileOS.write(b, off, len); - } catch (IOException e) { - throw new UncheckedIOException(e); - } + private void writeToFile(byte[] b, int off, int len) throws IOException { + tmpFileOS.write(b, off, len); totalWritten += len; } @@ -2221,14 +2205,17 @@ private void transferToFile() throws IOException { entry.file = getTempPathForEntry(null); // transfer the already written data from the byte array buffer into this tempfile try (OutputStream os = new BufferedOutputStream(Files.newOutputStream(entry.file))) { - new ByteArrayInputStream(buf, 0, count).transferTo(os); + baos.writeTo(os); } - // clear the in-memory buffer and shrink the buffer - reset(); - buf = new byte[0]; + // release the underlying byte array + baos = null; // append any further data to the file with buffering enabled tmpFileOS = new BufferedOutputStream(Files.newOutputStream(entry.file, APPEND)); } + + private byte[] toByteArray() { + return baos == null ? null : baos.toByteArray(); + } } private InputStream getInputStream(Entry e) From 90101d45110cdb8cd3d0db026868af8f3a8b5281 Mon Sep 17 00:00:00 2001 From: jpai Date: Thu, 22 Jul 2021 21:16:51 +0530 Subject: [PATCH 13/14] remove no longer necessary javadoc comment on test --- test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java b/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java index 7132195e8fd7d..1dcfe461dda12 100644 --- a/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java +++ b/test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java @@ -37,9 +37,6 @@ import java.util.concurrent.TimeUnit; /** - * This is intentionally a manual test. The (jtreg) configurations below are here only - * for reference about runtime expectations of this test. - * * @test * @bug 8190753 8011146 * @summary Verify that using zip filesystem for opening an outputstream for a zip entry whose From 9e78ba06a6fa9cdcee5b7ec7f792bc0f3f618ddd Mon Sep 17 00:00:00 2001 From: jpai Date: Fri, 23 Jul 2021 17:17:37 +0530 Subject: [PATCH 14/14] Implement review suggestions: - remove unnecessary "synchronized" - no need to open the temp file twice --- .../classes/jdk/nio/zipfs/ZipFileSystem.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java index 071aa147193f4..f04d8af64dd97 100644 --- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java +++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java @@ -2124,6 +2124,10 @@ public void close() throws IOException { // byte array, to that newly created file. The temp file is then opened // in append mode and any subsequent writes, including the one which triggered // the temporary file creation, will be written to the file. + // Implementation note: the "write" and the "close" methods of this implementation + // aren't "synchronized" because this FileRolloverOutputStream gets called + // only from either DeflatingEntryOutputStream or EntryOutputStream, both of which + // already have the necessary "synchronized" before calling these methods. private class FileRolloverOutputStream extends OutputStream { private ByteArrayOutputStream baos = new ByteArrayOutputStream(8192); private final Entry entry; @@ -2135,7 +2139,7 @@ private FileRolloverOutputStream(final Entry e) { } @Override - public synchronized void write(final int b) throws IOException { + public void write(final int b) throws IOException { if (tmpFileOS != null) { // already rolled over, write to the file that has been created previously writeToFile(b); @@ -2158,7 +2162,7 @@ public void write(final byte[] b) throws IOException { } @Override - public synchronized void write(final byte[] b, final int off, final int len) throws IOException { + public void write(final byte[] b, final int off, final int len) throws IOException { if (tmpFileOS != null) { // already rolled over, write to the file that has been created previously writeToFile(b, off, len); @@ -2203,14 +2207,11 @@ private void writeToFile(byte[] b, int off, int len) throws IOException { private void transferToFile() throws IOException { // create a tempfile entry.file = getTempPathForEntry(null); + tmpFileOS = new BufferedOutputStream(Files.newOutputStream(entry.file)); // transfer the already written data from the byte array buffer into this tempfile - try (OutputStream os = new BufferedOutputStream(Files.newOutputStream(entry.file))) { - baos.writeTo(os); - } + baos.writeTo(tmpFileOS); // release the underlying byte array baos = null; - // append any further data to the file with buffering enabled - tmpFileOS = new BufferedOutputStream(Files.newOutputStream(entry.file, APPEND)); } private byte[] toByteArray() {