Make NS_Alloc infallible

RESOLVED FIXED in Firefox 13

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Christophe JAILLET, Assigned: Lucas Molas)

Tracking

(Depends on: 1 bug)

Trunk
mozilla13
x86
Windows 2000
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11+ wontfix, firefox12 wontfix, firefox13 fixed)

Details

(Whiteboard: [good first bug] [mentor=jdm] [lang=c++][qa-])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.

Updated

6 years ago
Whiteboard: [good first bug] [mentor=jdm]

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 1

6 years ago
Created attachment 554758 [details] [diff] [review]
Check if NS_Alloc failed

Updated

6 years ago
Attachment #554758 - Flags: review?(benjamin)

Comment 2

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

You'll need to check for !*_retval for this to work properly.
Attachment #554758 - Flags: review?(benjamin)

Comment 3

6 years ago
Oh, sorry. I jumped the gun there D:

Comment 4

6 years ago
Created attachment 554767 [details] [diff] [review]
Fixing typo in previous patch... Check for failed allocation

Sorry about that typo.
Attachment #554758 - Attachment is obsolete: true
Attachment #554767 - Flags: review?(benjamin)
NS_Alloc is infallible.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
Attachment #554767 - Flags: review?(benjamin)

Comment 6

6 years ago
I'm told that NS_Alloc is just moz_malloc, which is fallible.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
If that's the case, please just make NS_Alloc infallible.
Assignee: nobody → josh
Summary: Possible NULL dereference in nsComponentManagerImpl::ContractIDToCID → Make NSAlloc infallible
Summary: Make NSAlloc infallible → Make NS_Alloc infallible

Updated

6 years ago
Whiteboard: [good first bug] [mentor=jdm] → [good first bug] [mentor=jdm] [lang=c++]
(Assignee)

Comment 8

6 years ago
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.

Updated

6 years ago
Attachment #554767 - Attachment is obsolete: true

Comment 9

6 years ago
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.
Assignee: josh → l5m5.dev
(Assignee)

Comment 10

6 years ago
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.
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.
(Assignee)

Comment 12

6 years ago
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.
Attachment #573254 - Attachment is obsolete: true

Updated

6 years ago
Attachment #573265 - Flags: review?(benjamin)
Attachment #573265 - Flags: review?(benjamin) → review+
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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/26ce0ab56d32

See comment 13 for for the reasoning behind the tracking flag.
tracking-firefox11: --- → ?
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
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Any reason not to use moz_{x,}malloc directly in our code instead of NS_Alloc?
Depends on: 705035
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.
(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.
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
Depends on: 705630
(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).
tracking-firefox11: ? → +
Depends on: 716594

Updated

5 years ago
Depends on: 719531

Updated

5 years ago
status-firefox11: --- → fixed
Depends on: 723472
http://hg.mozilla.org/releases/mozilla-beta/rev/b091fc697c22

Had to back this out of beta due to Bug 723472.
status-firefox11: fixed → wontfix
status-firefox12: --- → fixed
Target Milestone: mozilla11 → mozilla12
Whiteboard: [good first bug] [mentor=jdm] [lang=c++] → [good first bug] [mentor=jdm] [lang=c++][qa-]
https://hg.mozilla.org/releases/mozilla-beta/rev/6323a689d6f1
status-firefox12: fixed → wontfix
status-firefox13: --- → fixed
Target Milestone: mozilla12 → mozilla13
You need to log in before you can comment on or make changes to this bug.