Skip to content

Commit 6034506

Browse files
tballisontballison
authored
TIKA-4783 -- improve chm parsing (#2942)
1 parent 99b149c commit 6034506

14 files changed

Lines changed: 289 additions & 49 deletions

File tree

‎tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/chm/ChmCommons.java‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -307,26 +307,27 @@ public static byte[] copyOfRange(byte[] original, int from, int to) throws TikaE
307307
checkCopyOfRangeParams(original, from, to);
308308
int newLength = to - from;
309309
if (newLength < 0) {
310-
throw new IllegalArgumentException(from + " > " + to);
310+
throw new ChmParsingException(from + " > " + to);
311311
}
312312

313313
byte[] copy = new byte[newLength];
314314
System.arraycopy(original, from, copy, 0, Math.min(original.length - from, newLength));
315315
return copy;
316316
}
317317

318-
private static void checkCopyOfRangeParams(byte[] original, int from, int to) {
318+
private static void checkCopyOfRangeParams(byte[] original, int from, int to)
319+
throws ChmParsingException {
319320
if (original == null) {
320-
throw new NullPointerException("array is null");
321+
throw new ChmParsingException("array is null");
321322
}
322323
if (from < 0) {
323-
throw new IllegalArgumentException(from + " should be > 0");
324+
throw new ChmParsingException(from + " should be > 0");
324325
}
325326
if (to < 0) {
326-
throw new IllegalArgumentException(to + " should be > 0");
327+
throw new ChmParsingException(to + " should be > 0");
327328
}
328329
if (to > original.length) {
329-
throw new IllegalArgumentException("can't copy beyond array length");
330+
throw new ChmParsingException("can't copy beyond array length");
330331
}
331332
}
332333

‎tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/chm/ChmDirectoryListingSet.java‎

Lines changed: 30 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818

1919
import static java.nio.charset.StandardCharsets.UTF_8;
2020

21-
import java.math.BigInteger;
2221
import java.util.ArrayList;
2322
import java.util.HashSet;
2423
import java.util.List;
@@ -220,17 +219,12 @@ private void enumerateOneSegment(byte[] dir_chunk) throws ChmParsingException, T
220219
//setPlaceHolder(header_len);
221220
while (placeHolder > 0 && placeHolder < dir_chunk.length - PMGLheader.getFreeSpace()
222221
/*&& dir_chunk[placeHolder - 1] != 115*/) {
223-
//get entry name length
224-
int strlen = 0;// = getEncint(data);
225-
byte temp;
226-
while ((temp = dir_chunk[placeHolder++]) >= 0x80) {
227-
strlen <<= 7;
228-
strlen += temp & 0x7f;
229-
}
230-
231-
strlen = (strlen << 7) + temp & 0x7f;
222+
//get entry name length (variable-length ENCINT). The old inline
223+
//decoder compared a signed byte to 0x80 (never true), so it read a
224+
//single byte and capped the length at 127, corrupting longer names.
225+
int strlen = getEncint(dir_chunk);
232226

233-
if (strlen > dir_chunk.length) {
227+
if (strlen < 0 || strlen > dir_chunk.length) {
234228
throw new ChmParsingException("Bad data of a string length.");
235229
}
236230

@@ -324,22 +318,33 @@ private void enumerateOneSegment(byte[] dir_chunk) throws ChmParsingException, T
324318
* @param data_chunk
325319
* @return
326320
*/
327-
private int getEncint(byte[] data_chunk) {
328-
byte ob;
329-
BigInteger bi = BigInteger.ZERO;
330-
byte[] nb = new byte[1];
331-
332-
if (placeHolder < data_chunk.length) {
333-
while ((ob = data_chunk[placeHolder]) < 0) {
334-
nb[0] = (byte) ((ob & 0x7f));
335-
bi = bi.shiftLeft(7).add(new BigInteger(nb));
336-
setPlaceHolder(placeHolder + 1);
337-
}
338-
nb[0] = (byte) ((ob & 0x7f));
339-
bi = bi.shiftLeft(7).add(new BigInteger(nb));
321+
private int getEncint(byte[] dataChunk) {
322+
int start = placeHolder;
323+
//advance past the continuation bytes (high bit set) and the terminating byte
324+
while (placeHolder < dataChunk.length && dataChunk[placeHolder] < 0) {
325+
setPlaceHolder(placeHolder + 1);
326+
}
327+
if (placeHolder < dataChunk.length) {
340328
setPlaceHolder(placeHolder + 1);
341329
}
342-
return bi.intValue();
330+
return (int) decodeEncint(dataChunk, start);
331+
}
332+
333+
/**
334+
* Decodes a variable-length ENCINT beginning at {@code offset}: each byte
335+
* contributes its low 7 bits (most-significant group first) and the high bit
336+
* signals that another byte follows. Package-private for testing.
337+
*/
338+
static long decodeEncint(byte[] data, int offset) {
339+
long value = 0;
340+
while (offset < data.length && data[offset] < 0) {
341+
value = (value << 7) | (data[offset] & 0x7f);
342+
offset++;
343+
}
344+
if (offset < data.length) {
345+
value = (value << 7) | (data[offset] & 0x7f);
346+
}
347+
return value;
343348
}
344349

345350
/**

‎tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/chm/ChmExtractor.java‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,10 @@ public ChmExtractor(InputStream is) throws TikaException, IOException {
131131
setLzxBlocksCache(new ArrayList<>());
132132

133133
} catch (IOException e) {
134+
//don't swallow: propagate so a broken stream surfaces instead of leaving
135+
//a half-built extractor that NPEs later in ChmParser
134136
LOG.warn("IOException parsing chm file", e);
137+
throw e;
135138
}
136139
}
137140

‎tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/chm/ChmItsfHeader.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -395,10 +395,11 @@ private long unmarshalUInt32(byte[] data, long dest) throws TikaException {
395395
if (4 > getDataRemained()) {
396396
throw new TikaException("4 > dataLenght");
397397
}
398+
//shift the top byte in long arithmetic so a value >= 2^31 stays unsigned
398399
dest = (data[this.getCurrentPlace()] & 0xff) |
399400
(data[this.getCurrentPlace() + 1] & 0xff) << 8 |
400401
(data[this.getCurrentPlace() + 2] & 0xff) << 16 |
401-
(data[this.getCurrentPlace() + 3] & 0xff) << 24;
402+
((long) (data[this.getCurrentPlace() + 3] & 0xff)) << 24;
402403

403404
setDataRemained(this.getDataRemained() - 4);
404405
this.setCurrentPlace(this.getCurrentPlace() + 4);

‎tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/chm/ChmItspHeader.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,10 +166,11 @@ private long unmarshalUInt32(byte[] data, int dataLenght, long dest) throws Tika
166166
if (4 > dataLenght) {
167167
throw new TikaException("4 > dataLenght");
168168
}
169+
//shift the top byte in long arithmetic so a value >= 2^31 stays unsigned
169170
dest = (data[this.getCurrentPlace()] & 0xff) |
170171
(data[this.getCurrentPlace() + 1] & 0xff) << 8 |
171172
(data[this.getCurrentPlace() + 2] & 0xff) << 16 |
172-
(data[this.getCurrentPlace() + 3] & 0xff) << 24;
173+
((long) (data[this.getCurrentPlace() + 3] & 0xff)) << 24;
173174

174175
setDataRemained(this.getDataRemained() - 4);
175176
this.setCurrentPlace(this.getCurrentPlace() + 4);

‎tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/chm/ChmLzxBlock.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -876,10 +876,14 @@ private boolean validateConstructorParams(int blockNumber, byte[] dataSegment, l
876876
} else {
877877
throw new ChmParsingException("data segment should not be null");
878878
}
879-
if (blockLength > 0) {
879+
//cap at MAX_CONTENT_SIZE: the content buffer is (int) blockLength, so a
880+
//blockLength past that either overflows the int (negative allocation) or
881+
//trips the Integer.MAX_VALUE branch in checkLzxBlock. Real blocks are ~32KB.
882+
if (blockLength > 0 && blockLength <= MAX_CONTENT_SIZE) {
880883
++goodParameter;
881884
} else {
882-
throw new ChmParsingException("block length should be more than zero");
885+
throw new ChmParsingException(
886+
"block length should be between 1 and " + MAX_CONTENT_SIZE + ": " + blockLength);
883887
}
884888
return (goodParameter == 3);
885889
}

‎tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/chm/ChmLzxcControlData.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,10 +225,11 @@ private long unmarshalUInt32(byte[] data, long dest) throws ChmParsingException
225225
if (4 > getDataRemained()) {
226226
throw new ChmParsingException("4 > dataLenght");
227227
}
228+
//shift the top byte in long arithmetic so a value >= 2^31 stays unsigned
228229
dest = (data[this.getCurrentPlace()] & 0xff) |
229230
(data[this.getCurrentPlace() + 1] & 0xff) << 8 |
230231
(data[this.getCurrentPlace() + 2] & 0xff) << 16 |
231-
(data[this.getCurrentPlace() + 3] & 0xff) << 24;
232+
((long) (data[this.getCurrentPlace() + 3] & 0xff)) << 24;
232233

233234
setDataRemained(this.getDataRemained() - 4);
234235
this.setCurrentPlace(this.getCurrentPlace() + 4);

‎tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/chm/ChmLzxcResetTable.java‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,9 +110,16 @@ private long[] enumerateBlockAddresses(byte[] data) throws TikaException {
110110
setBlockCount(getDataRemained() / 8);
111111
}
112112

113-
long[] addresses = new long[(int) getBlockCount()];
113+
int count = (int) getBlockCount();
114+
if (count < 0) {
115+
throw new ChmParsingException("negative chm reset table block count: " + count);
116+
}
117+
long[] addresses = new long[count];
114118
int rem = getDataRemained() / 8;
115-
for (int i = 0; i < rem; i++) {
119+
//only read as many addresses as both the block count and the trailing data
120+
//allow, so a block count smaller than the data can't write past the array
121+
int toRead = Math.min(count, rem);
122+
for (int i = 0; i < toRead; i++) {
116123
long num = -1;
117124

118125
try {
@@ -144,10 +151,11 @@ private boolean validateParamaters(byte[] data, ChmLzxcResetTable chmLzxcResetTa
144151

145152
private long unmarshalUInt32(byte[] data, long dest) throws TikaException {
146153
ChmAssert.assertByteArrayNotNull(data);
154+
//shift the top byte in long arithmetic so a value >= 2^31 stays unsigned
147155
dest = (data[this.getCurrentPlace()] & 0xff) |
148156
(data[this.getCurrentPlace() + 1] & 0xff) << 8 |
149157
(data[this.getCurrentPlace() + 2] & 0xff) << 16 |
150-
(data[this.getCurrentPlace() + 3] & 0xff) << 24;
158+
((long) (data[this.getCurrentPlace() + 3] & 0xff)) << 24;
151159

152160
setDataRemained(this.getDataRemained() - 4);
153161
this.setCurrentPlace(this.getCurrentPlace() + 4);

‎tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/chm/ChmPmgiHeader.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,10 +91,11 @@ private long unmarshalUInt32(byte[] data, long dest) throws ChmParsingException
9191
if (4 > getDataRemained()) {
9292
throw new ChmParsingException("4 > dataLenght");
9393
}
94+
//shift the top byte in long arithmetic so a value >= 2^31 stays unsigned
9495
dest = (data[this.getCurrentPlace()] & 0xff) |
9596
(data[this.getCurrentPlace() + 1] & 0xff) << 8 |
9697
(data[this.getCurrentPlace() + 2] & 0xff) << 16 |
97-
(data[this.getCurrentPlace() + 3] & 0xff) << 24;
98+
((long) (data[this.getCurrentPlace() + 3] & 0xff)) << 24;
9899

99100
setDataRemained(this.getDataRemained() - 4);
100101
this.setCurrentPlace(this.getCurrentPlace() + 4);

‎tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-microsoft-module/src/main/java/org/apache/tika/parser/microsoft/chm/ChmPmglHeader.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,10 +133,11 @@ private long unmarshalUInt32(byte[] data) throws ChmParsingException {
133133
if (4 > getDataRemained()) {
134134
throw new ChmParsingException("4 > dataLenght");
135135
}
136+
//shift the top byte in long arithmetic so a value >= 2^31 stays unsigned
136137
dest = (data[this.getCurrentPlace()] & 0xff) |
137138
(data[this.getCurrentPlace() + 1] & 0xff) << 8 |
138139
(data[this.getCurrentPlace() + 2] & 0xff) << 16 |
139-
(data[this.getCurrentPlace() + 3] & 0xff) << 24;
140+
((long) (data[this.getCurrentPlace() + 3] & 0xff)) << 24;
140141

141142
setDataRemained(this.getDataRemained() - 4);
142143
this.setCurrentPlace(this.getCurrentPlace() + 4);

0 commit comments

Comments
 (0)