Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream #4607

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
123 changes: 117 additions & 6 deletions src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
Expand Up @@ -119,6 +119,11 @@ 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

// a threshold, in bytes, to decide whether to create a temp file
// 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"
// METHOD_DEFLATED otherwise
Expand Down Expand Up @@ -1944,11 +1949,11 @@ private OutputStream getOutputStream(Entry e) throws IOException {
if (zc.isUTF8())
e.flag |= FLAG_USE_UTF8;
OutputStream os;
if (useTempFile) {
if (useTempFile || e.size >= tempFileCreationThreshold) {
e.file = getTempPathForEntry(null);
os = Files.newOutputStream(e.file, WRITE);
} else {
os = new ByteArrayOutputStream((e.size > 0)? (int)e.size : 8192);
os = new FileRolloverOutputStream(e);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed change will address the specific issue shown in the bug. As Alan points out, there could be an issue if the deflated size is > 2gb. It would be good to look into that as part of your proposed fix.

if (e.method == METHOD_DEFLATED) {
return new DeflatingEntryOutputStream(e, os);
Expand Down Expand Up @@ -1988,8 +1993,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);
}
Expand Down Expand Up @@ -2024,8 +2030,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);
Expand Down Expand Up @@ -2107,6 +2114,110 @@ 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.
private class FileRolloverOutputStream extends OutputStream {
private ByteArrayOutputStream baos = new ByteArrayOutputStream(8192);
private final Entry entry;
private OutputStream tmpFileOS;
private long totalWritten = 0;

private FileRolloverOutputStream(final Entry e) {
this.entry = e;
}

@Override
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);
return;
}
if (totalWritten + 1 < tempFileCreationThreshold) {
// write to our in-memory byte array
baos.write(b);
totalWritten++;
return;
}
// rollover into a file
transferToFile();
writeToFile(b);
}

@Override
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);
return;
}
if (totalWritten + len < tempFileCreationThreshold) {
// write to our in-memory byte array
baos.write(b, off, len);
totalWritten += len;
return;
}
// rollover into a file
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 IOException {
tmpFileOS.write(b);
totalWritten++;
}

private void writeToFile(byte[] b, int off, int len) throws IOException {
tmpFileOS.write(b, off, len);
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))) {
baos.writeTo(os);
}
// 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)
throws IOException
{
Expand Down
98 changes: 98 additions & 0 deletions test/jdk/jdk/nio/zipfs/LargeCompressedEntrySizeTest.java
@@ -0,0 +1,98 @@
/*
* 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.IOException;
import java.io.OutputStream;
import java.nio.file.FileSystem;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Collections;
import java.util.Random;
import java.util.concurrent.TimeUnit;

/**
* @test
* @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
*/
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));
}


/**
* 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 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 = largeEntrySize;
// create a chunk of random bytes which we keep writing out
final int chunkSize = 102400;
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);
os.write(chunk, 0, numToWrite);
remaining -= numToWrite;
}
System.out.println("Took " + TimeUnit.SECONDS.toSeconds(System.currentTimeMillis() - start)
+ " seconds to generate entry of size " + largeEntrySize);
}
}
}

}