1843824 - PromiseObject::resolutionSite() can return dead wrappers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[André Bargull [:anba]]

PromiseObject::resolutionSite() can return dead wrappers, but callers don't seem to expect dead wrappers.

PromiseObject::resolutionSite() is returned from JS::GetPromiseResolutionSite(), so it's possible that this is also reproducible outside of the shell, therefore I'm marking this bug as sec-sensitive for now.

Test case:

var g = newGlobal({newCompartment: true})
g.outer = this;
g.eval(`
  // Create a new Promise in |outer| using new.target, but with
  // resolver functions in |g|.
  var resolvers;
  var p = Reflect.construct(Promise, [
    (resolve, reject) => {
      resolvers = {resolve, reject};
    }
  ], outer.Promise);

  resolvers.resolve({
    get then() {
      // Throw from the about to be nuked compartment.
      throw null;
    }
  });
`);

// Nuke CCWs, including the SavedFrame for the Promise resolution info.
nukeAllCCWs();

Crashes on release and in debug-mode asserts with:

Hit MOZ_CRASH(Invalid object. Dead wrapper?) at /home/andre/hg/mozilla-central/js/src/vm/JSObject.h:643

Stack:

#0  0x0000555557a36cec in JSObject::maybeUnwrapAs<js::SavedFrame>() (this=(JSObject *) 0x83625d66078 [object Proxy]) at /home/andre/hg/mozilla-central/js/src/vm/JSObject.h:643
#1  0x0000555557a19f0d in js::UnwrapSavedFrame(JSContext*, JSPrincipals*, JS::Handle<JSObject*>, JS::SavedFrameSelfHosted, bool&)
    (cx=0x7ffff7630100, principals=0x0, obj=(JSObject * const) 0x83625d66078 [object Proxy], selfHosted=JS::SavedFrameSelfHosted::Exclude, skippedAsync=@0x7fffffffd3ef: 170)
    at /home/andre/hg/mozilla-central/js/src/vm/SavedStacks.cpp:753
#2  0x0000555557a1bc19 in JS::BuildStackString(JSContext*, JSPrincipals*, JS::Handle<JSObject*>, JS::MutableHandle<JSString*>, unsigned long, js::StackFormat)
    (cx=0x7ffff7630100, principals=0x0, stack=(JSObject * const) 0x83625d66078 [object Proxy], stringp=0x0, indent=2, format=js::StackFormat::SpiderMonkey) at /home/andre/hg/mozilla-central/js/src/vm/SavedStacks.cpp:1055
#3  0x00005555573b2bf2 in PrintUnhandledRejection(JSContext*, JS::Handle<js::PromiseObject*>) (cx=0x7ffff7630100, promise=(js::PromiseObject * const) 0x3b5dce900ed8 [object Promise])
    at /home/andre/hg/mozilla-central/js/src/shell/js.cpp:10662
#4  0x000055555738b3f9 in ReportUnhandledRejections(JSContext*) (cx=0x7ffff7630100) at /home/andre/hg/mozilla-central/js/src/shell/js.cpp:10728
#5  0x0000555557379ed6 in Shell(JSContext*, js::cli::OptionParser*) (cx=0x7ffff7630100, op=0x7fffffffd9c0) at /home/andre/hg/mozilla-central/js/src/shell/js.cpp:10830
#6  0x0000555557374db8 in main(int, char**) (argc=2, argv=0x7fffffffdc28) at /home/andre/hg/mozilla-central/js/src/shell/js.cpp:11247

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1843824: Handle dead wrappers. r=iain!

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1843824: Add test case. r=iain!

Depends on D183714

Comment 3

[André Bargull [:anba]]

This affects all supported versions. The bug should be similar to bug 1518821. Jan mentioned in bug 1518821, comment #2 that wrapper nuking is only used for chrome => content and therefore it's unlikely that it can be triggered from content code.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:anba, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/980f0750db83
Handle dead wrappers. r=iain

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/980f0750db83

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:anba, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox117 to wontfix.

For more information, please visit BugBot documentation.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for ESR115 approval. It grafts cleanly.

Comment 9

[Iain Ireland [:iain]]

Comment on attachment 9344145 [details]
Bug 1843824: Handle dead wrappers. r=iain!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Sec-moderate bug involving wrapper nuking. It's unlikely to be triggerable from content code, but better safe than sorry.

Release management requested nomination. It grafts cleanly.

• User impact if declined: Dead wrappers could potentially be returned to callers that don't expect them.
• Fix Landed on Version: 118
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This just adds a check for dead wrappers.

Comment 10

[Pascal Chevrel:pascalc]

Comment on attachment 9344145 [details]
Bug 1843824: Handle dead wrappers. r=iain!

Approved for ESR 115.3, thanks.

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/1822ac63eb8d

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8fd46f35da92
Add test case. r=iain

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8fd46f35da92

Comment 14

[Daniel Veditz [:dveditz]]

Sorry for the burst of bugspam: filter on tinkling-glitter-filtrate
Adding reporter-external keyword to security bugs found by non-employees for accounting reasons