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

VERIFIED FIXED in Firefox -esr52

Status

()

defect
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: jya, Assigned: shu)

Tracking

52 Branch
mozilla55
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox52 wontfix, firefox-esr5253+ fixed, firefox53blocking verified, firefox54 fixed, firefox55 verified)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

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.
Crash Signature: [@0x0 | InternalConstruct]
Crash Signature: [@0x0 | InternalConstruct] → @0x0 | InternalConstruct
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.
Crash Signature: @0x0 | InternalConstruct → [@0x0 | InternalConstruct]
Crash Signature: [@0x0 | InternalConstruct] → [@@0x0 | InternalConstruct]
2075 crashes in the past 7 days, majority on OS X...
[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?
Crash Signature: [@@0x0 | InternalConstruct] → [@ @0x0 | js::Construct] [@ @0x0 | js::Proxy::construct] [@ @0x0 | InternalConstruct]
Flags: needinfo?(jorge)
OS: Unspecified → All
Hardware: Unspecified → All
Summary: Crashes (null Deref) in ScriptedProxyHandler::construct → Crashes (null Deref) in ScriptedProxyHandler::construct with lastpass addon
Duplicate of this bug: 1354848
I used their support ticket to report this issue - we shall see if someone responds.
Crash Signature: [@ @0x0 | js::Construct] [@ @0x0 | js::Proxy::construct] [@ @0x0 | InternalConstruct] → [@ @0x0 | js::Construct] [@ @0x0 | js::Proxy::construct] [@ @0x0 | InternalConstruct] [@ @0x0 | <name omitted> | js::ScriptedProxyHandler::construct const]
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.
I've pinged the developer.
Flags: needinfo?(jorge)
I can track this for 53, but since it looks the same on 52 release, not a release blocking issue.
adding the linux signature to the list...
Crash Signature: [@ @0x0 | js::Construct] [@ @0x0 | js::Proxy::construct] [@ @0x0 | InternalConstruct] [@ @0x0 | <name omitted> | js::ScriptedProxyHandler::construct const] → [@ @0x0 | js::Construct] [@ @0x0 | js::Proxy::construct] [@ @0x0 | InternalConstruct] [@ @0x0 | <name omitted> | js::ScriptedProxyHandler::construct const] [@ @0x0 | _fini]
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);
Adding [@ @0x0 | js::FetchName<T>] which appears to be the ESR signature.
Crash Signature: [@ @0x0 | js::Construct] [@ @0x0 | js::Proxy::construct] [@ @0x0 | InternalConstruct] [@ @0x0 | <name omitted> | js::ScriptedProxyHandler::construct const] [@ @0x0 | _fini] → [@ @0x0 | js::Construct] [@ @0x0 | js::Proxy::construct] [@ @0x0 | InternalConstruct] [@ @0x0 | <name omitted> | js::ScriptedProxyHandler::construct const] [@ @0x0 | _fini] [@ @0x0 | js::FetchName<T>]
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...
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()
:jya, since you can reproduce this, could you try running a build with the attached patch and see if the crashes go away?
Flags: needinfo?(jyavenard)
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.
Attachment #8857718 - Flags: review?(evilpies) → review+
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.
Let's land this on m-c please as we may need to get it into a 53 build.
Flags: needinfo?(wkocher)
Assignee: nobody → shu
(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
Flags: needinfo?(jyavenard)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/ec0e2532c1b0
Preserve IsCallable and IsConstructor when nuking wrappers. r=evilpie, a=me
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.
Flags: needinfo?(wkocher)
[Tracking Requested - why for this release]:
(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?
Flags: needinfo?(contact)
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.
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/89b29e846791
Follow-up: Fix tests to check for nuked wrapper exception. r=me a=bustage
Marking this as a release blocker for 53. StefanG is going to do some testing and verification today.
Is there anything we could/should do in the extension (besides removing Proxy altogether) to avoid this crash for existing Firefox builds?
(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?
Thanks, that explanation helps a lot -- we'll take a look at what we can do here.
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 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.
Attachment #8857718 - Flags: approval-mozilla-release?
Attachment #8857718 - Flags: approval-mozilla-esr52?
Attachment #8857718 - Flags: approval-mozilla-aurora?
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.
Attachment #8857718 - Flags: approval-mozilla-release?
Attachment #8857718 - Flags: approval-mozilla-release+
Attachment #8857718 - Flags: approval-mozilla-aurora?
Attachment #8857718 - Flags: approval-mozilla-aurora+
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.
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
Flags: needinfo?(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.
Flags: needinfo?(shu) → needinfo?(jcoppeard)
(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.
Flags: qe-verify-
(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.
Flags: needinfo?(contact)
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.
Per comment 22 this is fixed in trunk.
Target Milestone: --- → mozilla55
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
(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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(wmccloskey)
Flags: needinfo?(shu)
Flags: needinfo?(lhenry)
I've made uplift request for that one patch over in bug 1318384.
Flags: needinfo?(shu)
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.
Flags: needinfo?(wmccloskey)
That's only really an issue for 54/55, though, right? For ESR52, we'd be no worse off?
(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.
Can we please move the follow-up patch to another bug for better tracking?
See Also: → 1356691
Comment on attachment 8858411 [details] [diff] [review]
Make DeadObjectProxies always IsConstructor and IsCallable to save on extra slot write barrier.

Moved to bug 1356691
Attachment #8858411 - Attachment is obsolete: true
Attachment #8858411 - Flags: review?(wmccloskey)
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.
(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 on attachment 8857718 [details] [diff] [review]
Preserve IsCallable and IsConstructor when nuking wrappers.

let's fix this crash with the lastpass addon in 52esr
Flags: needinfo?(lhenry)
Attachment #8857718 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
(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.
Flags: needinfo?(jcoppeard)
Duplicate of this bug: 1355212
You need to log in before you can comment on or make changes to this bug.