948188 - Handle OOM in NewPropertyIteratorObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Christian Holler (:decoder)]

Attachment: 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!

Comment 3

[Nicholas Nethercote [inactive]]

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!

Comment 4

[Christian Holler (:decoder)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad9945733719

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad9945733719

Comment 6

[Chris Peterson [:cpeterson]]

Christian: should this OOM fix be uplifted to Aurora 28 and Beta 27?

Comment 7

[Christian Holler (:decoder)]

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).

Comment 8

[Christian Holler (:decoder)]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

(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 :)

Comment 10

[Christian Holler (:decoder)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/75a2bbd8c4b6

Comment 11

[u279076]

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.