garydgregory commented on code in PR #710:
URL: https://github.com/apache/commons-compress/pull/710#discussion_r2379158878
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarUtils.java:
##########
@@ -354,34 +354,44 @@ public static void formatUnsignedOctalString(final long
value, final byte[] buff
*
* @param input the input stream from which to read the special tar entry
content.
* @param encoding the encoding to use for reading names.
+ * @param maxEntryNameLength the maximum allowed length for entry names.
* @param entry the tar entry to handle.
* @param paxHeaders the map to update with PAX headers.
* @param sparseHeaders the list to update with sparse headers.
* @param globalPaxHeaders the map to update with global PAX headers.
* @param globalSparseHeaders the list to update with global sparse
headers.
* @throws IOException if an I/O error occurs while reading the entry.
*/
- static void handleSpecialTarRecord(final InputStream input, final
ZipEncoding encoding, final TarArchiveEntry entry, final Map<String, String>
paxHeaders,
- final List<TarArchiveStructSparse> sparseHeaders, final
Map<String, String> globalPaxHeaders,
- final List<TarArchiveStructSparse> globalSparseHeaders) throws
IOException {
+ static void handleSpecialTarRecord(
Review Comment:
Don't change the formatting please, I don't think any of us are on 80
character TTY terminals 😉
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java:
##########
@@ -65,7 +66,7 @@ private final class BoundedTarEntryInputStream extends
BoundedArchiveInputStream
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");
+ throw new EOFException("Truncated TAR archive: entry size
exceeds archive size");
Review Comment:
I'm not sure about this ... This is not a real EOF. Why mix in this change
in an large PR?
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarUtils.java:
##########
@@ -767,15 +813,24 @@ protected static Map<String, String>
parsePaxHeaders(final InputStream inputStre
final String keyword =
coll.toString(StandardCharsets.UTF_8);
// Get rest of entry
final int restLen = len - read;
+
+ // Validate entry length
+ // 1. Ignore empty keywords
if (restLen <= 1) { // only NL
headers.remove(keyword);
+ // 2. Entry length exceeds header size
} else if (headerSize >= 0 && restLen > headerSize
- totalRead) {
throw new ArchiveException("PAX header value
size %,d exceeds size of header record.", restLen);
} else {
+ // 3. Entry length exceeds configurable file
and link name limits
+ if ("path".equals(keyword) ||
"linkpath".equals(keyword)) {
+ ArchiveUtils.checkEntryNameLength(restLen
- 1, maxEntryPathLength, "TAR");
+ }
final byte[] rest =
IOUtils.readRange(inputStream, restLen);
final int got = rest.length;
if (got != restLen) {
- throw new ArchiveException("Failed to read
PAX header: Expected %,d bytes, read %,d.", restLen, got);
+ throw new EOFException(String.format(
Review Comment:
No need to change the formatting or the exception IMO since this is not a
real EOF.
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarFile.java:
##########
@@ -507,7 +511,14 @@ private TarArchiveEntry getNextTarEntry() throws
IOException {
lastWasSpecial = TarUtils.isSpecialTarRecord(currEntry);
if (lastWasSpecial) {
// Handle PAX, GNU long name, or other special records
- TarUtils.handleSpecialTarRecord(getInputStream(currEntry),
zipEncoding, currEntry, paxHeaders, sparseHeaders, globalPaxHeaders,
Review Comment:
Please don't change the formatting to this newspaper column style.
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarArchiveInputStream.java:
##########
@@ -512,7 +513,15 @@ public TarArchiveEntry getNextEntry() throws IOException {
lastWasSpecial = TarUtils.isSpecialTarRecord(currEntry);
if (lastWasSpecial) {
// Handle PAX, GNU long name, or other special records
- TarUtils.handleSpecialTarRecord(this, zipEncoding, currEntry,
paxHeaders, sparseHeaders, globalPaxHeaders, globalSparseHeaders);
+ TarUtils.handleSpecialTarRecord(
Review Comment:
Don't change the formatting please, I don't think any of us are on 80
character TTY terminals 😉
##########
src/main/java/org/apache/commons/compress/archivers/ar/ArArchiveInputStream.java:
##########
@@ -244,7 +246,8 @@ public void close() throws IOException {
* @since 1.3
*/
private String getBSDLongName(final String bsdLongName) throws IOException
{
- final int nameLen =
ParsingUtils.parseIntValue(bsdLongName.substring(BSD_LONGNAME_PREFIX_LEN));
+ final int nameLen =
Review Comment:
This formatting is awkward, put it all on one line. Our Checkstyle rules
allows for lines up to 160.
##########
src/main/java/org/apache/commons/compress/archivers/AbstractArchiveBuilder.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * 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;
+
+import org.apache.commons.io.build.AbstractStreamBuilder;
+
+/**
+ * Base builder for all archive formats.
Review Comment:
Nice idea to extract this into a class that's reused from streams and
non-streams.
"Base builder for all archive formats." -> "Abstracts the builder pattern
for all implementations that need to implement a maximum file name length."
##########
src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java:
##########
@@ -677,14 +680,23 @@ public SevenZFile(final SeekableByteChannel channel,
final String fileName, fina
this(channel, fileName, password, false, SevenZFileOptions.DEFAULT);
}
- private SevenZFile(final SeekableByteChannel channel, final String
fileName, final byte[] password, final boolean closeOnError, final int
maxMemoryLimitKiB,
- final boolean useDefaultNameForUnnamedEntries, final boolean
tryToRecoverBrokenArchives) throws IOException {
+ private SevenZFile(
Review Comment:
Don't change the formatting please, I don't think any of us are on 80
character TTY terminals 😉
##########
src/main/java/org/apache/commons/compress/archivers/tar/TarUtils.java:
##########
@@ -767,15 +813,24 @@ protected static Map<String, String>
parsePaxHeaders(final InputStream inputStre
final String keyword =
coll.toString(StandardCharsets.UTF_8);
// Get rest of entry
final int restLen = len - read;
+
+ // Validate entry length
+ // 1. Ignore empty keywords
if (restLen <= 1) { // only NL
headers.remove(keyword);
+ // 2. Entry length exceeds header size
} else if (headerSize >= 0 && restLen > headerSize
- totalRead) {
throw new ArchiveException("PAX header value
size %,d exceeds size of header record.", restLen);
} else {
+ // 3. Entry length exceeds configurable file
and link name limits
+ if ("path".equals(keyword) ||
"linkpath".equals(keyword)) {
Review Comment:
It might be worth extracting these magic strings into private constants.
--
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]
