395585 - OOM problems in nsPNGEncoder.cpp

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Mats Palmgren (inactive)]

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)]

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)]

Attachment: 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]]

Comment on attachment 565621 [details] [diff] [review]
null the imagebuffer pointer then we free it

Looks good.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

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)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b79549daab38

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b79549daab38