[Bug 61349] New: Add more sanity checks for byte[] allocation

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[Bug 61349] New: Add more sanity checks for byte[] allocation

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61349

            Bug ID: 61349
           Summary: Add more sanity checks for byte[] allocation
           Product: POI
           Version: 3.17-dev
          Hardware: PC
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: POI Overall
          Assignee: [hidden email]
          Reporter: [hidden email]
  Target Milestone: ---

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)

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61349] Add more sanity checks for byte[] allocation

Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61349

Dominik Stadler <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 OS|                            |All

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61349] Add more sanity checks for byte[] allocation

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61349

--- Comment #1 from Tim Allison <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61349] Add more sanity checks for byte[] allocation

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61349

--- Comment #2 from Javen O'Neal <[hidden email]> ---
Should we be making better use of the transient modifier when allocating
performance-related data structures?

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61349] Add more sanity checks for byte[] allocation

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61349

--- Comment #3 from Dominik Stadler <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[Bug 61349] Add more sanity checks for byte[] allocation

Bugzilla from bugzilla@apache.org
In reply to this post by Bugzilla from bugzilla@apache.org
https://bz.apache.org/bugzilla/show_bug.cgi?id=61349

--- Comment #4 from Tim Allison <[hidden email]> ---
Wow.  Thank you, Dominik!  I'm surprised there weren't more problems.

In r1809623, I bumped the following to 10MB:

PPDrawing
PPDrawingGroup
PlexOfCps
ExOleObjStg
ListLevel
SoundData

I bumped the following to 100MB:
EscherBlipRecord

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!

--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]