2034837 - Ion-inline WeakMapGetObject skips MarkSymbolForWeakMapReadBarrier

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

In bug 2029295 we added a MarkSymbolForWeakMapReadBarrier call for values returned by WeakMap.prototype.get, but the same issue also affects the Ion code for Weakmap.prototype.get(object):

// More reliable with --no-threads.

// Object key, Symbol value. Ion-inline WeakMapGetObject skips MarkSymbolForWeakMapReadBarrier.
// After GC, the value Symbol is gray in the zone atom bitmap.
// Pass the Ion-returned Symbol to a native (Object.is) to trigger cx->check -> checkAtom crash.
function f(k) {
  return wm.get(k);
}
with ({}) {} // Don't Ion-compile.
let key = {};
let val = Symbol();
let wm = new WeakMap();
wm.set(key, val);
grayRoot()[0] = key;
key = val = undefined;
gc();
let r = grayRoot()[0];
for (let i = 0; i < 2000; i++) { // warm into Ion
  f({});
}
let sym = f(r);                        // Ion path: returns gray Symbol
Object.is(sym, sym);                   // native call → cx->check(args[0])

[3587998] Hit MOZ_CRASH(*** Atom is marked gray for zone 7404d880c000 at argument 0) at js/src/vm/JSContext-inl.h:111

Claude flagged this difference between the C++ code and the JIT code in an audit looking for these types of issues.

Comment 1

[Iain Ireland [:iain]]

Attachment: (secure)

I left a comment in ExposeGCThingToActiveJS, but didn't consider that valueReadBarrier could have code added to it. There's a comment there now.

We have to pass in the Zone because we can't read it off a symbol that lives in the atom zone.

Comment 2

[BugBot [:suhaib / :marco]]

:iain, since this bug is a regression, could you fill (if possible) the regressed_by field?
For more information, please visit BugBot documentation.

Comment 3

[BugBot [:suhaib / :marco]]

This bug has been marked as a regression. Setting status flag for Nightly to affected.

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 2034837: Add testcase r=jandem

Comment 5

[Iain Ireland [:iain]]

Comment on attachment 9573640 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Similar difficulty to bug 2029295, where we said "very difficult".
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: Beta + release. Yes.
• If not all supported branches, which bug introduced the flaw?: Bug 2000328
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: The patch should apply cleanly.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely (for the same reasons as bug 2029295)
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 9573640 [details]
(secure)

sec-approval+ to land now and request uplifts.

Comment 7

[Iain Ireland [:iain]]

Attachment: (secure)

I left a comment in ExposeGCThingToActiveJS, but didn't consider that valueReadBarrier could have code added to it. There's a comment there now.

We have to pass in the Zone because we can't read it off a symbol that lives in the atom zone.

Original Revision: https://phabricator.services.mozilla.com/D296441

Comment 8

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Possible crash / security vulnerability.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: yes
• Steps to reproduce for manual QE testing: None
• Risk associated with taking this patch: low
• Explanation of risk level: This is a simple fix.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 9

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined/Reason for urgency: Possible crash / security vulnerability.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: yes
• Steps to reproduce for manual QE testing: None
• Risk associated with taking this patch: low
• Explanation of risk level: This is a simple fix.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 10

[Iain Ireland [:iain]]

Attachment: (secure)

I left a comment in ExposeGCThingToActiveJS, but didn't consider that valueReadBarrier could have code added to it. There's a comment there now.

We have to pass in the Zone because we can't read it off a symbol that lives in the atom zone.

Original Revision: https://phabricator.services.mozilla.com/D296441

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/783dd8752bd2
https://hg.mozilla.org/integration/autoland/rev/6fdf3630ed98
Call MarkSymbolForWeakMapReadBarrier in JIT path r=jonco

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg-edge.mozilla.org/mozilla-central/rev/6fdf3630ed98

Comment 13

[Brindusa Tot, DTE]

We noticed that the "qe-verify+" label was added on this bug. Can this issue be verified manually? If so, could you please provide specific steps for manual verification?

Comment 14

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/16f882a02294
https://hg.mozilla.org/releases/mozilla-beta/rev/a2cd9648067a

Comment 15

[Iain Ireland [:iain]]

(In reply to Brindusa Tot, DTE from comment #13)

We noticed that the "qe-verify+" label was added on this bug. Can this issue be verified manually? If so, could you please provide specific steps for manual verification?

Sorry, that was a misclick. Unsetting it.

Comment 16

[Dianna Smith [:diannaS]]

:iain should we wait to uplift bug 2036256 as well?

Comment 17

[Iain Ireland [:iain]]

Yes, that's probably best.

Comment 18

[Iain Ireland [:iain]]

I've queued bug 2036256 for landing. My beta uplift request failed because this patch hasn't been uplifted yet. It should apply cleanly on top of this patch. Please include that patch when you uplift this one. I filled in the uplift request form manually on Bugzilla.

Comment 19

[Dianna Smith [:diannaS]]

:iain it shouldnt have failed for beta because this patch is already in beta. For release, i can apply it once i land this, but i worry that the beta one itself needs a rebased patch for bug 2036256.

Comment 20

[Iain Ireland [:iain]]

Hmm. That surprises me. The patch is very local, and nothing else should have touched this code. Please let me know if anything goes wrong with the uplift.

Comment 21

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/3f3fd5d1de36
https://hg.mozilla.org/releases/mozilla-release/rev/037676792007

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/f76268fec3bf
https://hg.mozilla.org/integration/autoland/rev/f412460fc241
Add testcase r=jandem

Comment 23

[Norisz Fay [:noriszfay]]

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