1354294 - Crashes (null Deref) in ScriptedProxyHandler::construct with lastpass addon

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jean-Yves Avenard [:jya]]

URL visited:
https://www.tripadvisor.com/Tourism-g187139-Corsica-Vacations.html
https://www.tripadvisor.com/Attraction_Review-g656497-d1755237-Reviews-Aiguilles_de_Bavella_Bavella_Needles-Zonza_Corse_du_Sud_Corsica.html

Crash report:
https://crash-stats.mozilla.com/report/index/baa0f00c-62e3-4af9-b507-02b352170406

Experiencing dozen of crashes in the course of 5 minutes. All while browsing trip advisor

Mac 10.11

All in the same location:
https://hg.mozilla.org/releases/mozilla-release/annotate/e81854d6ce91/js/src/proxy/ScriptedProxyHandler.cpp#l1187

Comment 1

[Jeff Walden [:Waldo]]

It would be super-helpful to know what precise pointer is being null-derefed in there (or of course to have STR).  The stack, surprisingly for this code, doesn't make it immediately obvious.  If we knew just that one fact, this would likely be instantly fixable.

Comment 2

[Jean-Yves Avenard [:jya]]

well, can't get the signature link to work:

https://crash-stats.mozilla.com/signature/?_sort=-date&signature=%400x0%20|%20InternalConstruct

1169 crashes in the past 7 days on 52.

Comment 3

[Jean-Yves Avenard [:jya]]

Happened to me 5 times in the course of 2 minutes running Nightly:

last one:
https://crash-stats.mozilla.com/report/index/3ddf9997-2264-4eb4-9d9b-ebd502170411

visiting: https://www.lifewire.com/understanding-compressed-memory-os-x-2260327

Comment 4

[Jean-Yves Avenard [:jya]]

2075 crashes in the past 7 days, majority on OS X...

Comment 5

[[:philipp]]

[Tracking Requested - why for this release]:
this seems to be correlated to the lastpass addon and their update versions 4.1.45a (april 6) and 4.1.45.3a (april 10). it's happening on different operating systems and is causing over 8% of all the browser crashes on release in the past 3 days...

jorge, can we do some outreach to the addon authors?

Comment 7

[Marcia Knous [:marcia]]

I used their support ticket to report this issue - we shall see if someone responds.

Comment 8

[Jean-Yves Avenard [:jya]]

I'm hoping that we shouldn't crash regardless of the add-on :)

I'll run a debug version with lastpass installed and see if I can reproduce it and provide a better backtrace.

Comment 9

[Jorge Villalobos [:jorgev] (he/him)]

I've pinged the developer.

Comment 10

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

I can track this for 53, but since it looks the same on 52 release, not a release blocking issue.

Comment 11

[[:philipp]]

adding the linux signature to the list...

Comment 12

[Bob Copeland]

Lastpass developer here, major change between 4.1.44 and 4.1.45 is incorporation of a proxy object in the content script in order to isolate the CS from window properties (see below).  Crash happens with or without a binary component.

 (function (globalContext) {
    var hasOwnProperty = function(object, property) {
      if (object) {
        return Object.prototype.hasOwnProperty.call(object, property) || object.hasOwnProperty(property);
      }
      return false;
    };
    var isGlobalProperty = function(property) {
      var value = globalContext[property];
      if (hasOwnProperty(globalContext, property)) {
          return !(value instanceof Element || value instanceof HTMLCollection) || Object.getOwnPropertyNames(globalContext).includes(property);
      }
      else {
        return (typeof(EventTarget) !== 'undefined' && hasOwnProperty(EventTarget.prototype, property)) ||
               (typeof(ContentScriptGlobalScope) !== 'undefined' && hasOwnProperty(ContentScriptGlobalScope.prototype, property));
      }
    };
    var proxiedFunctions = Object.create(null);
    var proxy = new Proxy(Object.create(null), {
        get: function (target, property, receiver) {
            var isProxiedFunction = Object.prototype.hasOwnProperty.call(proxiedFunctions, property);

            if (property === Symbol.unscopables || !(isGlobalProperty(property) || isProxiedFunction))
                return void 0;

            var value = isProxiedFunction ? proxiedFunctions[property] : globalContext[property];

            if (!isProxiedFunction && typeof(value) === 'function') {
                value = proxiedFunctions[property] = new Proxy(value, {
                    apply: function (target, thisArg, argumentsList) {
                        return target.apply(thisArg === proxy ? globalContext : thisArg, argumentsList);
                    }
                });
            }

            return value;
        },
        set: function (target, property, value) {
            globalContext[property] = value;
            delete proxiedFunctions[property];
        },
        has: function () {
            return true;
        }
    });
    with (proxy) {

       // ... content script here ...

    })(this);

Comment 13

[Marcia Knous [:marcia]]

Adding [@ @0x0 | js::FetchName<T>] which appears to be the ESR signature.

Comment 14

[Shu-yu Guo [:shu]]

The thing that seems to be null is the JSNative constructHook, at the end of InternalConstruct. So the question is what kind of proxy target is being put in there that is IsConstructor but has a null construct hook...

Comment 15

[Shu-yu Guo [:shu]]

IRC convo with :evilpie has netted the following hypothesis, which sounds likely.

ScriptedProxyHandler pre-computes whether its target object IsConstructor at proxy construction time. However, if the target itself is a Proxy, the target may be recomputed to a DeadObjectProxy during the lifetime of the outer proxy. When we go to construct the outer proxy, it still thinks that its target IsConstructor. While DeadObjectProxy has both a call and a constructor hook (both just throw and report access to dead objects), it does not override BaseProxyHandler::isConstructor to return false. So, attempt to call JSObject::constructHook() on a DeadObjectProxy object will return nullptr, causing this crash here.

:evilpie also has a minimal reproducible test below.

  var global = newGlobal()
  var fun = global.eval("(function() {})")
  var p = new Proxy(fun, {})
  nukeCCW(fun);
  new p()

Comment 16

[Shu-yu Guo [:shu]]

Attachment: Preserve IsCallable and IsConstructor when nuking wrappers.

Comment 17

[Shu-yu Guo [:shu]]

:jya, since you can reproduce this, could you try running a build with the attached patch and see if the crashes go away?

Comment 18

[Tom S [:evilpie]]

Comment on attachment 8857718 [details] [diff] [review]
Preserve IsCallable and IsConstructor when nuking wrappers.

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

Nice.

::: js/src/builtin/TestingFunctions.cpp
@@ +4306,5 @@
>  }
>  
> +static bool
> +IsConstructor(JSContext* cx, unsigned argc, Value* vp)
> +{

Useful!

::: js/src/vm/ProxyObject.cpp
@@ +120,5 @@
> +    uint32_t callable = handler()->isCallable(this)
> +                        ? ScriptedProxyHandler::IS_CALLABLE : 0;
> +    uint32_t constructor = handler()->isConstructor(this)
> +                           ? ScriptedProxyHandler::IS_CONSTRUCTOR : 0;
> +    setExtra(ScriptedProxyHandler::IS_CALLCONSTRUCT_EXTRA,

Please add IS_CALLCONSTRUCT_EXTRA = 0 to dead DeadObjectProxy instead.

Comment 19

[Vincent Morel]

I'm taking the liberty comment here because I have a very consistent way of reproducing the crash.

It work on both my home (W7) and work (W10) computers.
Steps (Firefox = 52.0.2 - Lastpass >= 4.1.45a):
 - Go to https://i18n.myopenbee.com
 - Press TAB to select login field and enter something like "test"
 - Press TAB again to select password field and enter something like "test"
 - Press ENTER
 - Ta-da: https://bugzilla.mozilla.org/show_bug.cgi?id=1354294
Notes:
 - Don't use the mouse, it work much better with the keyboard.
 - You don't need to be logged in to Lastpass

Just now when writing these steps, I found out the key point look linked to pressing ENTER in the password field, even if it's empty.

Comment 20

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Let's land this on m-c please as we may need to get it into a 53 build.

Comment 21

[Jean-Yves Avenard [:jya]]

(In reply to Vincent Morel from comment #19)
> I'm taking the liberty comment here because I have a very consistent way of
> reproducing the crash.
> 
> It work on both my home (W7) and work (W10) computers.
> Steps (Firefox = 52.0.2 - Lastpass >= 4.1.45a):
>  - Go to https://i18n.myopenbee.com
>  - Press TAB to select login field and enter something like "test"
>  - Press TAB again to select password field and enter something like "test"
>  - Press ENTER


Unfortunately, I can't reproduce the crash with those steps.

It's very intermittent for me (using 55.0a1 (2017-04-13) (64-bit))... It hasn't occurred in the past 2 days

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/ec0e2532c1b0
Preserve IsCallable and IsConstructor when nuking wrappers. r=evilpie, a=me

Comment 23

[Ryan VanderMeulen [:RyanVM]]

We'll have dep builds from comment 22 within a couple hours. I'll post links to them here once they're ready. It would be fantastic if people who can reproduce the crash reliably could test it out.

Comment 24

[Julien Cristau [:jcristau]]

[Tracking Requested - why for this release]:

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> We'll have dep builds from comment 22 within a couple hours. I'll post links
> to them here once they're ready. It would be fantastic if people who can
> reproduce the crash reliably could test it out.

Win32 build containing this patch:
https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-win32/1492095983/firefox-55.0a1.en-US.win32.zip

Win64 build containing this patch:
https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-win64/1492095983/firefox-55.0a1.en-US.win64.zip

Happy to provide links for other platforms as well if people want to test on something other than Windows.

Vincent, can you see if this build works without crashing?

Comment 26

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

I looked through the crash signatures listed here and didn't see any big problems with Fennec. So maybe we don't need to re-spin the Fennec 53 RC for this issue. We probably should do 53 RC build 6, and another ESR52 build, though.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/89b29e846791
Follow-up: Fix tests to check for nuked wrapper exception. r=me a=bustage

Comment 28

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Marking this as a release blocker for 53. StefanG is going to do some testing and verification today.

Comment 29

[Bob Copeland]

Is there anything we could/should do in the extension (besides removing Proxy altogether) to avoid this crash for existing Firefox builds?

Comment 30

[Shu-yu Guo [:shu]]

(In reply to Bob Copeland from comment #29)
> Is there anything we could/should do in the extension (besides removing
> Proxy altogether) to avoid this crash for existing Firefox builds?

If the suspected bug being fixed in this bug is the actual bug, that means the addon is doing 2 things:

1) Putting a cross-compartment wrapper as the target of a Proxy. Meaning, a value was retrieved from a different global than the global the add-on is in, and was made as a target of the Proxy. Let's call this target T, and let's call the add-on created Proxy instance P.

2) P is being kept alive by the addon even after the global that T came from died, due to tab closure, navigating away, or whatever. Whenever this happens, Firefox replaces T with a "DeadObjectProxy" that throws a TypeError when it's touched in any way. The bug was that it was *supposed* to throw during construction too, but in the special case of being used as the target of a Proxy, it crashed.


Usually 2) is a bug. Is it possible to not keep strong references to things from other globals beyond those globals' lifetimes?

Comment 31

[Bob Copeland]

Thanks, that explanation helps a lot -- we'll take a look at what we can do here.

Comment 32

[Stefan [:StefanG_QA]]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0 (20170413030227)

I was able to reproduce the crash on Windows 10 and Mac OS El Capitan with the latest Nightly build only with installed LastPass (4.1.45a). After a short browser session I've experience a tab crashing on the TripAdvisor links from comment 0 and on the link from comment 3.

I've tested the both builds from comment 25. I did not experience any tabs crashing on Windows 10 with installed LastPass (4.1.45a) during my 20 minutes session on each browser. Everything is working fine for me.

Comment 33

[Shu-yu Guo [:shu]]

Comment on attachment 8857718 [details] [diff] [review]
Preserve IsCallable and IsConstructor when nuking wrappers.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Non-security sensitive crashes when using Proxies with cross-compartment wrappers that get nuked
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Low, bug fix.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/Bug causing the regression]: Probably bug 929467, been around a while.
[User impact if declined]: See above.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes, see comments in this bug.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No, see above.
[Why is the change risky/not risky?]: See above.
[String changes made/needed]: None.

Comment 34

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8857718 [details] [diff] [review]
Preserve IsCallable and IsConstructor when nuking wrappers.

Huge crash (browser and content), fix verified on m-c, let's uplift this for 53. I don't think it affects fennec.

Comment 35

[Stefan [:StefanG_QA]]

Mozilla/5.0(Macintosh; Intel Mac OS X 10.11; rv:55.0)Gecko/20100101 Firefox/55.0 (20170413130423)

On latest Nightly I've experienced a tabs crashes within 2-3 minutes of browsing with installed LastPass 4.1.45a.

I've tested the Mac OS build from comment 25. No crash is observed on any page during 20 minutes session.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/21484939216b

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/e0ac94302b32

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/176ddff87aeb

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Backed out from ESR52 for bustage. Shu, please land this on ESR52 once rebased so we can get the 52.1 respin going ASAP. Thanks!
https://hg.mozilla.org/releases/mozilla-esr52/rev/329812e9557c

https://treeherder.mozilla.org/logviewer.html#?job_id=91525226&repo=mozilla-esr52

Comment 40

[Shu-yu Guo [:shu]]

Ryan, looks like https://hg.mozilla.org/mozilla-central/rev/ace95a181f29d8bafa390026d82b725103cc00d0 needs to be uplifted as well for this patch to apply and work on ESR52. I'll NI :jonco.

Comment 41

[Andrei Vaida [:avaida]]

(In reply to Shu-yu Guo [:shu] from comment #33)
> [Is this code covered by automated tests?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Shy-yu Guo's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 42

[Vincent Morel]

(In reply to Jean-Yves Avenard [:jya] from comment #21)
> (In reply to Vincent Morel from comment #19)
> > I'm taking the liberty comment here because I have a very consistent way of
> > reproducing the crash.
> > 
> > It work on both my home (W7) and work (W10) computers.
> > Steps (Firefox = 52.0.2 - Lastpass >= 4.1.45a):
> >  - Go to https://i18n.myopenbee.com
> >  - Press TAB to select login field and enter something like "test"
> >  - Press TAB again to select password field and enter something like "test"
> >  - Press ENTER
> 
> 
> Unfortunately, I can't reproduce the crash with those steps.
> 
> It's very intermittent for me (using 55.0a1 (2017-04-13) (64-bit))... It
> hasn't occurred in the past 2 days

I'm really sorry my steps didn't work... also I just saw I made a link to this page instead of a crash reports.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #25)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> > We'll have dep builds from comment 22 within a couple hours. I'll post links
> > to them here once they're ready. It would be fantastic if people who can
> > reproduce the crash reliably could test it out.
> 
> Win32 build containing this patch:
> https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-
> win32/1492095983/firefox-55.0a1.en-US.win32.zip
> 
> Win64 build containing this patch:
> https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-
> win64/1492095983/firefox-55.0a1.en-US.win64.zip
> 
> Happy to provide links for other platforms as well if people want to test on
> something other than Windows.
> 
> Vincent, can you see if this build works without crashing?

I did some tests with this win32 build on my work computer (W10) and wasn't able to make it crash again following the steps from before. I tried for around 10min.

Comment 43

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to reproduce the crash on Firefox 55.0a1(2017-04-06), under Ubuntu 16.04x64.
The crash is no longer reproducible on Firefox 53.0(20170414022702).
Tests were performed under Mac OS X 10.12, Windows 10x64, Windows 7x86, Ubuntu 16.04x64.

Comment 44

[Julien Cristau [:jcristau]]

Per comment 22 this is fixed in trunk.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

(In reply to Shu-yu Guo [:shu] from comment #40)
> Ryan, looks like
> https://hg.mozilla.org/mozilla-central/rev/
> ace95a181f29d8bafa390026d82b725103cc00d0 needs to be uplifted as well for
> this patch to apply and work on ESR52. I'll NI :jonco.

I'm not sure Jon is around today or Monday given the Easter holidays, so we may need someone else to sign off on uplifting this extra changeset. In the mean time, here's a Try push for the hopeful ESR52 uplift:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26b0a30bcf3850b24781dd490a8b0b7aa157aaf0

It's looking good so far, so if someone wants to give the go-ahead on pushing this to ESR52, I'm happy to do so once the run finishes.

Comment 46

[Shu-yu Guo [:shu]]

I've made uplift request for that one patch over in bug 1318384.

Comment 47

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It seems fine to uplift the patch in bug 1318384. However, I'm worried that the patch here may regress the work in bug 1318384. The idea of the nuking patch there was to avoid setting the proxy extra slot, since that could invoke a write barrier that would keep a compartment alive longer than needed. Now we're modifying the extra slot again, so presumably we're invoking the write barrier.

Let's keep the needinfo on Jon to see what he thinks about this.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

That's only really an issue for 54/55, though, right? For ESR52, we'd be no worse off?

Comment 49

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #48)
> That's only really an issue for 54/55, though, right? For ESR52, we'd be no
> worse off?

Correct.

Comment 50

[Shu-yu Guo [:shu]]

Attachment: Make DeadObjectProxies always IsConstructor and IsCallable to save on extra slot write barrier.

Comment 51

[Shu-yu Guo [:shu]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9038156a6c360934b5a3291c36f23d744e77c3cf for patch above.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

Can we please move the follow-up patch to another bug for better tracking?

Comment 53

[Shu-yu Guo [:shu]]

Comment on attachment 8858411 [details] [diff] [review]
Make DeadObjectProxies always IsConstructor and IsCallable to save on extra slot write barrier.

Moved to bug 1356691

Comment 54

[void2258]

I am told that the frequent crashing issue I am having (ex: ebad0d57-ae2d-4f6a-93b4-8721e2170415) is correlated to this bug, which would mean it is not fixed yet.

Comment 55

[Jean-Yves Avenard [:jya]]

(In reply to void2258 from comment #54)
> I am told that the frequent crashing issue I am having (ex:
> ebad0d57-ae2d-4f6a-93b4-8721e2170415) is correlated to this bug, which would
> mean it is not fixed yet.

You're running 52.0.2 still, it doesn't have the fix.

Comment 56

[Julien Cristau [:jcristau]]

Comment on attachment 8857718 [details] [diff] [review]
Preserve IsCallable and IsConstructor when nuking wrappers.

let's fix this crash with the lastpass addon in 52esr

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/9e17e0266d21
https://hg.mozilla.org/releases/mozilla-esr52/rev/3ea0e0752031

Comment 58

[Jon Coppeard (:jonco) (PTO until 3rd April)]

(In reply to Shu-yu Guo [:shu] from comment #40)
I'm fine for you to uplift that patch in bug 1318384.  Looks like you requested approval already so clearing needinfo.

Comment 60

[David Alfred]

(In reply to Jean-Yves Avenard [:jya] from comment #3)

Happened to me 5 times in the course of 2 minutes running Nightly:

last one:
https://crash-stats.mozilla.com/report/index/3ddf9997-2264-4eb4-9d9b-
ebd502170411

visiting:
https://www.lifewire.com/understanding-compressed-memory-os-x-2260327
http://roundtripholidays.com.ng/

Comment 61

[Vietnam Shore Excursions]

URL visited:
https://vietnam-shoreexcursions.com/

Comment 62

[Julien Cristau [:jcristau]]

Restricting comments on this old+resolved bug which is attracting spam.