Skip to content

Commit

Permalink
Merge pull request #120 from theobisproject/COMPRESS-542
Browse files Browse the repository at this point in the history 
COMPRESS-542: Lazy allocation of SevenZArchiveEntry to prevent OOM on corrupt files
  • Loading branch information
@PeterAlfredLee
PeterAlfredLee committed Aug 15, 2020
2 parents 870b54c + 41359f5 commit 464ba19
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 41 deletions.
109 changes: 68 additions & 41 deletions src/main/java/org/apache/commons/compress/archivers/sevenz/SevenZFile.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +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;
Expand Down Expand Up @@ -934,10 +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];
for (int i = 0; i < files.length; i++) {
files[i] = new SevenZArchiveEntry();
}
final int numFilesInt = (int) numFiles;
final Map<Integer, SevenZArchiveEntry> fileMap = new HashMap<>();
BitSet isEmptyStream = null;
BitSet isEmptyFile = null;
BitSet isAnti = null;
Expand All @@ -949,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: {
Expand Down Expand Up @@ -980,68 +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) {
files[nextFile++].setName(new String(names, nextName, i-nextName, StandardCharsets.UTF_16LE));
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++) {
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++) {
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++) {
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++) {
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;
Expand Down Expand Up @@ -1070,30 +1081,46 @@ private void readFilesInfo(final ByteBuffer header, final Archive archive) throw
}
int nonEmptyFileCounter = 0;
int emptyFileCounter = 0;
for (int i = 0; i < files.length; i++) {
files[i].setHasStream(isEmptyStream == null || !isEmptyStream.get(i));
if (files[i].hasStream()) {
for (int i = 0; i < numFilesInt; i++) {
final SevenZArchiveEntry entryAtIndex = fileMap.get(i);
if (entryAtIndex == null) {
continue;
}
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;
}
}
archive.files = files;
final List<SevenZArchiveEntry> entries = new ArrayList<>();
for (final SevenZArchiveEntry e : fileMap.values()) {
if (e != null) {
entries.add(e);
}
}
archive.files = entries.toArray(new SevenZArchiveEntry[0]);
calculateStreamMap(archive);
}

private void checkEntryIsInitialized(final Map<Integer, SevenZArchiveEntry> 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();

Expand Down
19 changes: 19 additions & 0 deletions src/test/java/org/apache/commons/compress/archivers/sevenz/SevenZFileTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -708,6 +710,23 @@ public void retrieveInputStreamForAllEntriesWithoutCRCMultipleTimes() throws IOE
}
}

@Test
public void testNoOOMOnCorruptedHeader() throws IOException {
final List<Path> 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);
Expand Down
Binary file added src/test/resources/COMPRESS-542-1.7z
View file
Binary file not shown.
Binary file added src/test/resources/COMPRESS-542-2.7z
View file
Binary file not shown.
Binary file added src/test/resources/COMPRESS-542-endheadercorrupted.7z
View file
Binary file not shown.
Binary file added src/test/resources/COMPRESS-542-endheadercorrupted2.7z
View file
Binary file not shown.

0 comments on commit 464ba19

Please sign in to comment.