1548044 - OOM in AutoSweepObjectGroup leaves group flags in unexpected state

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Iain Ireland [:iain]]

I tracked down the source of a weird try failure on my call IC patches, and it turned out to be an unrelated intermittent TI bug.

In ObjectGroup::markPropertyNonWritable, we create an AutoSweepObjectGroup and pass it into getProperty. In getProperty, we assert that |sweep| does not have unknown properties.

The AutoSweepObjectGroup constructor calls ObjectGroup::sweep. As part of that function, we copy properties over from the old arena to the new one. If an allocation fails, we set unknown properties and return.

If an OOM test (or, in theory, a real OOM) occurs while we are copying properties, then we end up asserting in a debug build. I'm not sure what happens in a non-debug build, but Ted thinks this is similar enough to wild pointer issues in crash stats to justify marking this as a sec bug.

I have an instance of this bug trapped in rr, if we need to investigate anything more closely. (Note to self: js-52)

Comment 1

[Ted Campbell [:tcampbell]]

This is an excellent find. There is a possibility this answers Bug 1425293.

Comment 2

[Ted Campbell [:tcampbell]]

Attachment: Bug 1548044 - Handle unknownProperties as result of AutoSweepObjectGroup

Comment 3

[Ted Campbell [:tcampbell]]

To allow the IC work to land in the 68 cycle, I've disabled the flaky test in Bug 1549396. Once we fix things here, we can re-enable the test. I expect we will uplift this to 68 anyways, but I want the non-sec work landing normally.

Comment 4

[Jason Orendorff [:jorendorff]]

tcampbell, is this stalled? Was the patch meant to be up for review?

Comment 5

[Liz Henry (:lizzard)]

Following up in email with tcampbell.

Comment 6

[Ted Campbell [:tcampbell]]

Update: Reviving the patch and rebasing with a few tweaks. I've been working with Iain and Jan on going of the details and we think we have something workable. More testing, sanity checking needing but final version should be ready early next week.

I think we'll want to get this in to ESR68 for consistency. The trigger is an extremely well targeted OOM and the code is very complex so I expect we'll want to favour giving the patch a bit of time to bake on nightly instead of rushing to uplift.

Comment 7

[Ted Campbell [:tcampbell]]

Comment on attachment 9062031 [details]
Bug 1548044 - Handle unknownProperties as result of AutoSweepObjectGroup

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Difficult. We, the developers, are not clear how.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all supported
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: This patch grafts cleanly on ESR-68, FF-69, Beta-70, mozilla-central.
• How likely is this patch to cause regressions; how much testing does it need?: The code it affects is quite tricky, but the patch itself is straightforward although there can always be surprises. We don't have any additional testing requirements beyond usual treeherder testing. Giving it a little more time to bake on Nightly may be helpful though.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for mozilla-central. We'll want branch patches as well.

Comment 9

[Ted Campbell [:tcampbell]]

Testing once on try before I push to m-c shortly. The same patch works cleanly on all branches.

Comment 10

[Ted Campbell [:tcampbell]]

https://hg.mozilla.org/integration/autoland/rev/153482073368465845666344d84ec51d5b404f5c

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR68 approval when you get a chance.

Comment 13

[Ted Campbell [:tcampbell]]

No surprises in nightly when I looked at top-crashes for builds including the fix. I'm not convinced this will change crash rate, but it is good to get the potential sec issues fixed. Will nominate for uplifts.

Comment 14

[Ted Campbell [:tcampbell]]

Comment on attachment 9062031 [details]
Bug 1548044 - Handle unknownProperties as result of AutoSweepObjectGroup

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec bug.
• User impact if declined: Potential (hard to exploit) security bugs. Potentially responsible for random memory corruption crashes.
• Fix Landed on Version: 71
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): The change is straightforward and more consistent than the existing code, but the area of engine is complex.
• String or UUID changes made by this patch:

Beta/Release Uplift Approval Request

• User impact if declined: Potential (hard to exploit) security bugs. Potentially responsible for random memory corruption crashes.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): The change is straightforward and more consistent than the existing code, but the area of engine is complex.
• String changes made/needed:

Comment 15

[Ted Campbell [:tcampbell]]

This is unlikely to explain the crashes in Bug 1425293.

Once this lands, we'll need to re-enable gc/bug-1215678.js. It was tripping debug asserts without this patch.

Comment 16

[Liz Henry (:lizzard)]

Can you file a followup bug for re-enabling the test? thanks!

Comment 17

[Liz Henry (:lizzard)]

Comment on attachment 9062031 [details]
Bug 1548044 - Handle unknownProperties as result of AutoSweepObjectGroup

Fix for sec-high issue, let's uplift for beta 11.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/a80f2fc25393

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9062031 [details]
Bug 1548044 - Handle unknownProperties as result of AutoSweepObjectGroup

Fixes a JS sec bug, approved for 68.2esr.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/3c725f75c8ba

Comment 21

[Ted Campbell [:tcampbell]]

Re-adding test in Bug 1606463.