680556 - Make NS_Alloc infallible

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Christophe JAILLET]

User Agent: Mozilla/5.0 (Windows NT 5.0; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 20110811165603

Steps to reproduce:

Possible NULL dereference in nsComponentManagerImpl::ContractIDToCID.

Return value of NS_Alloc is not checked before use.

Comment 1

[Alex Miller]

Attachment: Check if NS_Alloc failed

Comment 2

[Josh Matthews [:jdm]]

Comment on attachment 554758 [details] [diff] [review]
Check if NS_Alloc failed

You'll need to check for !*_retval for this to work properly.

Comment 3

[Alex Miller]

Oh, sorry. I jumped the gun there D:

Comment 4

[Alex Miller]

Attachment: Fixing typo in previous patch... Check for failed allocation

Sorry about that typo.

Comment 5

[Benjamin Smedberg]

NS_Alloc is infallible.

Comment 6

[Josh Matthews [:jdm]]

I'm told that NS_Alloc is just moz_malloc, which is fallible.

Comment 7

[Benjamin Smedberg]

If that's the case, please just make NS_Alloc infallible.

Comment 8

[Lucas Molas]

Hi, I'd like to take this as my first bug. I know C++ but I'm not familiar with the code (although I'm trying to read as much as I can). I'm not sure where to start, I was checking the patch and mozalloc.cpp, maybe you could point me in the right direction.

Comment 9

[Josh Matthews [:jdm]]

Great! You'll want to look in http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsMemoryImpl.cpp#193, where NS_Alloc and NS_Realloc calls moz_malloc and moz_realloc respectively. This should be changed to moz_xmalloc and moz_xrealloc, which abort on OOM errors. This means you can also get rid of the checks to see if moz_malloc/moz_realloc fail as well, and just return the result directly.

Comment 10

[Lucas Molas]

Attachment: Modification to NS_Alloc and NS_Realloc to use moz_xmalloc and moz_xrealloc.

Thanks for the help! Here's the patch with the change you mentioned. Some references to NS_Alloc like http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsUUIDGenerator.cpp#113 check to see if it fails to return an OOM error, but as  NS_Alloc may return a nsnull in the case the size is too big, I'm not sure if those checks should be removed as well (also I've seen  NS_Alloc is defined differently in xpcom/stub/ and xpcom/glue/ as NS_Alloc_P and xpcomFunctions.allocFunc respectively, which I don't know if are finally references of the original function or differ in their implementation). The same goes for NS_Realloc.

I compiled the modified code but I don't know if I should run some tests as well.

Comment 11

[Josh Matthews [:jdm]]

That's a good point about the size being too big. Please take out those early returns - if we're making NS_Alloc infallible, it should be infallible in all cases. It's probably not necessary to modify existing callers of NS_Alloc to remove null checks (unless you want to and there's a manageable number). It looks like the other definitions of NS_Alloc just end up calling the one you're changing in various roundabout ways, so there shouldn't be a need for any other changes. With regards to testing, I expect that running the browser and surfing around will be a fine sanity check, and then we'll run the changes through the full testsuite on tryserver to make sure nothing's broken.

Comment 12

[Lucas Molas]

Attachment: NS_Alloc/Realloc without the size check

I've noticed that the version of NS_Alloc in http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/standalone/nsXPCOMGlue.cpp#445 checks to see if the function exists and otherwise returns nsnull, is that a problem?

I saw a bit over 50 checks to NS_Alloc throughout the code, I could remove them (most of them are very simple) but I'm not sure if I should be touching so much of the code.

I run the browser and seemed to worked OK.

Comment 13

[Benjamin Smedberg]

When this lands, please mark the tracking-firefox11? flag, because this could theoretically cause an uptick in out-of-memory aborts and we want to track that.

Comment 14

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/26ce0ab56d32

See comment 13 for for the reasoning behind the tracking flag.

Comment 15

[Ed Morley [:emorley]]

Lucas congratulations on your first patch in the tree! 

Hope to see you on IRC in #developers soon (see https://wiki.mozilla.org/IRC#Getting_Started for details). If you'd like to fix another bug (it would be great if you did!) but need some inspiration, pop on & say hi - and we'll find something for you :-)

https://hg.mozilla.org/mozilla-central/rev/26ce0ab56d32

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Any reason not to use moz_{x,}malloc directly in our code instead of NS_Alloc?

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

For some reason, this patch causes a ~32% increase in Tp_nochrome on android-xul and android-native. I see it when it landed on mozilla-inbound. I see it when it landed on mozilla-central and when it merged to birch.

I think we need to back it out.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #17)
> For some reason, this patch causes a ~32% increase in Tp_nochrome on
> android-xul and android-native. I see it when it landed on mozilla-inbound.
> I see it when it landed on mozilla-central and when it merged to birch.
> 
> I think we need to back it out.

Actually, it appears this was a talos deployment issue. Yay! No need to back out.

Comment 19

[Armen [:armenzg]]

I've verified the regression was caused by the talos upgrade.

Mozilla-Aurora should have stayed fixed in this graph:
http://graphs-new.mozilla.org/graph.html#tests=[[84,11,20],[84,63,20],[84,52,20]]&sel=none&displayrange=7&datatype=running

Comment 20

[Alex Keybl [:akeybl]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> When this lands, please mark the tracking-firefox11? flag, because this
> could theoretically cause an uptick in out-of-memory aborts and we want to
> track that.

Tracking and including Sheila/Kairo to see if we get any more OOM crashes (please correct me if this isn't the same thing as aborts).

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/releases/mozilla-beta/rev/b091fc697c22

Had to back this out of beta due to Bug 723472.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6323a689d6f1