Last Comment Bug 680556 - Make NS_Alloc infallible
: Make NS_Alloc infallible
Status: RESOLVED FIXED
[good first bug] [mentor=jdm] [lang=c...
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Windows 2000
: -- normal (vote)
: mozilla13
Assigned To: Lucas Molas
:
Mentors:
Depends on: 705035 705630 716594 719531 723472
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-19 14:48 PDT by Christophe JAILLET
Modified: 2012-04-05 11:22 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
wontfix
fixed


Attachments
Check if NS_Alloc failed (856 bytes, patch)
2011-08-21 15:04 PDT, Alex Miller
no flags Details | Diff | Splinter Review
Fixing typo in previous patch... Check for failed allocation (857 bytes, patch)
2011-08-21 16:30 PDT, Alex Miller
no flags Details | Diff | Splinter Review
Modification to NS_Alloc and NS_Realloc to use moz_xmalloc and moz_xrealloc. (1.03 KB, patch)
2011-11-09 10:50 PST, Lucas Molas
no flags Details | Diff | Splinter Review
NS_Alloc/Realloc without the size check (1.08 KB, patch)
2011-11-09 11:42 PST, Lucas Molas
benjamin: review+
Details | Diff | Splinter Review

Description Christophe JAILLET 2011-08-19 14:48:42 PDT
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 2011-08-21 15:04:40 PDT
Created attachment 554758 [details] [diff] [review]
Check if NS_Alloc failed
Comment 2 Josh Matthews [:jdm] 2011-08-21 16:04:45 PDT
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 2011-08-21 16:28:30 PDT
Oh, sorry. I jumped the gun there D:
Comment 4 Alex Miller 2011-08-21 16:30:46 PDT
Created attachment 554767 [details] [diff] [review]
Fixing typo in previous patch... Check for failed allocation

Sorry about that typo.
Comment 5 Benjamin Smedberg [:bsmedberg] 2011-08-23 12:23:01 PDT
NS_Alloc is infallible.
Comment 6 Josh Matthews [:jdm] 2011-08-23 12:27:37 PDT
I'm told that NS_Alloc is just moz_malloc, which is fallible.
Comment 7 Benjamin Smedberg [:bsmedberg] 2011-08-23 12:30:59 PDT
If that's the case, please just make NS_Alloc infallible.
Comment 8 Lucas Molas 2011-11-08 13:31:34 PST
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] 2011-11-08 13:53:58 PST
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 2011-11-09 10:50:54 PST
Created attachment 573254 [details] [diff] [review]
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] 2011-11-09 10:59:01 PST
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 2011-11-09 11:42:21 PST
Created attachment 573265 [details] [diff] [review]
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 [:bsmedberg] 2011-11-22 07:31:21 PST
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] 2011-11-22 09:33:11 PST
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] 2011-11-23 04:30:27 PST
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 (⌚ UTC+1/+2) 2011-11-23 07:10:20 PST
Any reason not to use moz_{x,}malloc directly in our code instead of NS_Alloc?
Comment 17 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-23 20:42:37 PST
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?) 2011-11-23 21:24:30 PST
(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 Zambrano [:armenzg] (EDT/UTC-4) 2011-11-25 07:25:37 PST
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] 2012-01-09 11:08:51 PST
(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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-02 09:57:31 PST
http://hg.mozilla.org/releases/mozilla-beta/rev/b091fc697c22

Had to back this out of beta due to Bug 723472.
Comment 22 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-04-05 11:22:49 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/6323a689d6f1

Note You need to log in before you can comment on or make changes to this bug.