You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2025/08/10 17:15:21 UTC
[PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
ppkarwasz opened a new pull request, #685:
URL: https://github.com/apache/commons-compress/pull/685
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).v
* Consistently throws `EOFException` in case of truncated content.
* Improves consistency of error messages.
- [x] I used AI to proofread the pull request.
- [x] Run a successful build using the default [Maven](https://maven.apache.org/) goal with `mvn`; that's `mvn` on the command line by itself.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #685:
URL: https://github.com/apache/commons-compress/pull/685#discussion_r2265435809
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -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 {
Review Comment:
I renamed/added these methods because I slightly changed how `ar` entries are parsed.
Previously, GNU string table entries were handled differently from other `ar` entries:
* For GNU string table entries, only the length and name fields were parsed, since the other fields are usually blank.
* For all other entries, every header field was parsed.
In this PR, I introduced a unified `parseEntry` method that parses both kinds of members. The new `asOptionalDecimal`, etc., methods handle cases where numeric fields are blank (as they often are for the GNU string table).
Initially, I considered adding an `if` to skip parsing `mtime`, `uid`, `gid`, and `mode` for GNU string table entries. But those fields aren’t critical even for normal files, so it’s cleaner to parse them as optional and avoid throwing conversion exceptions when they’re blank.
What do you think? Should we keep the original parsing logic or relax it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #685:
URL: https://github.com/apache/commons-compress/pull/685#discussion_r2265448147
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -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 {
Review Comment:
Fixed in https://github.com/apache/commons-compress/pull/685/commits/27f247a40d2f984b2dab79a7860b1d2e69088421
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #685:
URL: https://github.com/apache/commons-compress/pull/685#discussion_r2265456448
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -241,102 +232,172 @@ 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));
+ 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 foundGNUStringTable = 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
}
- }
- if (offset % 2 != 0) {
- if (in.read() < 0) {
- // hit eof
- return null;
+
+ // 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 (foundGNUStringTable) {
+ throw new EOFException("Premature end of ar archive: no regular entry after GNU string table.");
+ }
Review Comment:
Documented in https://github.com/apache/commons-compress/pull/685/commits/bce21564d764e24eb842bba8ce221b307c92a4b0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #685:
URL: https://github.com/apache/commons-compress/pull/685#discussion_r2265449394
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -241,102 +232,172 @@ 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));
+ 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 foundGNUStringTable = 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
}
- }
- if (offset % 2 != 0) {
- if (in.read() < 0) {
- // hit eof
- return null;
+
+ // 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 (foundGNUStringTable) {
+ throw new EOFException("Premature end of ar archive: no regular entry after GNU string table.");
+ }
+ currentEntry = null;
+ return null; // End of archive
}
- trackReadBytes(1);
- }
- {
- final int read = IOUtils.readFully(in, metaData);
- trackReadBytes(read);
- if (read == 0) {
- return null;
+ checkTrailer();
+
+ // Parse the header into a new entry
+ currentEntry = parseEntry(headerBuf);
+ entryOffset = getBytesRead(); // Store the offset of the entry
+
+ foundGNUStringTable = isGNUStringTable(currentEntry);
+ if (foundGNUStringTable) {
+ // If this is a GNU string table entry, read the extended names and continue
+ namebuffer = readGNUStringTable(currentEntry);
}
- if (read < metaData.length) {
- throw new ArchiveException("Truncated ar archive");
+ } while (foundGNUStringTable);
+
+ // Handle long file names and other special cases
+ String name = currentEntry.getName();
+ long len = currentEntry.getLength();
+ // Handle GNU ar: names ending with '/' are terminated (allows spaces in names)
+ if (name.endsWith("/")) {
+ name = name.substring(0, name.length() - 1);
+ } else if (isGNULongName(name)) {
+ // GNU ar: name is a reference to the string table (e.g., "/42"), resolve the actual name
+ final int off = ParsingUtils.parseIntValue(name.substring(1));
+ name = getExtendedName(off);
+ } else if (isBSDLongName(name)) {
+ // BSD ar: name is stored after the header, retrieve it
+ name = getBSDLongName(name);
+ // The entry length includes the file name length; adjust to get the actual file data length
+ final int nameLen = name.length();
+ if (nameLen > len) {
+ throw new ArchiveException(
+ "Invalid BSD long name: file name length (" + nameLen + ") exceeds entry length (" + len + ")");
}
+ len -= nameLen;
+ entryOffset += nameLen;
}
- {
- 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());
+
+ currentEntry = new ArArchiveEntry(
Review Comment:
Fixed in https://github.com/apache/commons-compress/pull/685/commits/da05e0ecd6860c51934b7751637961cc14011f41
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #685:
URL: https://github.com/apache/commons-compress/pull/685#discussion_r2265428892
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -241,102 +232,172 @@ 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));
+ 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 foundGNUStringTable = 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
}
- }
- if (offset % 2 != 0) {
- if (in.read() < 0) {
- // hit eof
- return null;
+
+ // 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 (foundGNUStringTable) {
+ throw new EOFException("Premature end of ar archive: no regular entry after GNU string table.");
+ }
Review Comment:
Then let's leave a link to the spec in the `//` comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #685:
URL: https://github.com/apache/commons-compress/pull/685#discussion_r2265438849
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -241,102 +232,172 @@ 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));
+ 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 foundGNUStringTable = 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
}
- }
- if (offset % 2 != 0) {
- if (in.read() < 0) {
- // hit eof
- return null;
+
+ // 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 (foundGNUStringTable) {
+ throw new EOFException("Premature end of ar archive: no regular entry after GNU string table.");
+ }
+ currentEntry = null;
+ return null; // End of archive
}
- trackReadBytes(1);
- }
- {
- final int read = IOUtils.readFully(in, metaData);
- trackReadBytes(read);
- if (read == 0) {
- return null;
+ checkTrailer();
+
+ // Parse the header into a new entry
+ currentEntry = parseEntry(headerBuf);
+ entryOffset = getBytesRead(); // Store the offset of the entry
+
+ foundGNUStringTable = isGNUStringTable(currentEntry);
+ if (foundGNUStringTable) {
+ // If this is a GNU string table entry, read the extended names and continue
+ namebuffer = readGNUStringTable(currentEntry);
}
- if (read < metaData.length) {
- throw new ArchiveException("Truncated ar archive");
+ } while (foundGNUStringTable);
+
+ // Handle long file names and other special cases
+ String name = currentEntry.getName();
+ long len = currentEntry.getLength();
+ // Handle GNU ar: names ending with '/' are terminated (allows spaces in names)
+ if (name.endsWith("/")) {
+ name = name.substring(0, name.length() - 1);
+ } else if (isGNULongName(name)) {
+ // GNU ar: name is a reference to the string table (e.g., "/42"), resolve the actual name
+ final int off = ParsingUtils.parseIntValue(name.substring(1));
+ name = getExtendedName(off);
+ } else if (isBSDLongName(name)) {
+ // BSD ar: name is stored after the header, retrieve it
+ name = getBSDLongName(name);
+ // The entry length includes the file name length; adjust to get the actual file data length
+ final int nameLen = name.length();
+ if (nameLen > len) {
+ throw new ArchiveException(
+ "Invalid BSD long name: file name length (" + nameLen + ") exceeds entry length (" + len + ")");
}
+ len -= nameLen;
+ entryOffset += nameLen;
}
- {
- 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());
+
+ currentEntry = new ArArchiveEntry(
Review Comment:
It does enforce some formatting through Checkstyle (for example, line length). I certainly hope we'll never format our code in this style anywhere, like... not worth mentioning ;-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #685:
URL: https://github.com/apache/commons-compress/pull/685#discussion_r2265372434
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -211,25 +202,25 @@ private String getBSDLongName(final String bsdLongName) throws IOException {
*/
private String getExtendedName(final int offset) throws IOException {
if (namebuffer == null) {
- throw new ArchiveException("Cannot process GNU long file name as no // record was found");
+ throw new ArchiveException("Cannot process GNU long file name as no GNU string table was found");
+ }
+ if (offset < 0 || offset >= namebuffer.length) {
Review Comment:
While `ParsingUtils.parseIntValue` will **not** throw an exception for negative values, the parsed string can **not** contain a sign, so this improvement is justified.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #685:
URL: https://github.com/apache/commons-compress/pull/685#discussion_r2265431016
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -241,102 +232,172 @@ 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));
+ 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 foundGNUStringTable = 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
}
- }
- if (offset % 2 != 0) {
- if (in.read() < 0) {
- // hit eof
- return null;
+
+ // 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 (foundGNUStringTable) {
+ throw new EOFException("Premature end of ar archive: no regular entry after GNU string table.");
+ }
+ currentEntry = null;
+ return null; // End of archive
}
- trackReadBytes(1);
- }
- {
- final int read = IOUtils.readFully(in, metaData);
- trackReadBytes(read);
- if (read == 0) {
- return null;
+ checkTrailer();
+
+ // Parse the header into a new entry
+ currentEntry = parseEntry(headerBuf);
+ entryOffset = getBytesRead(); // Store the offset of the entry
+
+ foundGNUStringTable = isGNUStringTable(currentEntry);
+ if (foundGNUStringTable) {
+ // If this is a GNU string table entry, read the extended names and continue
+ namebuffer = readGNUStringTable(currentEntry);
}
- if (read < metaData.length) {
- throw new ArchiveException("Truncated ar archive");
+ } while (foundGNUStringTable);
+
+ // Handle long file names and other special cases
+ String name = currentEntry.getName();
+ long len = currentEntry.getLength();
+ // Handle GNU ar: names ending with '/' are terminated (allows spaces in names)
+ if (name.endsWith("/")) {
+ name = name.substring(0, name.length() - 1);
+ } else if (isGNULongName(name)) {
+ // GNU ar: name is a reference to the string table (e.g., "/42"), resolve the actual name
+ final int off = ParsingUtils.parseIntValue(name.substring(1));
+ name = getExtendedName(off);
+ } else if (isBSDLongName(name)) {
+ // BSD ar: name is stored after the header, retrieve it
+ name = getBSDLongName(name);
+ // The entry length includes the file name length; adjust to get the actual file data length
+ final int nameLen = name.length();
+ if (nameLen > len) {
+ throw new ArchiveException(
+ "Invalid BSD long name: file name length (" + nameLen + ") exceeds entry length (" + len + ")");
}
+ len -= nameLen;
+ entryOffset += nameLen;
}
- {
- 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());
+
+ currentEntry = new ArArchiveEntry(
Review Comment:
Since Commons does **not** enforce a deterministic formatting style, I stick to the Palantir Java format so I don’t have to decide when to insert a newline and when not.
I’m fairly agnostic about the specific style, my main goal is to avoid thinking about formatting at all, so if you have a “better” option in mind, I’m happy to switch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #685:
URL: https://github.com/apache/commons-compress/pull/685#discussion_r2265416961
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -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 {
Review Comment:
I don't understand these method name changes. What's optional if there's always a return value? Either the method names can remain the same, or some Javadoc would be helpful. WDYT?
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -241,102 +232,172 @@ 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));
+ 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 foundGNUStringTable = 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
}
- }
- if (offset % 2 != 0) {
- if (in.read() < 0) {
- // hit eof
- return null;
+
+ // 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 (foundGNUStringTable) {
+ throw new EOFException("Premature end of ar archive: no regular entry after GNU string table.");
+ }
+ currentEntry = null;
+ return null; // End of archive
}
- trackReadBytes(1);
- }
- {
- final int read = IOUtils.readFully(in, metaData);
- trackReadBytes(read);
- if (read == 0) {
- return null;
+ checkTrailer();
+
+ // Parse the header into a new entry
+ currentEntry = parseEntry(headerBuf);
+ entryOffset = getBytesRead(); // Store the offset of the entry
+
+ foundGNUStringTable = isGNUStringTable(currentEntry);
+ if (foundGNUStringTable) {
+ // If this is a GNU string table entry, read the extended names and continue
+ namebuffer = readGNUStringTable(currentEntry);
}
- if (read < metaData.length) {
- throw new ArchiveException("Truncated ar archive");
+ } while (foundGNUStringTable);
+
+ // Handle long file names and other special cases
+ String name = currentEntry.getName();
+ long len = currentEntry.getLength();
+ // Handle GNU ar: names ending with '/' are terminated (allows spaces in names)
+ if (name.endsWith("/")) {
+ name = name.substring(0, name.length() - 1);
+ } else if (isGNULongName(name)) {
+ // GNU ar: name is a reference to the string table (e.g., "/42"), resolve the actual name
+ final int off = ParsingUtils.parseIntValue(name.substring(1));
+ name = getExtendedName(off);
+ } else if (isBSDLongName(name)) {
+ // BSD ar: name is stored after the header, retrieve it
+ name = getBSDLongName(name);
+ // The entry length includes the file name length; adjust to get the actual file data length
+ final int nameLen = name.length();
+ if (nameLen > len) {
+ throw new ArchiveException(
+ "Invalid BSD long name: file name length (" + nameLen + ") exceeds entry length (" + len + ")");
}
+ len -= nameLen;
+ entryOffset += nameLen;
}
- {
- 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());
+
+ currentEntry = new ArArchiveEntry(
Review Comment:
I'm not a fan of what I call "newspaper" formatting. Why is this API call in a column and not another?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #685:
URL: https://github.com/apache/commons-compress/pull/685#discussion_r2265367933
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -241,102 +232,172 @@ 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));
+ 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 foundGNUStringTable = 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
}
- }
- if (offset % 2 != 0) {
- if (in.read() < 0) {
- // hit eof
- return null;
+
+ // 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 (foundGNUStringTable) {
+ throw new EOFException("Premature end of ar archive: no regular entry after GNU string table.");
+ }
Review Comment:
This check incorrectly assumes that a GNU string table must be followed by a regular entry. However, a GNU string table could be the last entry in an archive if it's not actually referenced by any subsequent entries. This would cause valid archives to be rejected.
```suggestion
// End of archive (even if the last entry was a GNU string table, this is valid)
```
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -211,25 +202,25 @@ private String getBSDLongName(final String bsdLongName) throws IOException {
*/
private String getExtendedName(final int offset) throws IOException {
if (namebuffer == null) {
- throw new ArchiveException("Cannot process GNU long file name as no // record was found");
+ throw new ArchiveException("Cannot process GNU long file name as no GNU string table was found");
+ }
+ if (offset < 0 || offset >= namebuffer.length) {
Review Comment:
The condition `offset < 0` is unnecessary because the offset is parsed from a string using `ParsingUtils.parseIntValue(name.substring(1))` at line 290, which would throw an exception for negative values. The check should focus on the upper bound only.
```suggestion
if (offset >= namebuffer.length) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #685:
URL: https://github.com/apache/commons-compress/pull/685#discussion_r2265439816
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -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 {
Review Comment:
> I renamed/added these methods because I slightly changed how `ar` entries are parsed.
>
> Previously, GNU string table entries were handled differently from other `ar` entries:
>
> * For GNU string table entries, only the length and name fields were parsed, since the other fields are usually blank.
> * For all other entries, every header field was parsed.
>
> In this PR, I introduced a unified `parseEntry` method that parses both kinds of members. The new `asOptionalDecimal`, etc., methods handle cases where numeric fields are blank (as they often are for the GNU string table).
>
> Initially, I considered adding an `if` to skip parsing `mtime`, `uid`, `gid`, and `mode` for GNU string table entries. But those fields aren’t critical even for normal files, so it’s cleaner to parse them as optional and avoid throwing conversion exceptions when they’re blank.
>
> What do you think? Should we keep the original parsing logic or relax it?
All of the above doesn't answer my question. I think we leave the behavior as is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #685:
URL: https://github.com/apache/commons-compress/pull/685#discussion_r2265370653
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -241,102 +232,172 @@ 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));
+ 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 foundGNUStringTable = 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
}
- }
- if (offset % 2 != 0) {
- if (in.read() < 0) {
- // hit eof
- return null;
+
+ // 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 (foundGNUStringTable) {
+ throw new EOFException("Premature end of ar archive: no regular entry after GNU string table.");
+ }
Review Comment:
I’d keep the exception as it is unless we can point to a real-world tool that writes a trailing GNU string table. The FreeBSD `ar(5)` manual explicitly states:
> If present, this member immediately follows the archive symbol table if an archive symbol table is present, or is the first member otherwise.
Source: [FreeBSD man ar(5)](https://man.freebsd.org/cgi/man.cgi?query=ar&sektion=5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #685:
URL: https://github.com/apache/commons-compress/pull/685#issuecomment-3172915413
> Some minor comments plus `org.apache.commons.compress.archivers.ar.ArArchiveInputStreamTest.testGetNextArEntry()` needs to be enabled and updated.
I enabled `testGetNextArEntry` in 1f1f82fcd660546ed05f5cf41d7f37cecdd04d51, end removed the last (only) file entry, so that it fails with an `EOFException` as discussed in https://github.com/apache/commons-compress/pull/685#discussion_r2265367933
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Simplify `ArArchiveInputStream` special record handling [commons-compress]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #685:
URL: https://github.com/apache/commons-compress/pull/685
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org