|new| throws in canvas this may be not security sensitive, so feel free to remove the flag. mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp:2887 2882 2883 surfaceDataStride = w * 4; 2884 surfaceDataOffset = 0; 2885 } 2886 2887 nsAutoArrayPtr<jsval> jsvector(new jsval[w * h * 4]); 2888 if (!jsvector) [B] 2889 return NS_ERROR_OUT_OF_MEMORY; [B] is never reached, because |new| |throw|s causing exit. |w| and |h| are luser supplied values - there seveal other similar uses of |new| involving large values. potential solution is using |new (std::nothrow) f3c| another potential solution is redefining/overloading (if possible) global |new| but this kind of scares me. terminate called after throwing an instance of 'std::bad_alloc' what(): St9bad_alloc #13 0xb72b6ef1 in raise () from /lib/tls/libc.so.6 #14 0xb72b883b in abort () from /lib/tls/libc.so.6 #15 0xb7479f81 in __gnu_cxx::__verbose_terminate_handler () from /usr/lib/libstdc++.so.6 #16 0xb74778a5 in __gxx_personality_v0 () from /usr/lib/libstdc++.so.6 #17 0xb74778e2 in std::terminate () from /usr/lib/libstdc++.so.6 #18 0xb7477a4a in __cxa_throw () from /usr/lib/libstdc++.so.6 #19 0xb7477eb1 in operator new () from /usr/lib/libstdc++.so.6 #20 0xb7477f7d in operator new[] () from /usr/lib/libstdc++.so.6 ---Type <return> to continue, or q <return> to quit--- #21 0xb4fcbdd3 in nsCanvasRenderingContext2D::GetImageData (this=0x897d990) at /opt/joro/firefox/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp:2887 (gdb) frame 21 #21 0xb4fcbdd3 in nsCanvasRenderingContext2D::GetImageData (this=0x897d990) at /opt/joro/firefox/mozilla/content/canvas/src/nsCanvasRenderingContext2D.cpp:2887 2887 nsAutoArrayPtr<jsval> jsvector(new jsval[w * h * 4]); (gdb) li 2882 2883 surfaceDataStride = w * 4; 2884 surfaceDataOffset = 0; 2885 } 2886 2887 nsAutoArrayPtr<jsval> jsvector(new jsval[w * h * 4]); 2888 if (!jsvector) 2889 return NS_ERROR_OUT_OF_MEMORY; 2890 jsval *dest = jsvector.get(); 2891 PRUint8 *row;

|new| throws in canvas. in bash do 'ulimit -v 200000'

there are other cases of this construct, so a global solution should be sought imho.

mozilla@Weilbacher.org likes writing useless patches to this problem that don't fix anything :).

this isn't security sensitive. it's a checked alloc, bsmedberg is planning to eventually write our own new impl which does what we need (although someone is free to do it sooner).

(In reply to comment #4) > this isn't security sensitive. it's a checked alloc, bsmedberg is planning to > eventually write our own new impl which does what we need (although someone is > free to do it sooner). Where is this plan written of? /be

Merely checking whether the alloc succeeded doesn't protect you from integer overflow bugs...

in no particular order. 1. the usage of the buggy construct is common. 2. (1) is a sign of not very good coding practice and not very good security strategy. 3. similar stuff seems exploitable in exploder cf [A] --------------------------------------------------- [A] http://uninformed.org/index.cgi?v=4&a=5&t=sumry - "Exploiting the Otherwise Non-Exploitable on Windows" This paper describes a technique that can be applied in certain situations to gain arbitrary code execution through software bugs that would not otherwise be exploitable, such as NULL pointer dereferences. To facilitate this, an attacker gains control of the top-level unhandled exception filter for a process in an indirect fashion.

Many integer overflow bugs are exploitable to run arbitrary code. Failed allocations usually just cause null dereferences, but under-allocation is also possible, and that leads to buffer overflows. See http://www.owasp.org/index.php/Integer_overflow.

What is under-allocation and how would it manifest itself?

the scope of this bug is not integer overflow or under allocation. the scope of this bug is trying to alocate *enough but large* amount of memory after checking for overflow. the side effect is uncaught c++ exception resulting in gdb or talkback, making it similar to crash.

suspect there is no overflow below (try commenting |b=|) #include <stdio.h> using namespace std; int main() { char *x="x"; char *a=x; a=new char[2<<31 -1]; char *b=x; b=new char[2<<31 -1]; if(a==NULL || b==NULL) {printf("f4ck\n");return 1;} printf("a=%p %p\n",a,b); return(0); }

so overloading global |new| to |new (std::nothrow)| seems to fix simple testcases in the way drivers expect the code to behave. tried to do it this way in |prtypes.h|, but someone seems to overload global |new| already and linking libxpcom_core.so fails because of duplicate |new(size_t)|

I've done a bit of reading of the C++ spec's rules on operator new, and I haven't really found a good way to do what we want. While there are some hacks we could do to force new to return null rather than throw, I don't know of any that would prevent constructors from running when it returns null, which would usually cause a crash with a null-dereference trying to run the constructor. (It's not a problem only for types that lack constructors.) I have some spec questions I need to ask somebody more knowledgable, but I think the most likely ways to make progress are likely to be: * getting the gcc folks to make -fno-exceptions cause new to default to picking operator new(size_t, std::nothrow_t) * replacing all calls to global new with some ugly macro, or even with raw std::nothrow (if it's supported by all the compilers we care about) although there might be some C++ hack I'm not aware of.

i have very limited c++ knowledge, but tried to implement this - runs fine with testcases: --------------------------- #include <stdio.h> using namespace std; #include <new> #ifdef __cplusplus void* operator new(size_t size) { void *a; // puts("new new"); a=new (nothrow) char[size]; if(a==NULL) {puts(" new null");} return a; } #endif int main() { char *x="x"; char *a=x; a=new char[2<<31 -1]; char *b=x; b=new char[2<<31 -1]; /* if(a==NULL || b==NULL) {printf("f4ck\n");return 1;}*/ printf("a=%p %p\n",a,b); return(0); } --------------------------- (nothrow may be replaced by std::nothrow) put the above in |prtypes.h| and it compiles, but when linking shared xpcom, c++ complains about multiple new(size_t) - seems like someone already have overloaded global new. so if indeed someone have overloaded the global new, at least make it |nothrow| i have mixed feelings about involving the preprocessor in |new| - it may have side effects (may even not compile).

(In reply to comment #7) > "Exploiting the Otherwise Non-Exploitable on Windows" > ...an attacker gains control of the top-level unhandled exception filter So far it doesn't appear possible to exploit that in Mozilla: it relies on the order of loading and unloading dll's, and we never unload anything. It'd work if you added ActiveX support though :-( Isn't this bug a dupe of 324005? Also similar to bug 204143 and bug 290284

> What is under-allocation and how would it manifest itself? Suppose you have code (with 32-bit ints and pointers) that does: int size = siteSuppliedValue(); int *a = (int *) malloc(size * 4); for (int i = 0; i < size && i < 10; ++i) a[i] = siteSuppliedValue(); Upon first inspection, it might look like this is safe from buffer overflows. But if size is 2^30 + 1, the number passed to malloc will be 4 due to integer overflow, and many of the assignments will overflow. From the point of view of the code doing the assignments, the buffer was "under-allocated" for the amount of data involved. If there are integer overflow checks somewhere that protect the allocation at nsCanvasRenderingContext2D.cpp:2887, that's good.

(In reply to comment #16) > If there are integer overflow checks somewhere that protect the allocation at > nsCanvasRenderingContext2D.cpp:2887, that's good. There are -- nsCanvasRenderingContext2D.cpp:2859 checks that w/h are within mWidth/mHeight bounds, and mWidth/mHeight are checked earlier at nsCanvasRenderingContext2D.cpp:737 to make sure that mWidth*mHeight*4 doesn't overflow a 32-bit int.

(In reply to comment #15) > So far it doesn't appear possible to exploit that in Mozilla: it relies on the > order of loading and unloading dll's, and we never unload anything. > and are you sure none of the popular plugins never unloads anything in a suitable address space? > > Isn't this bug a dupe of 324005? Also similar to bug 204143 and bug 290284 > sure they are similar, feel free to resolve as dupe.

i'm aware of the SEH abuse, i was reminding people of it for the last couple of weeks. as for dll unloading, in theory one version of our plugin code was supposed to think about trying to do that, but i think it gave up. i tried a long time ago (circa 2000) to write macros to deal w/ this because i didn't have swap on BeOS, had only 512mb of ram, and gecko would have new throw. i had a bunch of macros, but i couldn't figure out how to write clean enough macros that people would actually be willing to use. the big problems i hit involved all the placement allocators and being able to mark them no throw properly.

(In reply to comment #19) > the big problems i hit involved all the placement allocators and being able to > mark them no throw properly. But that's the one macro that we actually do have and use extensively (named CPP_THROW_NEW, for some reason).

(In reply to comment #13) > that would prevent constructors from running when it returns null, which would > usually cause a crash with a null-dereference trying to run the constructor. According to the following (which I believe is an early version of ISO C++ Standard) the constructor will not be called when the non-throwing operator new returns null. (last sentence) http://www.kuzbass.ru:8086/docs/isocpp/expr.html#expr.new "-13- [Note: unless an allocation function is declared with an empty exception-specification (except.spec), throw(), it indicates failure to allocate storage by throwing a bad_alloc exception (clause except, lib.bad.alloc); it returns a non-null pointer otherwise. If the allocation function is declared with an empty exception-specification, throw(), it returns null to indicate failure to allocate storage and a non-null pointer otherwise. ] If the allocation function returns null, initialization shall not be done, the deallocation function shall not be called, and the value of the new-expression shall be null." The following compilers implement this correctly. gcc version 3.3.1, SuSE Linux gcc version 4.1.1, SuSE Linux gcc 4.0.1 on MacOSX 10.4.7 (Camino, native Intel build) MSVC++ 2005 Express Edition (Compiler Version 14.00.50727.42) on Windows XPSP2

I have tested this patch on: gcc version 3.3.1, SuSE Linux gcc version 4.1.1, SuSE Linux gcc 4.0.1 on MacOSX 10.4.7 (Camino, native Intel build) MSVC++ 2005 Express Edition (Compiler Version 14.00.50727.42) on Windows XPSP2 VC++ is a little special... the added code in prtypes.h compiles but results in an infinite loop when run because the inbuilt operator new(size_t, std::nothrow) is calling operator new(size_t). (disassembling it suggests it's implemented as "try{return new(sz);}catch(...){return 0;}"). So I excluded VC++ and used the link trick instead. This article have some details regarding MSVC++ and operator new: http://msdn.microsoft.com/msdnmag/issues/03/09/LegacySTLFix/default.aspx

So when I tried something similar to that earlier this week, I got: g++ -W -Wall -ansi -pedantic-errors -o new-test new-test.cpp new-test.cpp: In function ‘void* operator new(size_t)’: new-test.cpp:5: error: declaration of ‘void* operator new(size_t) throw ()’ throws different exceptions /usr/lib/gcc/i386-redhat-linux/4.1.1/../../../../include/c++/4.1.1/new:84: error: from previous declaration ‘void* operator new(size_t) throw (std::bad_alloc)’ new-test.cpp: In function ‘void* operator new [](size_t)’: new-test.cpp:11: error: declaration of ‘void* operator new [](size_t) throw ()’ throws different exceptions /usr/lib/gcc/i386-redhat-linux/4.1.1/../../../../include/c++/4.1.1/new:85: error: from previous declaration ‘void* operator new [](size_t) throw (std::bad_alloc)’ but I retried it just now without -pedantic-errors and it worked fine. That said, I think we compile with -pedantic, so I'd have expected you to get warnings with gcc -- and failing to compile with -pedantic isn't necessarily a good sign for the future. Also, I expect NSPR (which is C-only) may not want to enforce a no-exceptions policy on all its users, although maybe it does. It might be more appropriate to add this to nscore.h. I could be wrong, though. Also we have a NEW_H macro for compilers that have new.h but not new, so you can do: #include NEW_H

And I'm using gcc version 4.1.1 20060525 (Red Hat 4.1.1-1), from the Fedora Core 5 g++ package.

(In reply to comment #23) > I think we compile with -pedantic, so I'd have expected you to get > warnings with gcc -- and failing to compile with -pedantic isn't necessarily a > good sign for the future. This warns: g++ -pedantic test.cpp This does not: g++ -pedantic -fno-exceptions test.cpp which is why I missed it. I was trying to make a hint to the compiler that "this function does not throw" - it's not needed to make the patch work though. I added "throw(std::bad_alloc)" - making it silent with -pedantic. > Also, I expect NSPR (which is C-only) may not want to enforce a no-exceptions > policy on all its users, although maybe it does. It might be more appropriate > to add this to nscore.h. I could be wrong, though. FWIW, there are some uses of 'new' in nsprpub itself that checks the return value: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/nsprpub/pr/src/cplus/rcfileio.cpp&rev=1.6&root=/cvsroot&mark=112,113#96 http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/nsprpub/lib/prstreams/prstrms.cpp&rev=3.10&root=/cvsroot&mark=169,170#160 http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/nsprpub/pr/src/cplus/rcnetio.cpp&rev=1.6&root=/cvsroot&mark=62,63#55 There are a few uses which does not check the return value though, so it's hard to make any conclusions based on this. > Also we have a NEW_H macro for compilers that have new.h but not new, so you > can do: > #include NEW_H But NEW_H is defined in xpcom/xpcom-config.h and we probably shouldn't have a dependency from nsprpub to xpcom? If we make the change in nscore.h instead then it works... But, do all the C++ code we care about include xpcom/nscore.h?

1. add code to xpcom/nscore.h instead 2. use NEW_H 3. throw(std::bad_alloc)

(In reply to comment #25) > I was trying to make a hint to the compiler that "this function does not > throw" - it's not needed to make the patch work though. > I added "throw(std::bad_alloc)" - making it silent with -pedantic. In theory, you need the hint that it doesn't throw, throw(), to ensure that the compiler generates a null-check before the code to run the constructor. But it's possible that this isn't needed with gcc + -fno-exceptions.

(In reply to comment #27) I think that is what -fcheck-new is for, I'm going to redo the tests regarding constructor invocation with throw(std::bad_alloc)...

Yep, if we declare it "throw(std::bad_alloc)" then we need -fnew-check so the compiler adds a null-check of the return value before calling the constructor.

Given that we don't get the -pedantic warning when we're using -fno-exceptions, I'd rather leave it as throw() -- maybe use -fcheck-new as well, though. I'm not sure what this will do for our support for other compilers...

(In reply to comment #30) > I'd rather leave it as throw() -- maybe use -fcheck-new as well, though. Ok. I ran some more tests and found: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13215 The result of a new[] expression that fails with OOM is 0x4 for gcc < 3.4 (it does avoid calling the ctor though). (I assume we prefer to abort with the unhandled bad_alloc exception.) It does not affect the non-vector new.

the patch seems nice, but it breaks the build. c++ -o gtkmozembed2.o .... error: previous declaration of 'void* operator new [](size_t)' with 'C++' linkage ../../../../dist/include/xpcom/nscore.h:485: error: conflicts with new declaration with 'C' linkage

is this popular on talkback? signature?

(In reply to comment #32) > the patch seems nice, but it breaks the build. Thanks for the heads up. The problem is gtkmozembed.h which does extern "C" { #include "nscore.h" } I also note that if MOZILLA_CLIENT is *undefined* it doesn't include nscore.h and thus won't get the new operators. Maybe this is ok though.

Move the #include's outside of the extern "C" block. (this should be fixed regardless of what we do to nscore.h)

why treat MSVC specially?

(In reply to comment #36) > why treat MSVC specially? See comment 22

the patch builds and stops the exception, causing normal out of memory error. when the testcase is run without memory limits, it leaks about 1G of memory (after returning OM).

this overflows 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

this was originally just |new| throwing, but 2 things popped up: 1. memory leak of 1G per page visit 2. probably integer overflow should i file one or more of the above as new bugs or they are dupes?

prstreams and cplus are optional http://lxr.mozilla.org/mozilla/source/nsprpub/pr/src/Makefile.in 64 ifeq ($(USE_CPLUS), 1) 65 DIRS += cplus actually, i'm not sure you can build prstreams without manually asking.

We haven't built mozilla/nsprpub/pr/src/cplus and mozilla/nsprpub/lib/prstreams for a long time. You can consider them dead code.

*** Bug 324005 has been marked as a duplicate of this bug. ***

(In reply to comment #41) > this was originally just |new| throwing, but 2 things popped up: > > 1. memory leak of 1G per page visit > 2. probably integer overflow > > should i file one or more of the above as new bugs or they are dupes? If no one can point to a dup, please file new. /be

(In reply to comment #41) > > 1. memory leak of 1G per page visit this is bug 355217, though it needs the patch from this one to be applied. > 2. probably integer overflow this is bug 355216

Re-reading the comments in the bug I am not sure why this hasn't progressed in the last months. Were Mats' patches deemed inadequate? It seems to be a pretty general matter that would fix (or at least help fixing) lots of crash bugs, some of which were already mentioned. Because of the limited default address space of apps on OS/2 we are suffering especially. (While the work on bug 351246 will alleviate the problem a bit, bug 351943 and bug 364175 cannot be fixed without this one...) Is the security flag still necessary? If not, please remove it.

sure, this will kill a lot of crashes. also in mailnews. imho the patch is pretty safe.

A fix for the security issue spun into bug 355216 was released with FF2.0, clearing sensitive flag. reassigning to vlad to help get this in, seems to have stalled assigned to "nobody"

Comment on attachment 239537 [details] [diff] [review] fix? Marked this patch obsolete because it has been replace by Patch rev. 4 (attachment 239588 [details] [diff] [review]).

Comment on attachment 239588 [details] [diff] [review] Patch rev. 4 This comment in mozilla/config/config.mk needs to be updated: >+# Use an operator new that does not throw (bug 353144). >+# For other compilers we override operator new in prtypes.h. Change "prtypes.h" to "nscore.h".

Would like to get this stability fix on the branches but want some trunk-baking first. May or may not make these next releases.

I have investigated dbaron's comment 30: > I'm not sure what this will do for our support for other compilers... Unfortunately the patch fails to compile using Sun C++ 5.8 on Solaris 10. Using throw(std::bad_alloc) compiles, but it invokes the constructor even if the allocation fails. I don't see any easy way around that. We could still take the rev.4 patch of course, but then we should wrap the nscore.h addition in #ifdef __GNUC__ An alternative would be to macroize all calls to 'new'. for example, "new X" => "NS_NEW X" "new (a) X" => "NS_NEW1(a) X" ... #define NS_NEW new (std::nothrow) #define NS_NEW1(arg1) new ((arg1),std::nothrow) ... This is more portable and also more flexible in that it allows linking against code (third party or not) that expects 'new' to throw when it fails. If that is a combination we want to support then rev.4 could cause problems. I think I prefer the macro solution over "rev.4" actually. The price is that direct calls to 'new' will not be allowed - one has to use the NS_NEW macros. I have a patch almost ready that implements this. What do you think?

why is the macro needed instead of just adding the std::nothrow everywhere? are there compilers that don't support nothrow?

(In reply to comment #54) > why is the macro needed instead of just adding the std::nothrow everywhere? Not really, I picked it because it's shorter to type/gives shorter lines. It also provides some flexibility should we have a need to redefine the macros in some environment in the future. Either way is fine with me though. > are there compilers that don't support nothrow? Not that I know of. I've tested gcc3 and 4, VC8 and Sun's compiler.

I think the approach to redefine "new" would be great. Before, I had just confirmed that it still compiles with attachment 239588 [details] [diff] [review] on OS/2, but yesterday I actually tried to reproduce the problem from bug 351943 in a FF build with the patch, and found that it still crashed (trying to throw in a non try-catch environment). Perhaps use of -fcheck-new in addition to the patch is required? But I cannot exclude another compiler bug on OS/2. I also agree with Mats that if we have to change all instances of "new" we should use a macro to be more flexible for possible future redefines or yet unknown platform difficulties.

C++ portability is hard, even in 2007 apparently, but we should strive to use standard forms over our own idiomatic macrology. Just because some platform might (temporarily) have a broken compiler does not justify yet another peculiar Mozilla-ism. Has anyone tried getting a fix from Sun for the compiler problem reported in comment 53, if indeed it is a compiler bug? /be

Yes, please no NS_NEW* macros. We can add a configure check to make sure that the compiler being used does the right thing with our operator new; if it doesn't, print a warning and disable our NULL-returning operator new. When built with those compilers, operator new would still throw, but that's been the case for years now and so is likely not to really hurt anything.

(In reply to comment #53) > An alternative would be to macroize all calls to 'new'. for example, > "new X" => "NS_NEW X" > "new (a) X" => "NS_NEW1(a) X" > ... > #define NS_NEW new (std::nothrow) > #define NS_NEW1(arg1) new ((arg1),std::nothrow) > ... > all instances of |new| should be replaced with NS_NEW or NS_NEW1 - this doesn't seem easy? and just: #define new new (std::nothrow) besides beeing scary breaks new (std::nothrow) int[40];

* change all non-placement "new" calls to "new (std::nothrow)" * change all "operator new(size_t)" methods to "operator new(size_t, const std::nothrow_t& NS_NOTHROW_COMPILE_CHECK)" The NS_NOTHROW_COMPILE_CHECK is just a debug utility - when defined as "= std::nothrow" it causes a compile time error in gcc because of the "operator new(size_t)" ambiguity. This way the compiler flags any "new" calls that does not have "(std::nothrow)" as errors. See xpcom/base/nscore.h (I've left this feature off by default for now but I think we could enable it under NS_DEBUG in the future.)

Could you attach a version of the patch with just the changes that aren't just "new" -> "new (std::nothrow)"?

(In reply to comment #62) > Could you attach a version of the patch with just the changes that aren't just > "new" -> "new (std::nothrow)"? This is the manual adjustments I did after the scripted changes.

Comment on attachment 255097 [details] [diff] [review] Manual adjustments... ./content/base/src/nsMappedAttributes.cpp wrong indentation - return new (std::nothrow) morkFactory(new (std::nothrow) orkinHeap()); + return new morkFactory(new (std::nothrow) orkinHeap()); why this change? +++ ./layout/style/nsCSSDataBlock.h 2007-02-14 14:36:40.000000000 +0100 - sizeof(char) * block_chars); + sizeof(char) * block_chars, + std::nothrow); indentation here looks wrong (on the first line) +++ ./layout/style/nsCSSLoader.cpp 2007-02-14 14:36:40.000000000 +0100 + new (std::nothrow) SheetLoadData(this, EmptyString(), // title doesn't matter here aParserToUnblock, aURI, again, wrong indentation +++ ./layout/style/nsCSSValue.cpp 2007-02-14 14:36:40.000000000 +0100 again +++ ./mailnews/base/search/src/nsMsgSearchSession.cpp 2007-02-14 14:36:40.000000000 +0100 again why the nspr changes (of course I don't know if anyone uses that code...) I'm not sure you should make the airbag changes, that code is from elsewhere. why the ./toolkit/mozapps/update/src/updater/updater.cpp changes? ./widget/src/gtk/nsWindow.cpp wrong indentation +++ ./xpcom/base/nscore.h 2007-02-14 15:46:42.000000000 +0100 + * of the ambiguity with new(size_t). Calls through NS_NEW macros works which NS_NEW macros? They are mentioned several times in this file... ./xpfe/bootstrap/appleevents/nsAEClassDispatcher.cpp why are you removing the std::nothrow in this and other places?

Comment on attachment 250515 [details] Patch rev. 5 (diff -u2, zipped) This attachment edit page crashed my browser twice, fixing content type. zips aren't patches. that said, r=biesi, I trust that the automatic changes are fine

I don't really know why a patch to touch a lot of files in the tree is hiding out in a bug about canvas, but I don't think this is the right approach. I can almost guarantee that as soon as this lands new news will start appearing without nothrow. This needs a lot more discussion (newsgroups?) imho before it gets reviews or lands.

how about we change the new in canvas to malloc and close this bug?

Yes, this definitely needs to be raised in the newsgroup. I really disagree with this patch. We're simply never going to be able to keep all |new|s in check. Additionally, we're hopefully turning on exceptions in a not too distant future which would make this patch a waste of everyones time.

(In reply to comment #68) > Yes, this definitely needs to be raised in the newsgroup. > > I really disagree with this patch. We're simply never going to be able to keep > all |new|s in check. Additionally, we're hopefully turning on exceptions in a > not too distant future which would make this patch a waste of everyones time. > if you don't want to track new |new| the other approach is to overload global |new| with |nothrow| - as discussed here this seems to work. exceptions won't help and are just shifting the problem. with exceptions you will need similar refactoring around |new| the catch out of memory exceptions which is essentially the same as this patch. this is serious for low memory devices like mobiles.

Ok, this bug morphed into something much bigger, as stuart and others pointed out. Canvas can't use malloc because it uses nsAutoArrayPtr in a few places, which frees data with delete []. So, here's a patch that just adds std::nothrow to all the large buffer allocations. It explicitly does /not/ go through and add std::nothrow to all calls to new; that's not a convention that's been decided upon, so I won't do that, but this should get rid of the major cases covered by this bug.

(In reply to comment #66) > I can almost guarantee that as soon as this lands new news > will start appearing without nothrow. (In reply to comment #68) > We're simply never going to be able to keep all |new|s in check. Read the comments in this bug, there is a solution to force a compile error for that.

The *entire* code base is written under the assumption that 'new' returns NULL when it fails, but in fact it throws an exception. This is a systematic error and therefore it needs to be fixed globally. And the solution is amazingly simple, just change 'new' to 'new (std::nothrow)' everywhere. whatever...

some clever nice manager stand up and explain the proactiveSIKIRITY-antiDOS approach? this may not be very nice in mail.

Noone is saying that we shouldn't fix our issues with new and exceptions. What people are saying is that we should not fix them in THIS bug, and not without getting buy-in from brendan, jst, bz, sicking, etc. and letting people know what's going on via the newsgroups. This bug is specifically about canvas, where large buffers are allocated using new. I realize that this is a pretty pedantic approach, but I'm trying to fix this actual bug without having to create codebase-wide policy.

Patch for canvas was checked in; I'd be happy to help get the overall problem fixed, but let's open a separate bug for the codebase-wide issue and get it fixed there.

(In reply to comment #74) > This bug is specifically about canvas, > where large buffers are allocated using new. It's pretty stupid to now have to open a new bug (when this had already grown beyond the original problem anyway), and point to discussion and patches in this bug all the time...

(In reply to comment #76) > (In reply to comment #74) > > This bug is specifically about canvas, > > where large buffers are allocated using new. > > It's pretty stupid to now have to open a new bug (when this had already grown > beyond the original problem anyway), and point to discussion and patches in > this bug all the time... No, it's smart. Or (to avoid inflammatory suggestions of stupidity) it's more effective if your goal is to fix bugs. Bugs tend to grow interesting discussion hair, but with noise high enough that a new bug, citing bug 353144 comment 70 for example, actually results in more positive outcomes, sooner. /be

It was probably not really my call but as nobody else had stepped up to do it in the last months I now posted in the newsgroups. Of course still listing this bug number...

hello world, in bug 414511 comment 9 i noted: I think my favorite is this one: nsAutoArrayPtr<PRUint8> imageBuffer(new (std::nothrow) PRUint8[w * h * 4]); ok, don't throw. PRUint8 *imgPtr = imageBuffer.get(); get pointer #ifdef IS_LITTLE_ENDIAN *imgPtr++ = ib; use pointer. Oh really? Is that because of this bug?

instead of fixing every single |new|, why not just shadow the throwing |new| in a header? this kludge seems to work: mnew: ----- #ifndef MNEW1 #define MNEW1 #include <new> void * operator new (std::size_t sz) throw (std::bad_alloc) { void *p; p = new (std::nothrow) char[sz]; return p; } #endif ----- #include "mnew" #include <new> #include <iostream> using namespace std; int main() { int count2; //oldh=set_new_handler(out_of_memory); while(1) { count2++; int *x=new int[100000]; // Exhausts memory std::cout << "x=" << x << std::endl; } }

the order of the header inclusion doesn't seem to matter, |namespace std| isn't needed. the gcc library checks for sz==0

in order comment #80 to work with classes additional flag for g++ is needed: -fcheck-new