Last Comment Bug 395585 - OOM problems in nsPNGEncoder.cpp
: OOM problems in nsPNGEncoder.cpp
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Benoit Jacob [:bjacob] (mostly away)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-09-09 10:12 PDT by Mats Palmgren (:mats)
Modified: 2011-10-14 03:59 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
null the imagebuffer pointer then we free it (958 bytes, patch)
2011-10-07 13:05 PDT, Benoit Jacob [:bjacob] (mostly away)
netzen: review+
Details | Diff | Review

Description Mats Palmgren (:mats) 2007-09-09 10:12:50 PDT
A couple of places we should check if 'new' fails:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/modules/libpr0n/encoders/png/nsPNGEncoder.cpp&rev=1.5&root=/cvsroot&mark=266,257#255

I think the error handling here is insufficient:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/modules/libpr0n/encoders/png/nsPNGEncoder.cpp&rev=1.5&root=/cvsroot&mark=574#561

If we leave 'that->mImageBuffer' non-null I think other code (the destructor,
Close()) might free it twice, and other methods use "mImageBuffer != nsnull"
to check if it's valid...
We should set it to nsnull after PR_Free?
Comment 1 Benoit Jacob [:bjacob] (mostly away) 2011-10-07 12:48:26 PDT
operator new is now explicitly infallible,

  https://developer.mozilla.org/en/Infallible_memory_allocation

so the first issue here is fixed.

The second issue seems to still be a real possible bug, so let me make a 1-line patch...
Comment 2 Benoit Jacob [:bjacob] (mostly away) 2011-10-07 13:05:28 PDT
Created attachment 565621 [details] [diff] [review]
null the imagebuffer pointer then we free it

Brian seems to be the last to have touched this file -> asking for review.
Comment 3 Brian R. Bondy [:bbondy] 2011-10-07 19:27:51 PDT
Comment on attachment 565621 [details] [diff] [review]
null the imagebuffer pointer then we free it

Looks good.
Comment 4 Benoit Jacob [:bjacob] (mostly away) 2011-10-11 14:48:08 PDT
Maybe we need to do a post-mortem here. This bug had 'OOM' in its title. OOM bugs are probably a lot more likely than the average to be security sensitive. Can we have automatic queries to make sure that at least bugs explicitly mentioning OOM issues don't fall off the radar?
Comment 5 Benoit Jacob [:bjacob] (mostly away) 2011-10-13 05:12:13 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/b79549daab38
Comment 6 Ed Morley [:emorley] 2011-10-14 03:59:07 PDT
https://hg.mozilla.org/mozilla-central/rev/b79549daab38

Note You need to log in before you can comment on or make changes to this bug.