1331058 - Definite Properties Analysis doesn't detect |this| escaping via CallObjects

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Shell test

AKA as the Facebook crash.

Consider the following JS code:

  function foo(o) { return function() { return o; }; }

  function SomeObject(o) {
      this.x = foo(this);
  }

When we run the definite properties analysis on SomeObject, we inline foo. That's fine, but we then don't track that foo lets |this| escape via its CallObject.

The bug is in IonBuilder::initEnvironmentChain, where we don't call createCallObject if we're in analysis mode. We should just fix that condition to do the right thing for the DPA.

Attaching a testcase for the JS shell that crashes in exactly the same way.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

One-line fix.

Note that the "Skip this for analyses, as the script might not have a baseline script with template objects yet." comment only applies to the arguments analysis. We always have a BaselineScript when we run the DPA.

Comment 2

[Jan de Mooij [:jandem]]

It's possible something in Firefox 51 exposed this somehow (probably the frontend rewrite, see bug 1308800 comment 30), but the code we're fixing is also in ESR45. Let's fix this everywhere, just to be safe.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8826711 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not easily. Writing the shell test took me a while, even when I knew what the bug was. It's possible this is always a safe nullptr crash, but I'm not 100% sure.

> 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, though the crashes started in 51.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be trivial to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely. Green Try push should be sufficient.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8826711 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Unknown.
[User impact if declined]: Lots of Facebook crashes for certain users.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Verified locally.
[Needs manual test from QE? If yes, steps to reproduce]: Could test FB.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Very small and safe patch. We have lots of tests for this code.
[String changes made/needed]: None.

Comment 5

[Jan de Mooij [:jandem]]

NI as requested.

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for 51+. Waiting for Al/Dan to confirm whether we ought to take this in ESR45 or not.

Gerry/Liz, given the volume of the FB crash I'd suggest we take this in RC1. Also it's a one-line fix that jandem considers pretty safe and has an automated test for verification purpose.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Shell test 2

Here's a different test that shows this can be used to read uninitialized memory.

$ dist/bin/js --no-threads test.js
test.js:4:2 Error: Assertion failed: got 757935405, expected (void 0)

>>> hex(757935405)
'0x2d2d2d2d'

Comment 8

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

(In reply to Ritu Kothari (:ritu) from comment #6)
> Tracked for 51+. Waiting for Al/Dan to confirm whether we ought to take this
> in ESR45 or not.
> 
> Gerry/Liz, given the volume of the FB crash I'd suggest we take this in RC1.
> Also it's a one-line fix that jandem considers pretty safe and has an
> automated test for verification purpose.

I'll give sec-approval+ for trunk based on you being interested in this for RC1. I'd suggest taking it for ESR45.

Comment 9

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0431a0ab907d53646d359ad10507efe7f89dd487

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0431a0ab907d

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8826711 [details] [diff] [review]
Patch

Fix for the FB crash, sec-high issue. Please uplift to aurora, beta, and esr45.

Comment 12

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e04cc4582dd86864035f0aed5fac81907dea848
https://hg.mozilla.org/releases/mozilla-beta/rev/34030b2a1cd430e080d5b2a0df5e8f954ccb727e
https://hg.mozilla.org/releases/mozilla-esr45/rev/2ce94f2ea79743e4f5b63840514e47f68b65b236

Comment 13

[Kamil Jozwiak [:kjozwiak]]

Jan, I couldn't reproduce the crash using the shell test from comment#0 with a version of m-c [1] from the beginning of the month. However, I can reproduce the assertion using the shell test from comment#7. I went through bug#1308800 but it doesn't seem like anyone managed to reproduce the crash reliably.

Regarding the first shell test, should I be running it under js with specific flags? Would running verifications against the second shell test be sufficient enough in this case?

[1] https://archive.mozilla.org/pub/firefox/nightly/2017/01/2017-01-01-03-02-04-mozilla-central/

Comment 14

[Jan de Mooij [:jandem]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #13)
> Regarding the first shell test, should I be running it under js with
> specific flags? Would running verifications against the second shell test be
> sufficient enough in this case?

It's possible you need a debug shell build for the first test to crash. But yes, testing the second shell test should be sufficient - it's the same bug, just slightly different ways to expose it.

Regarding bug 1308800, I was able to reproduce the FB crash reliably without the patch and I could no longer reproduce it with the patch applied. It was a topcrash and the number of crashes dropped to zero after the patch landed, so I think we can be confident this is fixed.

Comment 15

[Jan de Mooij [:jandem]]

Landed a jit-test for this:

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

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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