It turns out that NS_HOLD_JS_OBJECTS and NS_DROP_JS_OBJECTS can fail, basically only in OOM situations where we couldn't create mJSHolders, or can't add an entry to the hashtable. There are around 28 places the former is called (including a bunch in IndexedDB), and only 4 of them check for error. http://mxr.mozilla.org/mozilla-central/ident?i=NS_HOLD_JS_OBJECTS&filter= This is bad because the object will think that the JS object it holds is rooted when it is not. So we should either fix the other 24 callees or just make these functions infallible and crash the browser if these hash table operations fail. I'm inclined to do the latter, because that will avoid regressions in the future, and it will require less changes, but I'll defer to the opinions of others. (NS_DROP_JS_OBJECTS isn't really checked either, but the only way it can fail is if there is no table, in which case the corresponding HOLD probably also failed, so who cares...) I marked this security sensitive, though I'm not sure if potential GC hazards when we're running out of memory and likely to be in danger of imminent crashing anyways is a big deal. This was found by Cindy Rubio González <firstname.lastname@example.org>'s error propagation analysis.
Aren't hashtable allocations infallible now?
(In reply to Kyle Huey [:khuey] (email@example.com) from comment #1) > Aren't hashtable allocations infallible now? XPConnect uses JS_DHash, which uses js_malloc, which I think is fallible. I could be wrong.
Would someone please add "[uwisc-analysis]" to the Whiteboard list for this bug? We've been using that elsewhere to tag bugs discovered by Cindy Rubio González <firstname.lastname@example.org>'s error propagation analysis. I'd add it here myself, but I don't have that power. Thanks!
> I'd add it here myself, but I don't have that power. Thanks! Refresh and you will!
Assignee: nobody → continuation
Depends on: 782792
I eliminated one of the ways NS_HOLD_JS_OBJECTS can fail in bug 782792, so this is mostly fixed now. Technically, it can still fail if there's no sXPConnect. That seems unlikely, though, so maybe it should just crash in that case...
Whiteboard: [uwisc-analysis] → [uwisc-analysis] fixed by bug 782792
Bug 792861 eliminates the last ways that these functions can fail, and adds checks to the chokepoints that will make the browser crash if it starts being able to fail, which will make this whole interface much safer. After that is done, we can close this.
Depends on: 792861
I guess we don't care too much what happens when there's no XPConnect around, so I'll mark this fixed...
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-firefox-esr10: --- → wontfix
status-firefox-esr17: --- → wontfix
status-b2g18: --- → fixed
status-firefox18: --- → fixed
You need to log in before you can comment on or make changes to this bug.