The default bug view has changed. See this FAQ.

Handle OOM in NewPropertyIteratorObject

RESOLVED FIXED in Firefox 28

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: decoder, Assigned: decoder)

Tracking

(Blocks: 2 bugs, {crash})

Trunk
mozilla29
x86_64
Linux
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 fixed, firefox29 fixed)

Details

(Whiteboard: [qa-], crash signature)

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
In NewPropertyIteratorObject we seem to be calling NewBuiltinClassInstance without checking its return value, although it's fallible:

>     return &NewBuiltinClassInstance(cx, &PropertyIteratorObject::class_)->as<PropertyIteratorObject>();


The attached patch checks the return value first and returns NULL on failure. This fixes an OOM crash bug for me that the fuzzer keeps hitting.
(Assignee)

Comment 1

3 years ago
Created attachment 8344973 [details] [diff] [review]
js-NewPropertyIteratorObject-oom.patch

Yay, Bugzilla ate my attachment \o/

njn, can you review this? Blame tells me you added the line in question :) Thanks!
Assignee: general → choller
Status: NEW → ASSIGNED
Attachment #8344973 - Flags: review?(n.nethercote)
(Assignee)

Updated

3 years ago
Duplicate of this bug: 948187
Comment on attachment 8344973 [details] [diff] [review]
js-NewPropertyIteratorObject-oom.patch

Review of attachment 8344973 [details] [diff] [review]:
-----------------------------------------------------------------

I suspect I refactored that line but didn't add it originally.  But the patch is simple and looks fine.  Thanks!
Attachment #8344973 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 4

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad9945733719
https://hg.mozilla.org/mozilla-central/rev/ad9945733719
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Christian: should this OOM fix be uplifted to Aurora 28 and Beta 27?
Flags: needinfo?(choller)
(Assignee)

Comment 7

3 years ago
It's not necessary to uplift OOM fixes unless we see them in some crash stats. Uplifting to Aurora would surely be possible (easy fix, and less crashes in fuzzing then).
Flags: needinfo?(choller)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8344973 [details] [diff] [review]
js-NewPropertyIteratorObject-oom.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): N/A
User impact if declined: Crashes with OOM conditions
Testing completed (on m-c, etc.): A few days on mozilla-central
Risk to taking this patch (and alternatives if risky): Not risky, patch is very simple (just adding a null check).
String or IDL/UUID changes made by this patch: None
Attachment #8344973 - Flags: approval-mozilla-aurora?
(In reply to Christian Holler (:decoder) from comment #7)
> It's not necessary to uplift OOM fixes unless we see them in some crash
> stats.

During the Aurora timeframe, I would think the bar would be pretty low with respect to taking potential stability fixes. If it's simple and low-risk, I would argue to just take it whether it's in crash stats. On beta, sure the bar is higher :)
Attachment #8344973 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/75a2bbd8c4b6
status-firefox28: --- → fixed
status-firefox29: --- → fixed
I don't think this needs QA verification. If anyone thinks that's a mistake please remove the [qa-] whiteboard tag and add the verifyme keyword.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.