You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by gg...@apache.org on 2024/04/26 18:57:53 UTC
(commons-compress) branch master updated (ca4760dad -> 810db5ae7)
This is an automated email from the ASF dual-hosted git repository.
ggregory pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/commons-compress.git
from ca4760dad [COMPRESS-675] Regression in pack200's Archive class -- underlying InputStream is now closed
new 364b563a2 Make private instance variable final
new 810db5ae7 unpack200.Archive.unpack() should not close streams it does not own
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
src/changes/changes.xml | 1 +
.../pack200/Pack200CompressorInputStream.java | 7 +-
.../compress/harmony/unpack200/Archive.java | 13 +-
.../compress/harmony/unpack200/ArchiveTest.java | 194 ++++++++++-----------
4 files changed, 107 insertions(+), 108 deletions(-)
(commons-compress) 02/02: unpack200.Archive.unpack() should not close streams it does not own
Posted by gg...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-compress.git
commit 810db5ae7ca6981c110dd13188a58a1c0cd29a5d
Author: Gary Gregory <ga...@gmail.com>
AuthorDate: Fri Apr 26 14:57:48 2024 -0400
unpack200.Archive.unpack() should not close streams it does not own
- org.apache.commons.compress.harmony.unpack200.Archive.unpack() should
not close streams it does not own (when constructed from
Archive(InputStream, JarOutputStream))
- Use try-with-resources
- Less side-effects in tests
---
src/changes/changes.xml | 1 +
.../pack200/Pack200CompressorInputStream.java | 7 +-
.../compress/harmony/unpack200/Archive.java | 10 +-
.../compress/harmony/unpack200/ArchiveTest.java | 194 ++++++++++-----------
4 files changed, 105 insertions(+), 107 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 814d004bf..f10980e24 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -51,6 +51,7 @@ The <action> type attribute can be add,update,fix,remove.
<action type="fix" issue="COMPRESS-676" dev="ecki" due-to="Martin Schneider">OSGi: Remove unresolvable dependencies in manifest and make the commons-io packages really optional.</action>
<action type="fix" dev="ggregory" due-to="Gary Gregory">Avoid possible NullPointerException in org.apache.commons.compress.utils.Sets.newHashSet(E...).</action>
<action type="fix" issue="COMPRESS-677" dev="ggregory" due-to="Jeffrey Adamson, Gary Gregory">ZipArchiveOutputStream.setEncoding(String) with a null value throws IllegalArgumentException.</action>
+ <action type="fix" dev="ggregory" due-to="Gary Gregory">org.apache.commons.compress.harmony.unpack200.Archive.unpack() should not close streams it does not own (when constructed from Archive(InputStream, JarOutputStream)).</action>
<!-- UPDATE -->
<action type="update" dev="ggregory" due-to="Dependabot, Gary Gregory">Bump org.apache.commons:commons-parent from 66 to 69 #495, #508.</action>
<action type="update" dev="ggregory" due-to="Dependabot, Gary Gregory">Bump org.ow2.asm:asm from 9.6 to 9.7 #504.</action>
diff --git a/src/main/java/org/apache/commons/compress/compressors/pack200/Pack200CompressorInputStream.java b/src/main/java/org/apache/commons/compress/compressors/pack200/Pack200CompressorInputStream.java
index 758029400..b64458aba 100644
--- a/src/main/java/org/apache/commons/compress/compressors/pack200/Pack200CompressorInputStream.java
+++ b/src/main/java/org/apache/commons/compress/compressors/pack200/Pack200CompressorInputStream.java
@@ -28,7 +28,6 @@ import java.util.jar.JarOutputStream;
import org.apache.commons.compress.compressors.CompressorInputStream;
import org.apache.commons.compress.java.util.jar.Pack200;
-import org.apache.commons.io.input.CloseShieldInputStream;
/**
* An input stream that decompresses from the Pack200 format to be read as any other stream.
@@ -138,11 +137,7 @@ public class Pack200CompressorInputStream extends CompressorInputStream {
unpacker.properties().putAll(properties);
}
if (file == null) {
- // unpack would close this stream but we want to give the call site more control
- // TODO unpack should not close its given stream.
- try (CloseShieldInputStream closeShield = CloseShieldInputStream.wrap(inputStream)) {
- unpacker.unpack(closeShield, jarOut);
- }
+ unpacker.unpack(inputStream, jarOut);
} else {
unpacker.unpack(file, jarOut);
}
diff --git a/src/main/java/org/apache/commons/compress/harmony/unpack200/Archive.java b/src/main/java/org/apache/commons/compress/harmony/unpack200/Archive.java
index 1a80c978f..d134a656d 100644
--- a/src/main/java/org/apache/commons/compress/harmony/unpack200/Archive.java
+++ b/src/main/java/org/apache/commons/compress/harmony/unpack200/Archive.java
@@ -64,6 +64,8 @@ public class Archive {
private final String outputFileName;
+ private final boolean closeStreams;
+
/**
* Creates an Archive with streams for the input and output files. Note: If you use this method then calling {@link #setRemovePackFile(boolean)} will have
* no effect.
@@ -82,6 +84,7 @@ public class Archive {
}
this.outputFileName = null;
this.inputSize = -1;
+ this.closeStreams = false;
}
/**
@@ -99,6 +102,7 @@ public class Archive {
this.inputStream = new BoundedInputStream(Files.newInputStream(inputPath), inputSize);
this.outputStream = new JarOutputStream(new BufferedOutputStream(new FileOutputStream(outputFileName)));
this.outputFileName = outputFileName;
+ this.closeStreams = true;
}
private boolean available(final InputStream inputStream) throws IOException {
@@ -214,8 +218,10 @@ public class Archive {
}
}
} finally {
- IOUtils.closeQuietly(inputStream);
- IOUtils.closeQuietly(outputStream);
+ if (closeStreams) {
+ IOUtils.closeQuietly(inputStream);
+ IOUtils.closeQuietly(outputStream);
+ }
IOUtils.closeQuietly(logFile);
}
if (removePackFile && inputPath != null) {
diff --git a/src/test/java/org/apache/commons/compress/harmony/unpack200/ArchiveTest.java b/src/test/java/org/apache/commons/compress/harmony/unpack200/ArchiveTest.java
index 67d983300..f123f7d3a 100644
--- a/src/test/java/org/apache/commons/compress/harmony/unpack200/ArchiveTest.java
+++ b/src/test/java/org/apache/commons/compress/harmony/unpack200/ArchiveTest.java
@@ -40,10 +40,8 @@ import java.util.jar.JarOutputStream;
import java.util.zip.ZipEntry;
import org.apache.commons.compress.AbstractTempDirTest;
-import org.apache.commons.io.IOUtils;
import org.apache.commons.io.input.BoundedInputStream;
import org.apache.commons.io.output.NullOutputStream;
-import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
@@ -53,41 +51,33 @@ import org.junit.jupiter.params.provider.ValueSource;
*/
public class ArchiveTest extends AbstractTempDirTest {
- InputStream in;
- JarOutputStream out;
-
- @AfterEach
- public void tearDown() throws Exception {
- IOUtils.closeQuietly(in, IOException::printStackTrace);
- IOUtils.closeQuietly(out, IOException::printStackTrace);
- }
-
@Test
public void testAlternativeConstructor() throws Exception {
final String inputFile = new File(Archive.class.getResource("/pack200/sql.pack.gz").toURI()).getPath();
final File file = createTempFile("sql", ".jar");
final String outputFile = file.getPath();
- final Archive archive = new Archive(inputFile, outputFile);
- archive.unpack();
+ new Archive(inputFile, outputFile).unpack();
}
@Test
public void testDeflateHint() throws Exception {
- in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
File file = createTempFile("sql", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- Archive archive = new Archive(in, out);
- archive.setDeflateHint(true);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ archive.setDeflateHint(true);
+ archive.unpack();
+ }
try (JarFile jarFile = new JarFile(file)) {
assertEquals(ZipEntry.DEFLATED, jarFile.getEntry("bin/test/org/apache/harmony/sql/tests/internal/rowset/CachedRowSetImplTest.class").getMethod());
-
- in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
- file = createTempFile("sql", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- archive = new Archive(in, out);
- archive.setDeflateHint(false);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz")) {
+ file = createTempFile("sql", ".jar");
+ try (JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ archive.setDeflateHint(false);
+ archive.unpack();
+ }
+ }
}
try (JarFile jarFile = new JarFile(file)) {
assertEquals(ZipEntry.STORED, jarFile.getEntry("bin/test/org/apache/harmony/sql/tests/internal/rowset/CachedRowSetImplTest.class").getMethod());
@@ -96,74 +86,73 @@ public class ArchiveTest extends AbstractTempDirTest {
@Test
public void testJustResourcesGZip() throws Exception {
- in = Archive.class.getResourceAsStream("/pack200/JustResources.pack.gz");
final File file = createTempFile("Just", "ResourcesGz.jar");
- out = new JarOutputStream(new FileOutputStream(file));
- final Archive archive = new Archive(in, out);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/JustResources.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ new Archive(in, out).unpack();
+ }
}
// Test verbose, quiet and log file options.
@Test
public void testLoggingOptions() throws Exception {
// test default option, which is quiet (no output at all)
- in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
File file = createTempFile("logtest", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- Archive archive = new Archive(in, out);
File logFile = createTempFile("logfile", ".txt");
- archive.setLogFile(logFile.getPath());
- archive.unpack();
-
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ archive.setLogFile(logFile.getPath());
+ archive.unpack();
+ }
// log file should be empty
try (FileReader reader = new FileReader(logFile)) {
assertFalse(reader.ready());
}
-
// test verbose
- in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
file = createTempFile("logtest", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- archive = new Archive(in, out);
- logFile = createTempFile("logfile", ".txt");
- archive.setLogFile(logFile.getPath());
- archive.setVerbose(true);
- archive.unpack();
-
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ logFile = createTempFile("logfile", ".txt");
+ archive.setLogFile(logFile.getPath());
+ archive.setVerbose(true);
+ archive.unpack();
+ }
// log file should not be empty
try (FileReader reader = new FileReader(logFile)) {
assertTrue(reader.ready());
}
-
// test append option
final long length = logFile.length();
- in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
file = createTempFile("logtest", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- archive = new Archive(in, out);
- archive.setLogFile(logFile.getPath(), true);
- archive.setVerbose(true);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ archive.setLogFile(logFile.getPath(), true);
+ archive.setVerbose(true);
+ archive.unpack();
+ }
assertTrue(logFile.length() > length);
- in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
file = createTempFile("logtest", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- archive = new Archive(in, out);
- archive.setLogFile(logFile.getPath(), false);
- archive.setVerbose(true);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ archive.setLogFile(logFile.getPath(), false);
+ archive.setVerbose(true);
+ archive.unpack();
+ }
assertEquals(logFile.length(), length);
-
// test setting quiet explicitly
- in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
file = createTempFile("logtest", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- archive = new Archive(in, out);
- logFile = createTempFile("logfile", ".txt");
- archive.setLogFile(logFile.getPath());
- archive.setQuiet(true);
- archive.unpack();
-
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ logFile = createTempFile("logfile", ".txt");
+ archive.setLogFile(logFile.getPath());
+ archive.setQuiet(true);
+ archive.unpack();
+ }
// log file should be empty
try (FileReader reader = new FileReader(logFile)) {
assertFalse(reader.ready());
@@ -217,8 +206,8 @@ public class ArchiveTest extends AbstractTempDirTest {
@Test
public void testRemovePackFile() throws Exception {
- final File original = new File(Archive.class.getResource("/pack200/sql.pack.gz").toURI());
final File copy = createTempFile("sqlcopy", ".pack.gz");
+ final File original = new File(Archive.class.getResource("/pack200/sql.pack.gz").toURI());
try (BufferedInputStream inputStream = new BufferedInputStream(new FileInputStream(original));
BufferedOutputStream outputStream = new BufferedOutputStream(new FileOutputStream(copy))) {
final byte[] bytes = new byte[256];
@@ -240,72 +229,78 @@ public class ArchiveTest extends AbstractTempDirTest {
// Test with an archive containing Annotations
@Test
public void testWithAnnotations() throws Exception {
- in = Archive.class.getResourceAsStream("/pack200/annotations.pack.gz");
final File file = createTempFile("annotations", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- final Archive archive = new Archive(in, out);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/annotations.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ archive.unpack();
+ }
}
// Test with an archive packed with the -E0 option
@Test
public void testWithE0() throws Exception {
- in = Archive.class.getResourceAsStream("/pack200/simple-E0.pack.gz");
final File file = createTempFile("simple-e0", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- final Archive archive = new Archive(in, out);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/simple-E0.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ archive.unpack();
+ }
}
// Test with an archive containing Harmony's JNDI module
@Test
public void testWithJNDIE1() throws Exception {
- in = Archive.class.getResourceAsStream("/pack200/jndi-e1.pack.gz");
final File file = createTempFile("jndi-e1", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- final Archive archive = new Archive(in, out);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/jndi-e1.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ archive.unpack();
+ }
}
// Test with a class containing lots of local variables (regression test for
// HARMONY-5470)
@Test
public void testWithLargeClass() throws Exception {
- in = Archive.class.getResourceAsStream("/pack200/LargeClass.pack.gz");
final File file = createTempFile("largeClass", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- final Archive archive = new Archive(in, out);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/LargeClass.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ archive.unpack();
+ }
}
// Test with an archive containing Harmony's Pack200 module
@Test
public void testWithPack200() throws Exception {
- in = Archive.class.getResourceAsStream("/pack200/pack200.pack.gz");
final File file = createTempFile("p200", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- final Archive archive = new Archive(in, out);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/pack200.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ archive.unpack();
+ }
}
// Test with an archive containing Harmony's Pack200 module, packed with -E1
@Test
public void testWithPack200E1() throws Exception {
- in = Archive.class.getResourceAsStream("/pack200/pack200-e1.pack.gz");
final File file = createTempFile("p200-e1", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- final Archive archive = new Archive(in, out);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/pack200-e1.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ archive.unpack();
+ }
}
// Test with an archive containing Harmony's SQL module
@Test
public void testWithSql() throws Exception {
- in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
final File file = createTempFile("sql", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- final Archive archive = new Archive(in, out);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/sql.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ new Archive(in, out).unpack();
+ }
final File compareFile = new File(Archive.class.getResource("/pack200/sqlUnpacked.jar").toURI());
try (JarFile jarFile = new JarFile(file);
JarFile jarFile2 = new JarFile(compareFile)) {
@@ -349,11 +344,12 @@ public class ArchiveTest extends AbstractTempDirTest {
// Test with an archive containing Harmony's SQL module, packed with -E1
@Test
public void testWithSqlE1() throws Exception {
- in = Archive.class.getResourceAsStream("/pack200/sql-e1.pack.gz");
final File file = createTempFile("sql-e1", ".jar");
- out = new JarOutputStream(new FileOutputStream(file));
- final Archive archive = new Archive(in, out);
- archive.unpack();
+ try (InputStream in = Archive.class.getResourceAsStream("/pack200/sql-e1.pack.gz");
+ JarOutputStream out = new JarOutputStream(new FileOutputStream(file))) {
+ final Archive archive = new Archive(in, out);
+ archive.unpack();
+ }
}
}
(commons-compress) 01/02: Make private instance variable final
Posted by gg...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
ggregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-compress.git
commit 364b563a222d06eae650e4395098f4b7c089cd66
Author: Gary Gregory <ga...@gmail.com>
AuthorDate: Fri Apr 26 12:56:15 2024 -0400
Make private instance variable final
---
.../java/org/apache/commons/compress/harmony/unpack200/Archive.java | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/main/java/org/apache/commons/compress/harmony/unpack200/Archive.java b/src/main/java/org/apache/commons/compress/harmony/unpack200/Archive.java
index 7e292c86d..1a80c978f 100644
--- a/src/main/java/org/apache/commons/compress/harmony/unpack200/Archive.java
+++ b/src/main/java/org/apache/commons/compress/harmony/unpack200/Archive.java
@@ -62,7 +62,7 @@ public class Archive {
private final long inputSize;
- private String outputFileName;
+ private final String outputFileName;
/**
* Creates an Archive with streams for the input and output files. Note: If you use this method then calling {@link #setRemovePackFile(boolean)} will have
@@ -80,6 +80,7 @@ public class Archive {
} else {
inputPath = null;
}
+ this.outputFileName = null;
this.inputSize = -1;
}