371135 - OOM crash [@ gfxImageSurface::gfxImageSurface ]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[(not currently active) Ted Mielczarek]

kernel32!RaiseException+0x53
MSVCR80!_CxxThrowException(void * pExceptionObject = 0x0012f8c4, struct _s__ThrowInfo * pThrowInfo = 0x781b5764)+0x46 [f:\rtm\vctools\crt_bld\self_x86\crt\prebuild\eh\throw.cpp @ 166]
MSVCR80!operator new(unsigned int size = 0x53578110)+0x69 [f:\rtm\vctools\crt_bld\self_x86\crt\src\new.cpp @ 63]
firefox!gfxImageSurface::gfxImageSurface(gfxASurface::gfxImageFormat format = ImageFormatRGB24 (1), long width = 17940, long height = 19485)+0x30 [c:\tinderbox\winnt_5.1_depend\mozilla\gfx\thebes\src\gfximagesurface.cpp @ 56]
firefox!nsThebesImage::Init(int aWidth = 1, int aHeight = 0, int aDepth = 24, nsMaskRequirements aMaskRequirements = nsMaskRequirements_kNoMask (0))+0xe5 [c:\tinderbox\winnt_5.1_depend\mozilla\gfx\src\thebes\nsthebesimage.cpp @ 121]
firefox!gfxImageFrame::Init(int aX = 0, int aY = 0, int aWidth = 17940, int aHeight = 19485, int aFormat = 0, unsigned short aDepth = 0x18)+0xfa [c:\tinderbox\winnt_5.1_depend\mozilla\gfx\src\shared\gfximageframe.cpp @ 142]
firefox!info_callback(struct png_struct_def * png_ptr = 0x02d49008, struct png_info_struct * info_ptr = 0x01db2ec0)+0x25f [c:\tinderbox\winnt_5.1_depend\mozilla\modules\libpr0n\decoders\png\nspngdecoder.cpp @ 350]
firefox!MOZ_PNG_push_have_info(struct png_struct_def * png_ptr = 0x008e59ca, struct png_info_struct * info_ptr = 0x02d49008)+0x15 [c:\tinderbox\winnt_5.1_depend\mozilla\modules\libimg\png\pngpread.c @ 1531]
firefox!MOZ_PNG_push_read_chunk(struct png_struct_def * png_ptr = 0x02d49008, struct png_info_struct * info_ptr = 0x01db2ec0)+0x268 [c:\tinderbox\winnt_5.1_depend\mozilla\modules\libimg\png\pngpread.c @ 300]
firefox!MOZ_PNG_proc_some_data(struct png_struct_def * png_ptr = 0x6033f186, struct png_info_struct * info_ptr = 0x03b9edc8)+0x3a [c:\tinderbox\winnt_5.1_depend\mozilla\modules\libimg\png\pngpread.c @ 55]

The bonsai URL produces a 17940x19485 PNG, which makes |new| throw an OOM exception.  Looks like the same crash that Ria mentioned in bug 370763 comment 1, but I don't think that bug is appropriate.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I thought for sure there was a bug on this already, but I'm probably just remembering IRC conversations between timeless and stuart.

Comment 2

[Jesse Ruderman]

This sounds like an integer overflow hazard.  "new throws an OOM exception" is what happens if you're *lucky*.

Comment 3

[Daniel Veditz [:dveditz]]

(In reply to comment #1)
> I thought for sure there was a bug on this already,

possibly bug 204143, bug 290284, and/or bug 353144

(In reply to comment #2)
> This sounds like an integer overflow hazard.  "new throws an OOM exception" is
> what happens if you're *lucky*.

17940x19485 isn't quite into the int-overflow range, and nsThebesImage has robust checking for overflow. *This* path isn't going to be anything other than a safe OOM crash.

There are, however, a surprising number of direct callers of gfxImageSurface, surprising to Stuart, anyway, and he's not one you want to surprise when it comes to using his image code.
http://lxr.mozilla.org/mozilla/search?string=new%20gfxImageSurface

Rather than hope all present and future callers do the appropriate checking we should add or move the checks into gfxImageSurface itself rather than trust all of the callers to get it right. nsSVGPatternFrame and nsSVGFilterFrame appear to do insufficient checking, for example.

Without checks directly in gfxImageSurface we also have to make sure all callers of CreateOffscreenSurface do the checks
http://lxr.mozilla.org/mozilla/search?string=CreateOffscreenSurface

One of those, nsCanvasRenderingContext2D, was fixed in bug 348351

gfxPlatform::OptimizeImage is passed already-checked nsThebesImages

I can't really tell with nsThebesDrawingSurface. The Init() that takes an nsIntSize and creates a new gfxImageSurface is probably OK because it just calls NS_ERROR and does nothing on non-gtk2 platforms (like windows) -- we can't be using that one. There's another Init() that calls CreateOffscreenSurface -- I couldn't verify that that one's safe.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #3)
> There are, however, a surprising number of direct callers of gfxImageSurface,
> surprising to Stuart, anyway, and he's not one you want to surprise when it
> comes to using his image code.
> http://lxr.mozilla.org/mozilla/search?string=new%20gfxImageSurface

Shouldn't be that surprising! :)

> Rather than hope all present and future callers do the appropriate checking we
> should add or move the checks into gfxImageSurface itself

Yes, gfxImageSurface should do those checks itself ideally.  The hard thing is what to do in case of OOM there, because it's a constructor.  We can just abort(), as it will lead to a crash somewhere down the line anyway, maybe?

Comment 5

[(not currently active) Ted Mielczarek]

(In reply to comment #4)
> Yes, gfxImageSurface should do those checks itself ideally.  The hard thing is
> what to do in case of OOM there, because it's a constructor.  We can just
> abort(), as it will lead to a crash somewhere down the line anyway, maybe?

Doesn't that turn this into a DoS?  That would suck.
 

Comment 6

[chris hofmann]

pav, any ideas on what to do to fix?

Comment 7

[Stuart Parmenter]

Use malloc?  Allow/catch exceptions?

Comment 8

[Jesse Ruderman]

Per dveditz and pav, this is not a security hole.

Comment 9

[Martijn Wargers (dead)]

Attachment: image

This is also an image that seems to generate this crash on current trunk builds after a few reloads.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: expose surface status and handle OOM and other errors

Ok, how about something like this to fix?  I can't reproduce the crash with the testcase with it, anyway.

Comment 11

[Martijn Wargers (dead)]

Attachment: backtrace

I tried your patch, but for some reason I crash at startup in my mingw debug build after applying it and rebuilding gfx/. I'm not sure why. The backtrace doesn't really seem related, but after I've backed out the patch, I don't crash anymore.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

You need to rebuild most everything, as it adds methods and changes layout of gfx classes...

Comment 13

[Stuart Parmenter]

Comment on attachment 261036 [details] [diff] [review]
expose surface status and handle OOM and other errors

>+    static PRBool CheckSurfaceSize(const gfxIntSize& sz, PRInt32 limit = 0)
>+    static PRBool CheckSurfaceSize(PRInt32 width, PRInt32 height, PRInt32)

Get rid of the 2nd method and I'd move the impl in to gfxASurface.cpp

>+    mData = (unsigned char *) PR_Malloc(mSize.height * mStride);
No real reason to use PR_Malloc -- just use malloc()

>+        PR_Free(mData);
free()


gfxASurface::CairoSurface() should probably assert if mSurface is null since it could never be null before

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Fixed, though because X11 sucks the function is called CairoStatus instead of Status.