Bug 61349 - Add more sanity checks for byte[] allocation
Summary: Add more sanity checks for byte[] allocation
Alias: None
Product: POI
Classification: Unclassified
Component: POI Overall (show other bugs)
Version: 3.17-dev
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: POI Developers List
Depends on:
Reported: 2017-07-26 18:50 UTC by Tim Allison
Modified: 2018-01-01 14:59 UTC (History)
0 users


Note You need to log in before you can comment on or make changes to this bug.
Description Tim Allison 2017-07-26 18:50:53 UTC
Now that I've added sanity checks for byte[] allocation in EMF/WMF, fuzzing is finding other areas where we might want to do this -- see stacktrace below.

For EMF/WMF, I set some arbitrary max lengths...should we do this more throughout the codebase to prevent ooms on corrupt files? 

Yet another OOM:

Caused by: java.lang.OutOfMemoryError: Java heap space
	at java.lang.Object.clone(Native Method)
	at org.apache.poi.ddf.EscherComplexProperty.<init>(EscherComplexProperty.java:46)
	at org.apache.poi.ddf.EscherPropertyFactory.createProperties(EscherPropertyFactory.java:69)
	at org.apache.poi.ddf.AbstractEscherOptRecord.fillFields(AbstractEscherOptRecord.java:54)
	at org.apache.poi.ddf.EscherContainerRecord.fillFields(EscherContainerRecord.java:81)
	at org.apache.poi.ddf.EscherContainerRecord.fillFields(EscherContainerRecord.java:81)
	at org.apache.poi.hwpf.model.EscherRecordHolder.fillEscherRecords(EscherRecordHolder.java:56)
	at org.apache.poi.hwpf.model.EscherRecordHolder.<init>(EscherRecordHolder.java:45)
	at org.apache.poi.hwpf.HWPFDocument.<init>(HWPFDocument.java:280)
Comment 1 Tim Allison 2017-09-21 14:56:03 UTC
r1809169 initial commit.

I tried to avoid checks in "serialize()" methods on the theory that the object has already been collected, it should be good.

I also avoided most checks where there was a copy of an existing array.

We'll likely have to increase some of the thresholds, and I look forward to running these mods against our regression sets.
Comment 2 Javen O'Neal 2017-09-21 15:40:09 UTC
Should we be making better use of the transient modifier when allocating performance-related data structures?
Comment 3 Dominik Stadler 2017-09-24 05:58:27 UTC
I ran a regression test run with the new limitations. 

Out of 1.1 million documents only 80 differences occurred in 4.0.0 compared to 3.17.
Out of these 19 were OOMs that probably happened before as well.

Thus only 61 documents fail with the new limit. 

I saw that almost all of them are in the 1-2MB range, a few try to allocate a bit more than 2MB, so if we raise the limit to 2.5MB, we should be safe for almost all documents of this corpus.

See http://people.apache.org/~centic/poi_regression/reportsAll/index317to400SNAPSHOT.html for details.
Comment 4 Tim Allison 2017-09-25 12:53:36 UTC
Wow.  Thank you, Dominik!  I'm surprised there weren't more problems.

In r1809623, I bumped the following to 10MB:


I bumped the following to 100MB:

There are still a few records with some pretty big sizes, but, that's the point of this fix, e.g.: 536,871,012, 2,013,296,702, 1,451,486,230  :)

Thank you, again, Dominik!
Comment 5 Dominik Stadler 2018-01-01 14:59:39 UTC
I think this is mostly fixed now, new items can be handled in separate issues if necessary.