garydgregory commented on code in PR #715:
URL: https://github.com/apache/commons-compress/pull/715#discussion_r2388694592
##########
src/main/java/org/apache/commons/compress/utils/ArchiveUtils.java:
##########
@@ -261,6 +262,23 @@ public static String toString(final ArchiveEntry entry) {
return sb.toString();
}
+ /**
+ * Checks that the specified range is within the bounds of an array of the
specified length.
+ *
+ * @param b the array
+ * @param off the starting offset of the range
+ * @param len the length of the range
+ * @throws IndexOutOfBoundsException if {@code off} is negative, or {@code
len} is negative, or {@code off + len} is greater than {@code arrayLength}
+ * @since 1.29.0
+ */
+ public static void checkFromIndexSize(final byte[] b, final int off, final
int len) {
Review Comment:
Let's not add this as a public method since it is planned to be in Commons
IO https://github.com/apache/commons-io/pull/790
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveInputStream.java:
##########
@@ -532,11 +513,20 @@ public TarArchiveEntry getNextEntry() throws IOException {
if (currEntry.isDirectory() && !currEntry.getName().endsWith("/")) {
currEntry.setName(currEntry.getName() + "/");
}
- // Update entry size in case it changed due to PAX headers
- entrySize = currEntry.getSize();
return currEntry;
}
+ private void afterRead(final int read) throws IOException {
+ // Count the bytes read
+ count(read);
+ // Check for truncated entries
+ if (read == -1 && entryOffset < currEntry.getSize()) {
+ throw new EOFException(String.format("Truncated TAR archive: entry
'%s' expected %d bytes, but got %d", currEntry.getName(), currEntry.getSize(),
Review Comment:
Use `%,d` instead of `%d` to use the locale-dependent thousand separator.
Use `actual` instead of `but got` which makes the vocabulary parallel unit
tests. I've been trying to be consitent on this elsewhere in Commons.
FWIW, I agree here that this is an actual EOF since we read a -1.
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveInputStream.java:
##########
@@ -820,37 +731,15 @@ public long skip(final long n) throws IOException {
* @throws IOException if a truncated tar archive is detected.
*/
private void skipRecordPadding() throws IOException {
- if (!isDirectory() && this.entrySize > 0 && this.entrySize %
getRecordSize() != 0) {
- final long available = in.available();
- final long numRecords = this.entrySize / getRecordSize() + 1;
- final long padding = numRecords * getRecordSize() - this.entrySize;
- long skipped = IOUtils.skip(in, padding);
- skipped = getActuallySkipped(available, skipped, padding);
+ final long entrySize = currEntry != null ? currEntry.getSize() : 0;
+ if (!isDirectory() && entrySize > 0 && entrySize % getRecordSize() !=
0) {
+ final long padding = getRecordSize() - (entrySize %
getRecordSize());
+ final long skipped = org.apache.commons.io.IOUtils.skip(in,
padding);
count(skipped);
- }
- }
-
- /**
- * Skip n bytes from current input stream, if the current input stream
doesn't have enough data to skip, jump to the next input stream and skip the
rest
- * bytes, keep doing this until total n bytes are skipped or the input
streams are all skipped
- *
- * @param n bytes of data to skip.
- * @return actual bytes of data skipped.
- * @throws IOException if an I/O error occurs.
- */
- private long skipSparse(final long n) throws IOException {
- if (sparseInputStreams == null || sparseInputStreams.isEmpty()) {
- return in.skip(n);
- }
- long bytesSkipped = 0;
- while (bytesSkipped < n && currentSparseInputStreamIndex <
sparseInputStreams.size()) {
- final InputStream currentInputStream =
sparseInputStreams.get(currentSparseInputStreamIndex);
- bytesSkipped += currentInputStream.skip(n - bytesSkipped);
- if (bytesSkipped < n) {
- currentSparseInputStreamIndex++;
+ if (skipped != padding) {
+ throw new EOFException(String.format("Truncated TAR archive:
failed to skip record padding for entry '%s'", currEntry.getName()));
Review Comment:
This looks to me like we didn't read a -1 so it might be better as an
`ArchiveException`.
##########
src/main/java/org/apache/commons/compress/archivers/tar/ComposedTarInputStream.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.archivers.tar;
+
+import java.io.EOFException;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Iterator;
+
+import org.apache.commons.compress.utils.ArchiveUtils;
+
+final class ComposedTarInputStream extends InputStream {
+
+ final Iterator<? extends InputStream> streams;
+ private final long size;
+ private InputStream current;
+ private long position;
+
+ ComposedTarInputStream(final Iterable<? extends InputStream> streams,
final long size) {
+ this.streams = streams.iterator();
+ this.size = size;
+ this.current = this.streams.hasNext() ? this.streams.next() : null;
+ this.position = 0;
Review Comment:
Don't initialize instance variables to their default values. But why not use
(or extend) `java.io.SequenceInputStream`?
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveInputStream.java:
##########
@@ -797,21 +718,11 @@ public long skip(final long n) throws IOException {
if (n <= 0 || isDirectory()) {
return 0;
}
- final long availableOfInputStream = in.available();
- final long available = currEntry.getRealSize() - entryOffset;
- final long numToSkip = Math.min(n, available);
- long skipped;
- if (!currEntry.isSparse()) {
- skipped = IOUtils.skip(in, numToSkip);
- // for non-sparse entry, we should get the bytes actually skipped
bytes along with
- // inputStream.available() if inputStream is instance of
FileInputStream
- skipped = getActuallySkipped(availableOfInputStream, skipped,
numToSkip);
- } else {
- skipped = skipSparse(numToSkip);
+ if (currEntry == null || currentInputStream == null) {
+ throw new IllegalStateException("No current tar entry");
Review Comment:
Is this indicative of a programming error (which grants the use of ISE) or
an archive format exception, in which case an `ArchiveException` would be
justified.
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java:
##########
@@ -57,85 +61,30 @@ private final class BoundedTarEntryInputStream extends
BoundedArchiveInputStream
private final TarArchiveEntry entry;
- private long entryOffset;
-
- private int currentSparseInputStreamIndex;
-
BoundedTarEntryInputStream(final TarArchiveEntry entry, final
SeekableByteChannel channel) throws IOException {
- super(entry.getDataOffset(), entry.getRealSize());
- if (channel.size() - entry.getSize() < entry.getDataOffset()) {
- throw new ArchiveException("Entry size exceeds archive size");
- }
+ super(entry.getDataOffset(), entry.getSize());
this.entry = entry;
this.channel = channel;
}
@Override
protected int read(final long pos, final ByteBuffer buf) throws
IOException {
- if (entryOffset >= entry.getRealSize()) {
- return -1;
- }
- final int totalRead;
- if (entry.isSparse()) {
- totalRead = readSparse(entryOffset, buf, buf.limit());
- } else {
- totalRead = readArchive(pos, buf);
- }
+ requireNonNull(buf, "ByteBuffer");
+ // The caller ensures that [pos, pos + buf.remaining()] is within
[start, end]
+ channel.position(pos);
+ final int totalRead = channel.read(buf);
if (totalRead == -1) {
- if (buf.array().length > 0) {
- throw new ArchiveException("Truncated TAR archive");
+ if (buf.remaining() > 0) {
+ throw new EOFException(String.format("Truncated TAR
archive: expected at least %d bytes, but got only %d bytes",
Review Comment:
FWIW, I agree here that this is an actual EOF since we read a -1.
--
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]
