You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by pk...@apache.org on 2025/08/10 16:15:40 UTC
(commons-compress) 01/01: Simplify `ArArchiveInputStream` special record handling
This is an automated email from the ASF dual-hosted git repository.
pkarwasz pushed a commit to branch feat/simplify-ar-logic
in repository https://gitbox.apache.org/repos/asf/commons-compress.git
commit cab0988bf0165577920a31d6a765ded53edb430f
Author: Piotr P. Karwasz <pk...@apache.org>
AuthorDate: Sun Aug 10 18:14:07 2025 +0200
Simplify `ArArchiveInputStream` special record handling
Refactors `ArArchiveInputStream.getNextEntry()` to follow the same structural approach as `TarArchiveInputStream.getNextEntry()`, improving consistency and maintainability.
**Key changes:**
* Unifies parsing of the GNU string table with the pattern used for TAR special records.
* Replaces the _ad-hoc_ byte counter with the inherited `count()` method for more accurate and readable tracking.
* Adds a sanity check: throws an exception if a regular file entry does not follow a special AR member (GNU string table).
---
src/changes/changes.xml | 1 +
.../compress/archivers/ar/ArArchiveEntry.java | 3 +
.../archivers/ar/ArArchiveInputStream.java | 302 +++++++++++++--------
.../apache/commons/compress/archivers/ArTest.java | 10 +-
.../archivers/ar/ArArchiveInputStreamTest.java | 6 +-
5 files changed, 196 insertions(+), 126 deletions(-)
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index 8733be649..f2f32126a 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -72,6 +72,7 @@ The <action> type attribute can be add,update,fix,remove.
<!-- FIX ar -->
<action type="fix" dev="ggregory" due-to="Gary Gregory">org.apache.commons.compress.archivers.ar.ArArchiveInputStream.readGNUStringTable(byte[], int, int) now provides a better exception message, wrapping the underlying exception.</action>
<action type="fix" dev="ggregory" due-to="Gary Gregory">org.apache.commons.compress.archivers.ar.ArArchiveInputStream.read(byte[], int, int) now throws ArchiveException instead of ArithmeticException.</action>
+ <action type="fix" dev="pkarwasz" due-to="Tyler Nighswander">Simplify handling of special ar records in ArArchiveInputStream.</action>
<!-- FIX unpack200 -->
<action type="fix" dev="ggregory" due-to="Gary Gregory">org.apache.commons.compress.harmony.unpack200 now throws Pack200Exception, IllegalArgumentException, and IllegalStateException instead of Error.</action>
<!-- FIX pack200 -->
diff --git a/src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveEntry.java b/src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveEntry.java
index d42dafbf8..154d6c901 100644
--- a/src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveEntry.java
+++ b/src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveEntry.java
@@ -20,6 +20,7 @@
import java.io.File;
import java.io.IOException;
+import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.LinkOption;
import java.nio.file.Path;
@@ -61,6 +62,8 @@ public class ArArchiveEntry implements ArchiveEntry {
/** The header for each entry */
public static final String HEADER = "!<arch>\n";
+ static final byte[] HEADER_BYTES = HEADER.getBytes(StandardCharsets.US_ASCII);
+
/** The trailer for each entry {@code 0x60 0x0A} */
public static final String TRAILER = "`\012";
diff --git a/src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java b/src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java
index 2c6fba80e..1034e26dd 100644
--- a/src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java
+++ b/src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java
@@ -58,11 +58,6 @@ public class ArArchiveInputStream extends ArchiveInputStream<ArArchiveEntry> {
private static final String GNU_STRING_TABLE_NAME = "//";
private static final Pattern GNU_LONGNAME_PATTERN = Pattern.compile("^/\\d+");
- /**
- * Signature {@code 3c21 7261 6863 0a3e}
- */
- private static final byte[] SIGNATURE = { 0x21, 0x3c, 0x61, 0x72, 0x63, 0x68, 0x3e, 0x0a };
-
/**
* Does the name look like it is a long name (or a name containing spaces) as encoded by BSD ar?
* <p>
@@ -103,8 +98,8 @@ private static boolean isBSDLongName(final String name) {
* are number of characters, not line or string number within the "//" file.
* </p>
*/
- private static boolean isGNUStringTable(final String name) {
- return GNU_STRING_TABLE_NAME.equals(name);
+ private static boolean isGNUStringTable(final ArArchiveEntry entry) {
+ return GNU_STRING_TABLE_NAME.equals(entry.getName());
}
/**
@@ -115,11 +110,9 @@ private static boolean isGNUStringTable(final String name) {
* @return true, if this stream is an Ar archive stream, false otherwise.
*/
public static boolean matches(final byte[] buffer, final int ignored) {
- return ArrayUtils.startsWith(buffer, SIGNATURE);
+ return ArrayUtils.startsWith(buffer, ArArchiveEntry.HEADER_BYTES);
}
- private long offset;
-
private boolean closed;
/*
@@ -131,7 +124,7 @@ public static boolean matches(final byte[] buffer, final int ignored) {
private byte[] namebuffer;
/**
- * The offset where the current entry started. -1 if no entry has been called
+ * The offset where the data for the current entry starts.
*/
private long entryOffset = -1;
@@ -147,24 +140,22 @@ public ArArchiveInputStream(final InputStream inputStream) {
super(inputStream, StandardCharsets.US_ASCII.name());
}
- private int asInt(final byte[] byteArray, final int offset, final int len) throws IOException {
- return asInt(byteArray, offset, len, 10, false);
+ private int asOptionalDecimal(final byte[] byteArray, final int offset, final int len) throws IOException {
+ return asInt(byteArray, offset, len, 10);
}
- private int asInt(final byte[] byteArray, final int offset, final int len, final boolean treatBlankAsZero) throws IOException {
- return asInt(byteArray, offset, len, 10, treatBlankAsZero);
+ private int asOptionalOctal(final byte[] byteArray, final int offset, final int len) throws IOException {
+ return asInt(byteArray, offset, len, 8);
}
private int asInt(final byte[] byteArray, final int offset, final int len, final int base) throws IOException {
- return asInt(byteArray, offset, len, base, false);
+ final String string = ArchiveUtils.toAsciiString(byteArray, offset, len).trim();
+ return string.isEmpty() ? 0 : ParsingUtils.parseIntValue(string, base);
}
- private int asInt(final byte[] byteArray, final int offset, final int len, final int base, final boolean treatBlankAsZero) throws IOException {
+ private long asOptionalLong(final byte[] byteArray, final int offset, final int len) throws IOException {
final String string = ArchiveUtils.toAsciiString(byteArray, offset, len).trim();
- if (string.isEmpty() && treatBlankAsZero) {
- return 0;
- }
- return ParsingUtils.parseIntValue(string, base);
+ return string.isEmpty() ? 0 : ParsingUtils.parseLongValue(string);
}
private long asLong(final byte[] byteArray, final int offset, final int len) throws IOException {
@@ -195,7 +186,7 @@ private String getBSDLongName(final String bsdLongName) throws IOException {
final int nameLen = ParsingUtils.parseIntValue(bsdLongName.substring(BSD_LONGNAME_PREFIX_LEN));
final byte[] name = IOUtils.readRange(in, nameLen);
final int read = name.length;
- trackReadBytes(read);
+ count(read);
if (read != nameLen) {
throw new EOFException(bsdLongName);
}
@@ -241,102 +232,160 @@ private String getExtendedName(final int offset) throws IOException {
*/
@Deprecated
public ArArchiveEntry getNextArEntry() throws IOException {
- if (currentEntry != null) {
- final long entryEnd = entryOffset + currentEntry.getLength();
- final long skipped = org.apache.commons.io.IOUtils.skip(in, entryEnd - offset);
- trackReadBytes(skipped);
- currentEntry = null;
- }
- if (offset == 0) {
- final byte[] expected = ArchiveUtils.toAsciiBytes(ArArchiveEntry.HEADER);
- final byte[] realized = IOUtils.readRange(in, expected.length);
- final int read = realized.length;
- trackReadBytes(read);
- if (read != expected.length) {
- throw new ArchiveException("Failed to read header. Occurred at byte: %,d", getBytesRead());
- }
- if (!Arrays.equals(expected, realized)) {
- throw new ArchiveException("Invalid header " + ArchiveUtils.toAsciiString(realized));
- }
- }
- if (offset % 2 != 0) {
- if (in.read() < 0) {
- // hit eof
- return null;
- }
- trackReadBytes(1);
- }
- {
- final int read = IOUtils.readFully(in, metaData);
- trackReadBytes(read);
- if (read == 0) {
- return null;
- }
- if (read < metaData.length) {
- throw new ArchiveException("Truncated ar archive");
+ return getNextEntry();
+ }
+
+ /*
+ * Returns the next AR file entry in this stream.
+ * <p>
+ * The method skips special AR file entries, such as those used by GNU.
+ * </p>
+ * @return The next AR file entry.
+ * @throws IOException if the entry could not be read or is malformed.
+ */
+ @Override
+ public ArArchiveEntry getNextEntry() throws IOException {
+ skipGlobalSignature();
+
+ // Handle special GNU ar entries
+ boolean lastWasGNUStringTable = false;
+ do {
+ // If there is a current entry, skip any unread data and padding
+ if (currentEntry != null) {
+ IOUtils.skip(this, Long.MAX_VALUE); // Skip to end of current entry
+ skipRecordPadding(); // Skip padding to align to the next record
}
- }
- {
- final byte[] expected = ArchiveUtils.toAsciiBytes(ArArchiveEntry.TRAILER);
- final byte[] realized = IOUtils.readRange(in, expected.length);
- final int read = realized.length;
- trackReadBytes(read);
- if (read != expected.length) {
- throw new ArchiveException("Failed to read entry trailer. Occurred at byte: " + getBytesRead());
+
+ // Read the next header record
+ final byte[] headerBuf = getRecord();
+ if (headerBuf == null) {
+ // If we encountered special records but no file entry, the archive is malformed
+ if (lastWasGNUStringTable) {
+ throw new ArchiveException(
+ "Premature end of ar archive. Didn't find any file entry after GNU string table.");
+ }
+ currentEntry = null;
+ return null; // End of archive
}
- if (!Arrays.equals(expected, realized)) {
- throw new ArchiveException("Invalid entry trailer. not read the content? Occurred at byte: " + getBytesRead());
+ checkTrailer();
+
+ // Parse the header into a new entry
+ currentEntry = parseEntry(headerBuf);
+ entryOffset = getBytesRead(); // Store the offset of the entry
+
+ lastWasGNUStringTable = isGNUStringTable(currentEntry);
+ if (lastWasGNUStringTable) {
+ // If this is a GNU string table entry, read the extended names and continue
+ namebuffer = readGNUStringTable(currentEntry);
}
- }
- entryOffset = offset;
- // GNU ar uses a '/' to mark the end of the file name; this allows for the use of spaces without the use of an extended file name.
+ } while (lastWasGNUStringTable);
+
+ // GNU ar uses a '/' to mark the end of the file name; this allows for the use of spaces without the use of an
+ // extended file name.
// entry name is stored as ASCII string
- String temp = ArchiveUtils.toAsciiString(metaData, NAME_OFFSET, NAME_LEN).trim();
- if (isGNUStringTable(temp)) { // GNU extended file names entry
- currentEntry = readGNUStringTable(metaData, LENGTH_OFFSET, LENGTH_LEN);
- return getNextArEntry();
- }
- long len;
- try {
- len = asLong(metaData, LENGTH_OFFSET, LENGTH_LEN);
- } catch (final NumberFormatException e) {
- throw new ArchiveException("Broken archive, unable to parse ar_size field as a number", (Throwable) e);
- }
- if (temp.endsWith("/")) { // GNU terminator
- temp = temp.substring(0, temp.length() - 1);
- } else if (isGNULongName(temp)) {
- final int off = ParsingUtils.parseIntValue(temp.substring(1)); // get the offset
- temp = getExtendedName(off); // convert to the long name
- } else if (isBSDLongName(temp)) {
- temp = getBSDLongName(temp);
+ String name = currentEntry.getName();
+ long len = currentEntry.getLength();
+ if (name.endsWith("/")) { // GNU terminator
+ name = name.substring(0, name.length() - 1);
+ } else if (isGNULongName(name)) {
+ final int off = ParsingUtils.parseIntValue(name.substring(1)); // get the offset
+ name = getExtendedName(off); // convert to the long name
+ } else if (isBSDLongName(name)) {
+ name = getBSDLongName(name);
// entry length contained the length of the file name in
// addition to the real length of the entry.
// assume file name was ASCII, there is no "standard" otherwise
- final int nameLen = temp.length();
+ final int nameLen = name.length();
len -= nameLen;
entryOffset += nameLen;
}
- if (len < 0) {
+ if (currentEntry.getSize() < 0) {
throw new ArchiveException("Broken archive, entry with negative size");
}
- try {
- currentEntry = new ArArchiveEntry(temp, len, asInt(metaData, USER_ID_OFFSET, USER_ID_LEN, true),
- asInt(metaData, GROUP_ID_OFFSET, GROUP_ID_LEN, true), asInt(metaData, FILE_MODE_OFFSET, FILE_MODE_LEN, 8),
- asLong(metaData, LAST_MODIFIED_OFFSET, LAST_MODIFIED_LEN));
- return currentEntry;
- } catch (final NumberFormatException e) {
- throw new ArchiveException("Broken archive, unable to parse entry metadata fields as numbers", (Throwable) e);
+ currentEntry = new ArArchiveEntry(
+ name,
+ len,
+ currentEntry.getUserId(),
+ currentEntry.getGroupId(),
+ currentEntry.getMode(),
+ currentEntry.getLastModified());
+ return currentEntry;
+ }
+
+ /**
+ * Skips the global archive signature if at the beginning of the stream.
+ *
+ * @throws IOException if an I/O error occurs while reading the stream or if the signature is invalid.
+ */
+ private void skipGlobalSignature() throws IOException {
+ final long offset = getBytesRead();
+ if (offset == 0) {
+ final byte[] expectedMagic = ArArchiveEntry.HEADER_BYTES;
+ final byte[] actualMagic = IOUtils.readRange(in, expectedMagic.length);
+ count(actualMagic.length);
+ if (expectedMagic.length != actualMagic.length) {
+ throw new ArchiveException("Failed to read header. Occurred at byte: %,d", getBytesRead());
+ }
+ if (!Arrays.equals(expectedMagic, actualMagic)) {
+ throw new ArchiveException("Invalid header " + ArchiveUtils.toAsciiString(actualMagic));
+ }
}
}
- /*
- * (non-Javadoc)
+ /**
+ * Skips the padding bytes at the end of each record.
+ * <p>
+ * The AR format requires that each record is padded to an even number of bytes, so if the current offset is odd,
+ * we skip one byte.
+ * </p>
*
- * @see org.apache.commons.compress.archivers.ArchiveInputStream#getNextEntry()
+ * @throws IOException if an I/O error occurs while reading the stream.
*/
- @Override
- public ArArchiveEntry getNextEntry() throws IOException {
- return getNextArEntry();
+ private void skipRecordPadding() throws IOException {
+ // If the offset is odd, we need to skip one byte
+ final long offset = getBytesRead();
+ if (offset % 2 != 0) {
+ count(in.read() < 0 ? 0 : 1);
+ }
+ }
+
+ /**
+ * Reads the next raw record from the input stream.
+ * <p>
+ * The record is expected to be of a fixed size defined by the AR format.
+ * </p>
+ *
+ * @return the byte array containing the record data, or null if the end of the stream is reached.
+ * @throws IOException if an I/O error occurs while reading the stream or if the record is malformed.
+ */
+ private byte[] getRecord() throws IOException {
+ final int read = IOUtils.readFully(in, metaData);
+ count(read);
+ if (read == 0) {
+ return null;
+ }
+ if (read < metaData.length) {
+ throw new ArchiveException("Truncated ar archive");
+ }
+ return metaData;
+ }
+
+ /**
+ * Checks and skips the trailer of the current entry.
+ *
+ * @throws IOException if the trailer is invalid or not read correctly.
+ */
+ private void checkTrailer() throws IOException {
+ // Check and skip the record trailer
+ final byte[] expectedTrailer = ArchiveUtils.toAsciiBytes(ArArchiveEntry.TRAILER);
+ final byte[] actualTrailer = IOUtils.readRange(in, expectedTrailer.length);
+ count(actualTrailer.length);
+ if (expectedTrailer.length != actualTrailer.length) {
+ throw new ArchiveException("Failed to read entry trailer. Occurred at byte: " + getBytesRead());
+ }
+ if (!Arrays.equals(expectedTrailer, actualTrailer)) {
+ throw new ArchiveException("Invalid entry trailer. not read the content? Occurred at byte: " + getBytesRead());
+ }
}
/**
@@ -362,12 +411,13 @@ public int read(final byte[] b, final int off, final int len) throws IOException
throw new IllegalStateException("No current ar entry");
}
final long entryEnd = entryOffset + currentEntry.getLength();
+ final long offset = getBytesRead();
if (len < 0 || offset >= entryEnd) {
return -1;
}
final int toRead = ArchiveException.toIntExact(Math.min(len, entryEnd - offset));
- final int ret = this.in.read(b, off, toRead);
- trackReadBytes(ret);
+ final int ret = in.read(b, off, toRead);
+ count(ret);
return ret;
}
@@ -376,26 +426,42 @@ public int read(final byte[] b, final int off, final int len) throws IOException
*
* @see #isGNUStringTable
*/
- private ArArchiveEntry readGNUStringTable(final byte[] length, final int offset, final int len) throws IOException {
- final int bufflen;
- try {
- bufflen = asInt(length, offset, len); // Assume length will fit in an int
- } catch (final IOException e) {
- throw new ArchiveException("Broken archive, unable to parse GNU string table length field as a number", (Throwable) e);
+ private byte[] readGNUStringTable(final ArArchiveEntry entry) throws IOException {
+ if (entry.getLength() > Integer.MAX_VALUE) {
+ throw new ArchiveException("Invalid GNU string table entry size: " + entry.getLength());
}
- namebuffer = IOUtils.readRange(in, bufflen);
+ final int size = (int) entry.getLength();
+ final byte [] namebuffer = IOUtils.readRange(in, size);
final int read = namebuffer.length;
- trackReadBytes(read);
- if (read != bufflen) {
- throw new ArchiveException("Failed to read complete // record: expected=" + bufflen + " read=" + read);
+ count((long) read);
+ if (read != size) {
+ throw new ArchiveException("Failed to read complete // record: expected=" + size + " read=" + read);
}
- return new ArArchiveEntry(GNU_STRING_TABLE_NAME, bufflen);
+ return namebuffer;
}
- private void trackReadBytes(final long read) {
- count(read);
- if (read > 0) {
- offset += read;
+ /**
+ * Parses the entry metadata from the provided raw record.
+ *
+ * @param headerBuf the buffer containing the entry metadata.
+ * @return an {@link ArArchiveEntry} object containing the parsed metadata.
+ * @throws IOException if the metadata cannot be parsed correctly.
+ */
+ private ArArchiveEntry parseEntry(final byte[] headerBuf) throws IOException {
+ // Parse the entry metadata from the header buffer
+ try {
+ final String name =
+ ArchiveUtils.toAsciiString(headerBuf, NAME_OFFSET, NAME_LEN).trim();
+ final long length = asLong(headerBuf, LENGTH_OFFSET, LENGTH_LEN);
+ final int userId = asOptionalDecimal(headerBuf, USER_ID_OFFSET, USER_ID_LEN);
+ final int groupId = asOptionalDecimal(headerBuf, GROUP_ID_OFFSET, GROUP_ID_LEN);
+ final int mode = asOptionalOctal(headerBuf, FILE_MODE_OFFSET, FILE_MODE_LEN);
+ final long lastModified = asOptionalLong(headerBuf, LAST_MODIFIED_OFFSET, LAST_MODIFIED_LEN);
+ return new ArArchiveEntry(name, length, userId, groupId, mode, lastModified);
+ } catch (final IllegalArgumentException e) {
+ throw new ArchiveException("Broken archive, entry with negative size", (Throwable) e);
+ } catch (final IOException e) {
+ throw new ArchiveException("Failed to parse ar entry.", (Throwable) e);
}
}
}
diff --git a/src/test/java/org/apache/commons/compress/archivers/ArTest.java b/src/test/java/org/apache/commons/compress/archivers/ArTest.java
index ceb7501dc..965005779 100644
--- a/src/test/java/org/apache/commons/compress/archivers/ArTest.java
+++ b/src/test/java/org/apache/commons/compress/archivers/ArTest.java
@@ -181,7 +181,7 @@ void testExplicitFileEntry() throws Exception {
//
final ArArchiveEntry out;
try (ArArchiveInputStream ais = new ArArchiveInputStream(Files.newInputStream(archive.toPath()))) {
- out = ais.getNextArEntry();
+ out = ais.getNextEntry();
}
assertNotNull(out);
assertEquals("foo", out.getName());
@@ -202,7 +202,7 @@ void testFileEntryFromFile() throws Exception {
}
final ArArchiveEntry out;
try (ArArchiveInputStream ais = new ArArchiveInputStream(Files.newInputStream(archive.toPath()))) {
- out = ais.getNextArEntry();
+ out = ais.getNextEntry();
}
assertNotNull(out);
assertEquals("foo", out.getName());
@@ -224,7 +224,7 @@ void testFileEntryFromPath() throws Exception {
}
final ArArchiveEntry out;
try (ArArchiveInputStream ais = new ArArchiveInputStream(Files.newInputStream(archive.toPath()))) {
- out = ais.getNextArEntry();
+ out = ais.getNextEntry();
}
assertNotNull(out);
assertEquals("foo", out.getName());
@@ -249,7 +249,7 @@ void testXtestDirectoryEntryFromFile() throws Exception {
}
final ArArchiveEntry out;
try (ArArchiveInputStream ais = new ArArchiveInputStream(Files.newInputStream(archive.toPath()))) {
- out = ais.getNextArEntry();
+ out = ais.getNextEntry();
}
assertNotNull(out);
assertEquals("foo/", out.getName());
@@ -274,7 +274,7 @@ void testXtestExplicitDirectoryEntry() throws Exception {
}
final ArArchiveEntry out;
try (ArArchiveInputStream ais = new ArArchiveInputStream(Files.newInputStream(archive.toPath()))) {
- out = ais.getNextArEntry();
+ out = ais.getNextEntry();
}
assertNotNull(out);
assertEquals("foo/", out.getName());
diff --git a/src/test/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStreamTest.java b/src/test/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStreamTest.java
index d6190f5d4..caf3d0be7 100644
--- a/src/test/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStreamTest.java
+++ b/src/test/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStreamTest.java
@@ -201,14 +201,14 @@ public int read() throws IOException {
}
}) {
try (ArArchiveInputStream archiveInputStream = new ArArchiveInputStream(simpleInputStream)) {
- final ArArchiveEntry entry1 = archiveInputStream.getNextArEntry();
+ final ArArchiveEntry entry1 = archiveInputStream.getNextEntry();
assertNotNull(entry1);
assertEquals("test1.xml", entry1.getName());
assertEquals(610L, entry1.getLength());
- final ArArchiveEntry entry2 = archiveInputStream.getNextArEntry();
+ final ArArchiveEntry entry2 = archiveInputStream.getNextEntry();
assertEquals("test2.xml", entry2.getName());
assertEquals(82L, entry2.getLength());
- assertNull(archiveInputStream.getNextArEntry());
+ assertNull(archiveInputStream.getNextEntry());
}
}
}