Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/changes/changes.xml
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ The <action> type attribute can be add,update,fix,remove.
<action type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz">Correct byte accounting and truncation errors in ARJ input stream.</action>
<action type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz">Add strict header validation in ARJ input stream and `selfExtracting` option.</action>
<!-- FIX unpack200 -->
<action type="fix" dev="tallison" due-to="Tim Allison">Pack200CompressorInputStream no longer throws InaccessibleObjectException when decompressing a FileInputStream or buffered stream on Java 17 and later; org.apache.commons.compress.harmony.unpack200 no longer reflects into java.io internals.</action>
<action type="fix" dev="ggregory" due-to="Gary Gregory, Stanislav Fort">org.apache.commons.compress.harmony.unpack200 now throws Pack200Exception, IllegalArgumentException, and IllegalStateException instead of other runtime exceptions and Error.</action>
<action type="fix" dev="pkarwasz" due-to="Piotr P. Karwasz">Enforce strict attribute layout parsing in Pack200.</action>
<!-- FIX pack200 -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@

import java.io.BufferedInputStream;
import java.io.BufferedOutputStream;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.IOException;
Expand Down Expand Up @@ -80,11 +79,9 @@ public class Archive {
public Archive(final InputStream inputStream, final JarOutputStream outputStream) throws IOException {
this.inputStream = Pack200UnpackerAdapter.newBoundedInputStream(inputStream);
this.outputStream = outputStream;
if (inputStream instanceof FileInputStream) {
inputPath = Paths.get(Pack200UnpackerAdapter.readPathString((FileInputStream) inputStream));
} else {
inputPath = null;
}
// The input path is unknown when constructed from a stream: we cannot recover it from an arbitrary InputStream without reflecting into java.io, which is
// forbidden under JPMS on Java 17+. It is only used for verbose logging here, and removePackFile has no effect for the stream constructor anyway.
this.inputPath = null;
this.outputFileName = null;
this.inputSize = -1;
this.closeStreams = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@

import java.io.BufferedInputStream;
import java.io.File;
import java.io.FileInputStream;
import java.io.FilterInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.net.URISyntaxException;
Expand All @@ -35,8 +33,6 @@
import org.apache.commons.compress.harmony.pack200.Pack200Exception;
import org.apache.commons.compress.java.util.jar.Pack200.Unpacker;
import org.apache.commons.io.input.BoundedInputStream;
import org.apache.commons.io.input.CloseShieldInputStream;
import org.apache.commons.lang3.reflect.FieldUtils;

/**
* This class provides the binding between the standard Pack200 interface and the internal interface for (un)packing.
Expand All @@ -59,28 +55,28 @@ static BoundedInputStream newBoundedInputStream(final File file) throws IOExcept
return newBoundedInputStream(file.toPath());
}

private static BoundedInputStream newBoundedInputStream(final FileInputStream fileInputStream) throws IOException {
return newBoundedInputStream(readPathString(fileInputStream));
}

/**
* Wraps the given InputStream in a {@link BoundedInputStream}, unless it already is one.
* <p>
* The stream is bounded only by its own end of file. We deliberately do not try to recover the size of an underlying file to set an explicit bound: doing so
* would require reflecting into the internal fields of {@code java.io.FileInputStream} / {@code java.io.FilterInputStream}, which is not permitted under the
* Java Platform Module System and throws {@code InaccessibleObjectException} on Java 17 and later (unless the caller adds
* {@code --add-opens java.base/java.io=ALL-UNNAMED}). Bounding a {@code FileInputStream} to its file size is equivalent to reading it to its natural end of
* file anyway, so nothing is lost.
* </p>
*
* @param inputStream the InputStream to wrap.
* @return a BoundedInputStream.
* @throws IOException if an I/O error occurs.
*/
@SuppressWarnings("resource") // Caller closes.
static BoundedInputStream newBoundedInputStream(final InputStream inputStream) throws IOException {
if (inputStream instanceof BoundedInputStream) {
// Already bound.
return (BoundedInputStream) inputStream;
}
if (inputStream instanceof CloseShieldInputStream) {
// Don't unwrap to keep close shield.
return newBoundedInputStream(BoundedInputStream.builder().setInputStream(inputStream).get());
}
if (inputStream instanceof FilterInputStream) {
return newBoundedInputStream(unwrap((FilterInputStream) inputStream));
}
if (inputStream instanceof FileInputStream) {
return newBoundedInputStream((FileInputStream) inputStream);
}
// No limit
return newBoundedInputStream(BoundedInputStream.builder().setInputStream(inputStream).get());
// No explicit limit: the BoundedInputStream is bounded by the underlying stream's end of file.
return BoundedInputStream.builder().setInputStream(inputStream).get();
}

/**
Expand Down Expand Up @@ -134,29 +130,6 @@ static BoundedInputStream newBoundedInputStream(final URL url) throws IOExceptio
return newBoundedInputStream(Paths.get(url.toURI()));
}

@SuppressWarnings("unchecked")
private static <T> T readField(final Object object, final String fieldName) {
try {
return (T) FieldUtils.readField(object, fieldName, true);
} catch (final IllegalAccessException e) {
return null;
}
}

static String readPathString(final FileInputStream fis) {
return readField(fis, "path");
}

/**
* Unwraps the given FilterInputStream to return its wrapped InputStream.
*
* @param filterInputStream The FilterInputStream to unwrap.
* @return The wrapped InputStream
*/
static InputStream unwrap(final FilterInputStream filterInputStream) {
return readField(filterInputStream, "in");
}

/**
* Constructs a new Pack200UnpackerAdapter.
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.commons.compress.compressors.pack200;

import java.io.BufferedInputStream;
import java.io.FileInputStream;
import java.io.InputStream;
import java.nio.file.Files;
import java.nio.file.Paths;

/**
* Not a unit test: a tiny program launched in a <em>separate</em> JVM by {@link Pack200ModuleAccessTest}, deliberately <strong>without</strong>
* {@code --add-opens java.base/java.io=ALL-UNNAMED}.
* <p>
* Decompressing a Pack200 stream must not require reflective access to {@code java.io} internals. When it did (see the history of
* {@code Pack200UnpackerAdapter}), this program threw {@link java.lang.reflect.InaccessibleObjectException} on Java 17 and later for any input that was a
* {@link FileInputStream} or a {@link java.io.FilterInputStream} (such as a {@link BufferedInputStream}). This is the exact situation real callers hit, because
* the project's own test suite hides it by adding {@code --add-opens} to Surefire.
* </p>
* <p>
* Usage: {@code java -cp <classpath> org.apache.commons.compress.compressors.pack200.Pack200ModuleAccessHelper <packFile>}. Prints one line per input form and
* exits with status {@code 0} only if every form decompressed; otherwise it prints the failure and exits non-zero.
* </p>
*/
public final class Pack200ModuleAccessHelper {

/**
* Decompresses {@code packFile} once for each interesting input-stream form.
*
* @param args {@code args[0]} is the path to a Pack200 file.
*/
public static void main(final String[] args) throws Exception {
final String packFile = args[0];
int failures = 0;
// A raw FileInputStream used to trigger reflection on java.io.FileInputStream.path.
failures += attempt("FileInputStream", new FileInputStream(packFile));
// A BufferedInputStream (a FilterInputStream) used to trigger reflection on java.io.FilterInputStream.in.
failures += attempt("BufferedInputStream", new BufferedInputStream(new FileInputStream(packFile)));
// Files.newInputStream returns a non-FilterInputStream that never needed reflection; included as a control.
failures += attempt("Files.newInputStream", Files.newInputStream(Paths.get(packFile)));
if (failures > 0) {
System.out.println("FAILED: " + failures + " input form(s) could not be decompressed without --add-opens.");
System.exit(1);
}
System.out.println("SUCCESS: all input forms decompressed without --add-opens.");
}

private static int attempt(final String label, final InputStream rawInput) {
try (InputStream in = rawInput;
Pack200CompressorInputStream pack200 = new Pack200CompressorInputStream(in)) {
final byte[] buffer = new byte[8192];
long total = 0;
int n;
while ((n = pack200.read(buffer)) != -1) {
total += n;
}
System.out.println(" OK " + label + " -> " + total + " bytes");
return 0;
} catch (final Throwable t) {
System.out.println(" FAIL " + label + " -> " + t);
return 1;
}
}

private Pack200ModuleAccessHelper() {
// Utility class launched via main(String[]).
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.commons.compress.compressors.pack200;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assumptions.assumeTrue;

import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;

import org.apache.commons.io.IOUtils;
import org.junit.jupiter.api.Test;

/**
* Guarantees that decompressing a Pack200 stream works on a stock JVM, i.e. <strong>without</strong> {@code --add-opens java.base/java.io=ALL-UNNAMED}.
* <p>
* This cannot be checked in-process: the project's Surefire configuration adds that flag on Java 17+ (it is required by other tests that reflect into
* {@code java.io}). With the package open, the formerly-reflective unpack code passes too, so an ordinary test could not tell the fix from the bug. This test
* therefore launches {@link Pack200ModuleAccessHelper} in a fresh child JVM with no such flag, reproducing exactly what a real caller's JVM looks like, and
* asserts the child succeeds.
* </p>
*
* @see Pack200ModuleAccessHelper
*/
class Pack200ModuleAccessTest {

/** Strong encapsulation (denying reflective {@code setAccessible} on non-open {@code java.base} packages by default) only applies on Java 16 and later. */
private static final int MIN_ENFORCING_JAVA = 16;

private static int specVersion() {
// "1.8" on Java 8, "9"/"11"/"17"/... on Java 9+.
final String version = System.getProperty("java.specification.version");
if (version.startsWith("1.")) {
return Integer.parseInt(version.substring(2));
}
return Integer.parseInt(version);
}

@Test
void unpackDoesNotRequireAddOpens() throws Exception {
assumeTrue(specVersion() >= MIN_ENFORCING_JAVA,
"On Java < " + MIN_ENFORCING_JAVA + " reflective access to java.io is permitted, so a child JVM cannot reproduce the failure.");
final Path packFile = Paths.get(getClass().getResource("/pack200/HelloWorld.pack").toURI());
final boolean windows = System.getProperty("os.name").toLowerCase().contains("win");
final String javaBin = Paths.get(System.getProperty("java.home"), "bin", windows ? "java.exe" : "java").toString();
// java.class.path is enough to relaunch on the same classpath (it is honored even when Surefire uses a manifest-only booter JAR).
final ProcessBuilder builder = new ProcessBuilder(javaBin, "-cp", System.getProperty("java.class.path"),
Pack200ModuleAccessHelper.class.getName(), packFile.toString()).redirectErrorStream(true);
// Make sure no ambient flags (e.g. an --add-opens smuggled in via JAVA_TOOL_OPTIONS) leak into the child and mask the failure.
builder.environment().remove("JAVA_TOOL_OPTIONS");
builder.environment().remove("_JAVA_OPTIONS");
final Process process = builder.start();
final ExecutorService reader = Executors.newSingleThreadExecutor();
try {
final Future<String> outputFuture = reader.submit(() -> IOUtils.toString(process.getInputStream(), StandardCharsets.UTF_8));
final boolean finished = process.waitFor(2, TimeUnit.MINUTES);
if (!finished) {
process.destroyForcibly();
}
final String output = outputFuture.get(30, TimeUnit.SECONDS);
assertTrue(finished, () -> "Child JVM did not finish in time. Output:\n" + output);
assertEquals(0, process.exitValue(),
() -> "Decompressing a Pack200 stream must not require --add-opens java.base/java.io=ALL-UNNAMED.\nChild JVM output:\n" + output);
// Sanity: the child really exercised the FileInputStream path that used to throw InaccessibleObjectException.
assertTrue(output.contains("OK FileInputStream"), () -> "Unexpected child JVM output:\n" + output);
} finally {
reader.shutdownNow();
}
}
}
Loading