Copilot commented on code in PR #790:
URL: https://github.com/apache/commons-compress/pull/790#discussion_r3667842035
##########
src/main/java/org/apache/commons/compress/archivers/cpio/CpioArchiveInputStream.java:
##########
@@ -432,9 +432,7 @@ private CpioArchiveEntry readNewEntry(final boolean hasCrc)
throws IOException {
}
newEntry.setInode(readAsciiLong(8, 16));
final long mode = readAsciiLong(8, 16);
- if (CpioUtil.fileType(mode) != 0) { // mode is initialized to 0
- newEntry.setMode(mode);
- }
+ setMode(newEntry, mode);
Review Comment:
`setMode(...)` declares `throws ArchiveException` but is called from methods
that (per this diff) declare only `throws IOException`, which will not compile
unless `ArchiveException` is an `IOException` subtype. Additionally, the `new
ArchiveException("... 0%s ... %,d", ...)` call looks like it relies on a
formatting constructor; Commons Compress’ `ArchiveException` typically only
accepts a message (and optionally a cause). Suggested fix: either (1) make
`setMode` throw `IOException` (or wrap into `IOException`) so it aligns with
the reader methods’ signatures, and format the message explicitly, or (2) catch
`ArchiveException` at the call sites and wrap/translate to `IOException`. Also
preserve the original `IllegalArgumentException` as the cause when rethrowing
so diagnostics aren’t lost.
##########
src/main/java/org/apache/commons/compress/archivers/cpio/CpioArchiveInputStream.java:
##########
@@ -470,9 +468,7 @@ private CpioArchiveEntry readOldAsciiEntry() throws
IOException {
ret.setDevice(readAsciiLong(6, 8));
ret.setInode(readAsciiLong(6, 8));
final long mode = readAsciiLong(6, 8);
- if (CpioUtil.fileType(mode) != 0) {
- ret.setMode(mode);
- }
+ setMode(ret, mode);
Review Comment:
`setMode(...)` declares `throws ArchiveException` but is called from methods
that (per this diff) declare only `throws IOException`, which will not compile
unless `ArchiveException` is an `IOException` subtype. Additionally, the `new
ArchiveException("... 0%s ... %,d", ...)` call looks like it relies on a
formatting constructor; Commons Compress’ `ArchiveException` typically only
accepts a message (and optionally a cause). Suggested fix: either (1) make
`setMode` throw `IOException` (or wrap into `IOException`) so it aligns with
the reader methods’ signatures, and format the message explicitly, or (2) catch
`ArchiveException` at the call sites and wrap/translate to `IOException`. Also
preserve the original `IllegalArgumentException` as the cause when rethrowing
so diagnostics aren’t lost.
##########
src/main/java/org/apache/commons/compress/archivers/cpio/CpioArchiveInputStream.java:
##########
@@ -500,9 +496,7 @@ private CpioArchiveEntry readOldBinaryEntry(final boolean
swapHalfWord) throws I
oldEntry.setDevice(readBinaryLong(2, swapHalfWord));
oldEntry.setInode(readBinaryLong(2, swapHalfWord));
final long mode = readBinaryLong(2, swapHalfWord);
- if (CpioUtil.fileType(mode) != 0) {
- oldEntry.setMode(mode);
- }
+ setMode(oldEntry, mode);
Review Comment:
`setMode(...)` declares `throws ArchiveException` but is called from methods
that (per this diff) declare only `throws IOException`, which will not compile
unless `ArchiveException` is an `IOException` subtype. Additionally, the `new
ArchiveException("... 0%s ... %,d", ...)` call looks like it relies on a
formatting constructor; Commons Compress’ `ArchiveException` typically only
accepts a message (and optionally a cause). Suggested fix: either (1) make
`setMode` throw `IOException` (or wrap into `IOException`) so it aligns with
the reader methods’ signatures, and format the message explicitly, or (2) catch
`ArchiveException` at the call sites and wrap/translate to `IOException`. Also
preserve the original `IllegalArgumentException` as the cause when rethrowing
so diagnostics aren’t lost.
##########
src/main/java/org/apache/commons/compress/archivers/cpio/CpioArchiveInputStream.java:
##########
@@ -538,6 +532,27 @@ private byte[] readRange(final int len) throws IOException
{
return b;
}
+ /**
+ * Sets the mode of an entry from a header field.
+ * <p>
+ * A mode of 0 is left unset because only the trailer may use it; the
callers check that by name.
+ * </p>
+ *
+ * @param entry The entry to update.
+ * @param mode The mode read from the header.
+ * @throws ArchiveException if the mode names a file type CPIO does not
define.
+ */
+ private void setMode(final CpioArchiveEntry entry, final long mode) throws
ArchiveException {
+ if (CpioUtil.fileType(mode) == 0) {
+ return;
+ }
+ try {
+ entry.setMode(mode);
+ } catch (final IllegalArgumentException e) {
+ throw new ArchiveException("Corrupted CPIO archive: Invalid file
mode 0%s at byte: %,d", Long.toOctalString(mode), getBytesRead());
+ }
+ }
Review Comment:
`setMode(...)` declares `throws ArchiveException` but is called from methods
that (per this diff) declare only `throws IOException`, which will not compile
unless `ArchiveException` is an `IOException` subtype. Additionally, the `new
ArchiveException("... 0%s ... %,d", ...)` call looks like it relies on a
formatting constructor; Commons Compress’ `ArchiveException` typically only
accepts a message (and optionally a cause). Suggested fix: either (1) make
`setMode` throw `IOException` (or wrap into `IOException`) so it aligns with
the reader methods’ signatures, and format the message explicitly, or (2) catch
`ArchiveException` at the call sites and wrap/translate to `IOException`. Also
preserve the original `IllegalArgumentException` as the cause when rethrowing
so diagnostics aren’t lost.
##########
src/test/java/org/apache/commons/compress/archivers/cpio/CpioArchiveInputStreamTest.java:
##########
@@ -175,6 +175,79 @@ void testEndOfFileInEntry_c_namesize_0xFFFFFFFF() throws
Exception {
}
}
+ @Test
+ void testInvalidFileTypeInMode() throws Exception {
+ // c_mode declares file type 0170000, which CPIO does not define
+ // @formatter:off
+ final String header =
+ "070701" + // c_magic
+ "00000001" + // c_ino
+ "0000F000" + // c_mode
+ "00000000" + // c_uid
+ "00000000" + // c_gid
+ "00000001" + // c_nlink
+ "00000000" + // c_mtime
+ "00000000" + // c_filesize
+ "00000000" + // c_devmajor
+ "00000000" + // c_devminor
+ "00000000" + // c_rdevmajor
+ "00000000" + // c_rdevminor
+ "00000002" + // c_namesize
+ "00000000" + // c_check
+ "a\0";
+ // @formatter:on
+ try (CpioArchiveInputStream cpio = CpioArchiveInputStream.builder()
+ .setByteArray(header.getBytes(StandardCharsets.US_ASCII))
+ .get()) {
+ assertThrows(ArchiveException.class, cpio::getNextEntry);
+ }
Review Comment:
The PR description notes `getNextEntry` declares `IOException`; asserting
that it throws a checked `ArchiveException` directly is generally incompatible
with that public API (unless there’s an existing translation layer you’re
relying on). If the intended contract is “I/O API throws `IOException` but
signals archive corruption via message/cause,” consider updating the tests to
assert `IOException` and verify the error message and/or `getCause()` type. If
you truly want `ArchiveException` to surface, the production API likely needs
to be adjusted accordingly (which may not be possible for an override).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
