From 80e5a46d4fe0bd9ff4700b7adddc7cc4b277809d Mon Sep 17 00:00:00 2001 From: theobisproject Date: Mon, 27 Jul 2020 15:21:57 +0200 Subject: [PATCH 1/2] COMPRESS-542: Lazy allocation of SevenZArchiveEntry to prevent OOM on corrupt files --- .../compress/archivers/sevenz/SevenZFile.java | 36 +++++++++++++++---- 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java b/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java index 9722764921..b3283ef09d 100644 --- a/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java +++ b/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java @@ -38,6 +38,7 @@ import java.util.BitSet; import java.util.EnumSet; import java.util.LinkedList; +import java.util.List; import java.util.zip.CRC32; import org.apache.commons.compress.utils.BoundedInputStream; @@ -934,10 +935,7 @@ private BitSet readBits(final ByteBuffer header, final int size) throws IOExcept private void readFilesInfo(final ByteBuffer header, final Archive archive) throws IOException { final long numFiles = readUint64(header); assertFitsIntoInt("numFiles", numFiles); - final SevenZArchiveEntry[] files = new SevenZArchiveEntry[(int)numFiles]; - for (int i = 0; i < files.length; i++) { - files[i] = new SevenZArchiveEntry(); - } + final SevenZArchiveEntry[] files = new SevenZArchiveEntry[(int) numFiles]; BitSet isEmptyStream = null; BitSet isEmptyFile = null; BitSet isAnti = null; @@ -981,8 +979,13 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw int nextName = 0; for (int i = 0; i < names.length; i += 2) { if (names[i] == 0 && names[i+1] == 0) { - files[nextFile++].setName(new String(names, nextName, i-nextName, StandardCharsets.UTF_16LE)); + String fName = new String(names, nextName, i - nextName, StandardCharsets.UTF_16LE); + if (files[nextFile] == null) { + files[nextFile] = new SevenZArchiveEntry(); + } + files[nextFile].setName(fName); nextName = i + 2; + nextFile++; } } if (nextName != names.length || nextFile != files.length) { @@ -997,6 +1000,9 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw throw new IOException("Unimplemented"); } for (int i = 0; i < files.length; i++) { + if (files[i] == null) { + files[i] = new SevenZArchiveEntry(); + } files[i].setHasCreationDate(timesDefined.get(i)); if (files[i].getHasCreationDate()) { files[i].setCreationDate(header.getLong()); @@ -1011,6 +1017,9 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw throw new IOException("Unimplemented"); } for (int i = 0; i < files.length; i++) { + if (files[i] == null) { + files[i] = new SevenZArchiveEntry(); + } files[i].setHasAccessDate(timesDefined.get(i)); if (files[i].getHasAccessDate()) { files[i].setAccessDate(header.getLong()); @@ -1025,6 +1034,9 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw throw new IOException("Unimplemented"); } for (int i = 0; i < files.length; i++) { + if (files[i] == null) { + files[i] = new SevenZArchiveEntry(); + } files[i].setHasLastModifiedDate(timesDefined.get(i)); if (files[i].getHasLastModifiedDate()) { files[i].setLastModifiedDate(header.getLong()); @@ -1039,6 +1051,9 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw throw new IOException("Unimplemented"); } for (int i = 0; i < files.length; i++) { + if (files[i] == null) { + files[i] = new SevenZArchiveEntry(); + } files[i].setHasWindowsAttributes(attributesDefined.get(i)); if (files[i].getHasWindowsAttributes()) { files[i].setWindowsAttributes(header.getInt()); @@ -1071,6 +1086,9 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw int nonEmptyFileCounter = 0; int emptyFileCounter = 0; for (int i = 0; i < files.length; i++) { + if (files[i] == null) { + continue; + } files[i].setHasStream(isEmptyStream == null || !isEmptyStream.get(i)); if (files[i].hasStream()) { if (archive.subStreamsInfo == null) { @@ -1090,7 +1108,13 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw ++emptyFileCounter; } } - archive.files = files; + List entries = new ArrayList<>(); + for (SevenZArchiveEntry e : files) { + if (e != null) { + entries.add(e); + } + } + archive.files = entries.toArray(new SevenZArchiveEntry[0]); calculateStreamMap(archive); } From 41359f56e62d41ed59493cdcbaa5d52d0f89fbb9 Mon Sep 17 00:00:00 2001 From: theobisproject Date: Thu, 13 Aug 2020 19:43:13 +0200 Subject: [PATCH 2/2] COMPRESS-542: Prevent OOM at array creation --- .../compress/archivers/sevenz/SevenZFile.java | 115 +++++++++--------- .../archivers/sevenz/SevenZFileTest.java | 19 +++ src/test/resources/COMPRESS-542-1.7z | Bin 0 -> 102 bytes src/test/resources/COMPRESS-542-2.7z | Bin 0 -> 165 bytes .../COMPRESS-542-endheadercorrupted.7z | Bin 0 -> 233 bytes .../COMPRESS-542-endheadercorrupted2.7z | Bin 0 -> 233 bytes 6 files changed, 78 insertions(+), 56 deletions(-) create mode 100644 src/test/resources/COMPRESS-542-1.7z create mode 100644 src/test/resources/COMPRESS-542-2.7z create mode 100644 src/test/resources/COMPRESS-542-endheadercorrupted.7z create mode 100644 src/test/resources/COMPRESS-542-endheadercorrupted2.7z diff --git a/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java b/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java index b3283ef09d..fd0fe0af82 100644 --- a/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java +++ b/src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java @@ -37,8 +37,10 @@ import java.util.Arrays; import java.util.BitSet; import java.util.EnumSet; +import java.util.HashMap; import java.util.LinkedList; import java.util.List; +import java.util.Map; import java.util.zip.CRC32; import org.apache.commons.compress.utils.BoundedInputStream; @@ -935,7 +937,8 @@ private BitSet readBits(final ByteBuffer header, final int size) throws IOExcept private void readFilesInfo(final ByteBuffer header, final Archive archive) throws IOException { final long numFiles = readUint64(header); assertFitsIntoInt("numFiles", numFiles); - final SevenZArchiveEntry[] files = new SevenZArchiveEntry[(int) numFiles]; + final int numFilesInt = (int) numFiles; + final Map fileMap = new HashMap<>(); BitSet isEmptyStream = null; BitSet isEmptyFile = null; BitSet isAnti = null; @@ -947,7 +950,7 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw final long size = readUint64(header); switch (propertyType) { case NID.kEmptyStream: { - isEmptyStream = readBits(header, files.length); + isEmptyStream = readBits(header, numFilesInt); break; } case NID.kEmptyFile: { @@ -978,85 +981,78 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw int nextFile = 0; int nextName = 0; for (int i = 0; i < names.length; i += 2) { - if (names[i] == 0 && names[i+1] == 0) { - String fName = new String(names, nextName, i - nextName, StandardCharsets.UTF_16LE); - if (files[nextFile] == null) { - files[nextFile] = new SevenZArchiveEntry(); - } - files[nextFile].setName(fName); + if (names[i] == 0 && names[i + 1] == 0) { + checkEntryIsInitialized(fileMap, nextFile); + fileMap.get(nextFile).setName(new String(names, nextName, i - nextName, StandardCharsets.UTF_16LE)); nextName = i + 2; nextFile++; } } - if (nextName != names.length || nextFile != files.length) { + if (nextName != names.length || nextFile != numFiles) { throw new IOException("Error parsing file names"); } break; } case NID.kCTime: { - final BitSet timesDefined = readAllOrBits(header, files.length); + final BitSet timesDefined = readAllOrBits(header, numFilesInt); final int external = getUnsignedByte(header); if (external != 0) { throw new IOException("Unimplemented"); } - for (int i = 0; i < files.length; i++) { - if (files[i] == null) { - files[i] = new SevenZArchiveEntry(); - } - files[i].setHasCreationDate(timesDefined.get(i)); - if (files[i].getHasCreationDate()) { - files[i].setCreationDate(header.getLong()); + for (int i = 0; i < numFilesInt; i++) { + checkEntryIsInitialized(fileMap, i); + final SevenZArchiveEntry entryAtIndex = fileMap.get(i); + entryAtIndex.setHasCreationDate(timesDefined.get(i)); + if (entryAtIndex.getHasCreationDate()) { + entryAtIndex.setCreationDate(header.getLong()); } } break; } case NID.kATime: { - final BitSet timesDefined = readAllOrBits(header, files.length); + final BitSet timesDefined = readAllOrBits(header, numFilesInt); final int external = getUnsignedByte(header); if (external != 0) { throw new IOException("Unimplemented"); } - for (int i = 0; i < files.length; i++) { - if (files[i] == null) { - files[i] = new SevenZArchiveEntry(); - } - files[i].setHasAccessDate(timesDefined.get(i)); - if (files[i].getHasAccessDate()) { - files[i].setAccessDate(header.getLong()); + for (int i = 0; i < numFilesInt; i++) { + checkEntryIsInitialized(fileMap, i); + final SevenZArchiveEntry entryAtIndex = fileMap.get(i); + entryAtIndex.setHasAccessDate(timesDefined.get(i)); + if (entryAtIndex.getHasAccessDate()) { + entryAtIndex.setAccessDate(header.getLong()); } } break; } case NID.kMTime: { - final BitSet timesDefined = readAllOrBits(header, files.length); + final BitSet timesDefined = readAllOrBits(header, numFilesInt); final int external = getUnsignedByte(header); if (external != 0) { throw new IOException("Unimplemented"); } - for (int i = 0; i < files.length; i++) { - if (files[i] == null) { - files[i] = new SevenZArchiveEntry(); - } - files[i].setHasLastModifiedDate(timesDefined.get(i)); - if (files[i].getHasLastModifiedDate()) { - files[i].setLastModifiedDate(header.getLong()); + for (int i = 0; i < numFilesInt; i++) { + checkEntryIsInitialized(fileMap, i); + final SevenZArchiveEntry entryAtIndex = fileMap.get(i); + entryAtIndex.setHasLastModifiedDate(timesDefined.get(i)); + if (entryAtIndex.getHasLastModifiedDate()) { + entryAtIndex.setLastModifiedDate(header.getLong()); } } break; } case NID.kWinAttributes: { - final BitSet attributesDefined = readAllOrBits(header, files.length); + final BitSet attributesDefined = readAllOrBits(header, numFilesInt); final int external = getUnsignedByte(header); if (external != 0) { throw new IOException("Unimplemented"); } - for (int i = 0; i < files.length; i++) { - if (files[i] == null) { - files[i] = new SevenZArchiveEntry(); - } - files[i].setHasWindowsAttributes(attributesDefined.get(i)); - if (files[i].getHasWindowsAttributes()) { - files[i].setWindowsAttributes(header.getInt()); + for (int i = 0; i < numFilesInt; i++) { + checkEntryIsInitialized(fileMap, i); + final SevenZArchiveEntry entryAtIndex = fileMap.get(i); + entryAtIndex.setHasWindowsAttributes(attributesDefined.get(i)); + if (entryAtIndex.getHasWindowsAttributes()) { + entryAtIndex.setWindowsAttributes(header.getInt()); } } break; @@ -1085,31 +1081,32 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw } int nonEmptyFileCounter = 0; int emptyFileCounter = 0; - for (int i = 0; i < files.length; i++) { - if (files[i] == null) { + for (int i = 0; i < numFilesInt; i++) { + final SevenZArchiveEntry entryAtIndex = fileMap.get(i); + if (entryAtIndex == null) { continue; } - files[i].setHasStream(isEmptyStream == null || !isEmptyStream.get(i)); - if (files[i].hasStream()) { + entryAtIndex.setHasStream(isEmptyStream == null || !isEmptyStream.get(i)); + if (entryAtIndex.hasStream()) { if (archive.subStreamsInfo == null) { throw new IOException("Archive contains file with streams but no subStreamsInfo"); } - files[i].setDirectory(false); - files[i].setAntiItem(false); - files[i].setHasCrc(archive.subStreamsInfo.hasCrc.get(nonEmptyFileCounter)); - files[i].setCrcValue(archive.subStreamsInfo.crcs[nonEmptyFileCounter]); - files[i].setSize(archive.subStreamsInfo.unpackSizes[nonEmptyFileCounter]); + entryAtIndex.setDirectory(false); + entryAtIndex.setAntiItem(false); + entryAtIndex.setHasCrc(archive.subStreamsInfo.hasCrc.get(nonEmptyFileCounter)); + entryAtIndex.setCrcValue(archive.subStreamsInfo.crcs[nonEmptyFileCounter]); + entryAtIndex.setSize(archive.subStreamsInfo.unpackSizes[nonEmptyFileCounter]); ++nonEmptyFileCounter; } else { - files[i].setDirectory(isEmptyFile == null || !isEmptyFile.get(emptyFileCounter)); - files[i].setAntiItem(isAnti != null && isAnti.get(emptyFileCounter)); - files[i].setHasCrc(false); - files[i].setSize(0); + entryAtIndex.setDirectory(isEmptyFile == null || !isEmptyFile.get(emptyFileCounter)); + entryAtIndex.setAntiItem(isAnti != null && isAnti.get(emptyFileCounter)); + entryAtIndex.setHasCrc(false); + entryAtIndex.setSize(0); ++emptyFileCounter; } } - List entries = new ArrayList<>(); - for (SevenZArchiveEntry e : files) { + final List entries = new ArrayList<>(); + for (final SevenZArchiveEntry e : fileMap.values()) { if (e != null) { entries.add(e); } @@ -1118,6 +1115,12 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw calculateStreamMap(archive); } + private void checkEntryIsInitialized(final Map archiveEntries, final int index) { + if (archiveEntries.get(index) == null) { + archiveEntries.put(index, new SevenZArchiveEntry()); + } + } + private void calculateStreamMap(final Archive archive) throws IOException { final StreamMap streamMap = new StreamMap(); diff --git a/src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZFileTest.java b/src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZFileTest.java index 9b85f750cd..175fdbe426 100644 --- a/src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZFileTest.java +++ b/src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZFileTest.java @@ -28,9 +28,11 @@ import java.nio.file.Files; import java.nio.file.Path; import java.security.NoSuchAlgorithmException; +import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.Iterator; +import java.util.List; import java.util.Map; import java.util.Random; @@ -708,6 +710,23 @@ public void retrieveInputStreamForAllEntriesWithoutCRCMultipleTimes() throws IOE } } + @Test + public void testNoOOMOnCorruptedHeader() throws IOException { + final List testFiles = new ArrayList<>(); + testFiles.add(getPath("COMPRESS-542-1.7z")); + testFiles.add(getPath("COMPRESS-542-2.7z")); + testFiles.add(getPath("COMPRESS-542-endheadercorrupted.7z")); + testFiles.add(getPath("COMPRESS-542-endheadercorrupted2.7z")); + + for (final Path file : testFiles) { + try (SevenZFile sevenZFile = new SevenZFile(Files.newByteChannel(file))) { + fail("Expected IOException: start header corrupt and unable to guess end header"); + } catch (IOException e) { + assertEquals("Start header corrupt and unable to guess end header", e.getMessage()); + } + } + } + private void test7zUnarchive(final File f, final SevenZMethod m, final byte[] password) throws Exception { try (SevenZFile sevenZFile = new SevenZFile(f, password)) { test7zUnarchive(sevenZFile, m); diff --git a/src/test/resources/COMPRESS-542-1.7z b/src/test/resources/COMPRESS-542-1.7z new file mode 100644 index 0000000000000000000000000000000000000000..b5bd519cd0738d665c2613851ed824025b5309d1 GIT binary patch literal 102 zcmXr7+Ou9=hJi5n2LufN|1&T!0@X4y{BHn~%pfM~|NpEqKyeU&a2&u)mjD0%GlB>K G1_l6CtQB|w literal 0 HcmV?d00001 diff --git a/src/test/resources/COMPRESS-542-2.7z b/src/test/resources/COMPRESS-542-2.7z new file mode 100644 index 0000000000000000000000000000000000000000..8c68e75baf38ba1ccba98ba74f916b13b450298a GIT binary patch literal 165 zcmXr7+Ou9=h5;X7WM%l@z`!c=A4oDVDlh;EsK`Ae5jG_De+Hq46+m@B@E@Y@|NjaH nhW{Xa|3TW-Wf)n1Gb(@-fVm)542%d}ybu`%266HK4&N96d*~m} literal 0 HcmV?d00001 diff --git a/src/test/resources/COMPRESS-542-endheadercorrupted.7z b/src/test/resources/COMPRESS-542-endheadercorrupted.7z new file mode 100644 index 0000000000000000000000000000000000000000..93b3e9d019c4494bd51fcfcbcaa1b8fb15bec530 GIT binary patch literal 233 zcmXr7+Ou9=hJl3v2YA5X$RHcbU?s(wUbn93PEE_fQ;)kgx_$BeWw-YC!C;Ad5nzpt z#_a3(FGNMx%(r)UWNmL->={;k`Kp)Vf|5^}9KPQUb6#ql^7zKa*J8(JpHc6POrDf( zw7po$ZCU`U(BoUzHg(KBdD>slsn=SJRn=>Y00o%{mnsti%YRD zdYq~j+?pfxc1F*w?rSaj9N$%Q7nI!Fw>G2uGlMvrDkEn@69YRpBLkx{Gb1DG`~Uy{ PgPg|2=*q%;i-7?E^U_%j literal 0 HcmV?d00001 diff --git a/src/test/resources/COMPRESS-542-endheadercorrupted2.7z b/src/test/resources/COMPRESS-542-endheadercorrupted2.7z new file mode 100644 index 0000000000000000000000000000000000000000..f21f7c416a73549a399de6b5601b3fe49886d0e4 GIT binary patch literal 233 zcmXr7+Ou9=hJl3v2YA5X$RHcbU?s(wUbn93PEE_fQ;)kgx_$BeWw-YC!C;Ad5nzpt z#_a3(FGNMx%(r)UWNmL->={;k`Kp)Vf|5^}9KPQUb6#ql^7zKa*J8(JpHc6POrDf( zw7po$ZCU`U(BoUzHg(KBdD>slsn=SJRn=>Y00o%{mnsti%YRD zdYq~j+?pfxc1F*w?rSaj9N$%Q7nI!Fw>G2uGlMvrDkEn@69YRpBLkx{Gb1DG2Zq$M QvOu?SF}kua-(p|@0NYnrU;qFB literal 0 HcmV?d00001