signs of int overflow in nsCanvasRenderingContext2D::GetImageData forked from https://bugzilla.mozilla.org/show_bug.cgi?id=353144#c39 testcase seems to overflow probably because of this: nsAutoArrayPtr<jsval> jsvector(new jsval[w * h * 4]); w*h*4 > 1G (gdb) bt #0 0xffffe410 in __kernel_vsyscall () #1 0xb73827b6 in nanosleep () from /lib/tls/libc.so.6 #2 0xb73825df in sleep () from /lib/tls/libc.so.6 #3 0xb7eedbeb in ah_crap_handler (signum=11) at nsSigHandlers.cpp:134 #4 0xb7f06948 in nsProfileLock::FatalSignalHandler (signo=11) at nsProfileLock.cpp:210 #5 <signal handler called> #6 0xb65d8162 in nsCanvasRenderingContext2D::GetImageData (this=0x8fc11a0) at /opt/joro/firefox/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp:2910 (gdb) frame 6 #6 0xb65d8162 in nsCanvasRenderingContext2D::GetImageData (this=0x8fc11a0) at /opt/joro/firefox/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp:2910 2910 *dest++ = INT_TO_JSVAL(b); (gdb) p dest $7 = (jsval *) 0xb1695000 $ grep -i b16950 /proc/1561/maps 715f3000-b1695000 rwxp 715f3000 00:00 0 b1695000-b16b9000 r-xp 00000000 03:08 1541471 /usr/share/fonts/default/Type1/n021023l.pfb (gdb) p (w*h*4)/(1024*1024) $9 = 1024

ffs.. jsval is a 32-bit value, so yeah, this can overflow. I need to write a helper function for these kinds of allocations, but, for now, I'll have a quickie patch shortly.

Fix -- needs review, I don't have a full tree to build this moment but I can do so in about an hour.

Vlad, mconnor tells me that you were thinking this wasn't bad enough to cause a respin on its own, so I'm minusing and adding a whiteboard comment to take it as a ridealong if we do respin the RC. Renominate if you think that it's bad enough to cause a respin on its own.

Comment on attachment 241075 [details] [diff] [review] fix Sorry, didn't mean to steal the review as well

disclaimer: my c++ is lame. i suspect that in some cases of operator new[], g++ stores *somewhere* the number of objects to be allocated (probably to know how many destructors to call on delete[]). *probably* this is in the allocated block. this seems confirmed by a very ugly overload of `operator new`. so i suggest a fix along the lines of: if ( (PRUint32) w*h*4 > (PRUint32) -(1<<20) ) return NS_ERROR_INVALID_ARG;

(In reply to comment #6) oops i meant: if ( (PRUint32) w*h*4 > ((PRUint32) -(1<<20))/sizeof(jsval) ) return NS_ERROR_INVALID_ARG;

brendan, bz: can someone comment how g++'s delete[] know how many destructors to call without storing the number of objects of new[] in the allocated block? last time i read about it was explained as 'magic'.

(In reply to comment #8) > brendan, bz: > > can someone comment how g++'s delete[] know how many destructors to call > without storing the number of objects of new[] in the allocated block? That's a good point. I like your suggested fix -- I'll respin the patch shortly.

on g++ 4 classes without destructors seem safe, but ones with destructors overflow like this: #include <stdio.h> using namespace std; #include <new> class S { int i; public: S() { puts("S::S()"); i=1;} ~S() { puts("S::~S()"); } }; int main() { S *b=NULL; volatile size_t si= (size_t)-4; b=new S[(si)/sizeof(S)]; delete[] b; return(0); }

Checked in to branch and trunk, over-the-shoulder review from mscott and a+ from schrep.

Please, please add some comments explaining the magic here....

from inspection the patch seems to have very low risk of regression, except for the following minor side effect: on 64+ bit systems with a lot of vm, lusers won't be able to use very large objects. doesn't seem a risk at the moment. actually previous checks for |w| and |h| already do this, so this is not limited to the patch.

(In reply to comment #12) > Please, please add some comments explaining the magic here.... > if you are writing about comments in the patch i suspect they were removed on purpose so `bad guys' don't learn some tricks ;) patch 'fix' has nice comments.

> i suspect they were removed on purpose so `bad guys' don't learn some tricks I think that's bullshit, sorry. It's clear what the patch is doing -- preventing overflow. The checkin comment said so. What's not clear is why this particular algorithm is needed to prevent overflow. Simply mentioning the business about array size info being stored in the allocated block by GCC etc would help a lot to keep this code maintainable and wouldn't tell the skript kiddiez anything useful, imo.

With the testcase, I get this with a branch build: Error: uncaught exception: [Exception... "Component returned failure code: 0x8007000e (NS_ERROR_OUT_OF_MEMORY) [nsIDOMCanvasRenderingContext2D.getImageData]" nsresult: "0x8007000e (NS_ERROR_OUT_OF_MEMORY)" location: "JS frame :: https://bugzilla.mozilla.org/attachment.cgi?id=241040 :: v :: line 14" data: no] I get this error with branch builds before and after the patch has been checked in. With the 2006-10-04 trunk build, I still crash. Talkback ID: TB24140179K kernel32.dll + 0x12a5b (0x7c812a5b) MSVCR80.dll + 0x28dd3 (0x60258dd3) MSVCR80.dll + 0x30e59 (0x60260e59) nsCanvasRenderingContext2D::GetImageData [mozilla\content\canvas\src\nscanvasrenderingcontext2d.cpp, line 2874] XPTC_InvokeByIndex [mozilla\xpcom\reflect\xptcall\src\md\win32\xptcinvoke.cpp, line 102] XPCWrappedNative::CallMethod [mozilla\js\src\xpconnect\src\xpcwrappednative.cpp, line 2197] So I'm reopening the bug for that.

(In reply to comment #16) I think this is the new throws on vc8 trunk kernel32.dll!7c812a5b() [Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll] kernel32.dll!7c812a5b() > msvcr80d.dll!_onexit_nolock(int (void)* func=0xe06d7363) Line 157 + 0x9 bytes C msvcr80d.dll!_CxxThrowException(void * pExceptionObject=0x0012dbe8, const _s__ThrowInfo * pThrowInfo=0x103037e4) Line 166 C++ msvcr80d.dll!operator new(unsigned int size=1073872900) Line 64 C++ gklayout.dll!operator new[](unsigned int count=1073872900) Line 7 + 0x9 bytes C++ gklayout.dll!nsCanvasRenderingContext2D::GetImageData() Line 2880 + 0x10 bytes C++

this is different from |new| throwing. but to reproduce it is needed a lot of VM, otherwise new throws or maybe null dereference. tested on linux 2.6 with 1.5G ram and 4G swap. building at the moment, will let you know.

on a 1.8 branch nightly i get NS_ERROR_ILLEGAL value and firefox survives.

fixed on cvs trunk and 1.8 branch: JavaScript error: , line 0: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIDOMCanvasRenderingContext2D.getImageData]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: https://bugzilla.mozilla.org/attachment.cgi?id=241040 :: v :: line 14" data: no] Martijn, i consider this fixed by vlad. if your concerns are about new throwing check: |new| throws in canvas Bug 353144 though the testcases are similar, this is different from just out of memory. if you have a linux box, you may try adding 4G of swap in a file via mkswap(8) and swapon(8)

blocking1.8.0.8 doesn't seem to make sense - 1.8.0.7 does support getImageData

(In reply to comment #21) > blocking1.8.0.8 doesn't seem to make sense - 1.8.0.7 does support getImageData > 1.8.0.7 does NOT support getImageData

(In reply to comment #20) > Martijn, i consider this fixed by vlad. > > if your concerns are about new throwing check: > |new| throws in canvas Bug 353144 Ok, I filed bug 355498, because I still crash with the testcase on trunk windows builds. Bug 353144 doesn't seem related to this.

(In reply to comment #23) > Ok, I filed bug 355498, because I still crash with the testcase on trunk > windows builds. Bug 353144 doesn't seem related to this. ok. can't see the bug so can't comment, but according to my tests this is fixed on linux.

This is not needed on the 1.8.0 branch.

(In reply to comment #12) > Please, please add some comments explaining the magic here.... Sorry, I should have done so; I'll add comments to the trunk at least.