Fix RegExpShared read barriers (again)

RESOLVED FIXED in Firefox 32, Firefox OS v2.0

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

({csectype-uaf, sec-high})

Trunk
mozilla32
csectype-uaf, sec-high
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox30 unaffected, firefox31 unaffected, firefox32 fixed, firefox-esr24 unaffected, b2g-v1.2 unaffected, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 unaffected, b2g-v2.0 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8430211 [details] [diff] [review]
patch

I don't think bug 1012642 was the right fix for RegExpShared read barriers when accessed from objects.  That bug added a barrier when copying RegExpShared pointers from one RegExpObject to another, but no barrier is tripped when getting the RegExpShared from an object to actually execute the regexp.

In this case there will be a RegExpGuard on the stack, which will trace the RegExpShared during root marking but which won't do anything during sweep.  So we could run through these steps to destroy the RegExpShared while it is still in use.

- start an IGC outside the RegExpGuard
- create the RegExpGuard, get the RegExpShared
- trace the RegExpObject and clear the pointer to its RegExpShared
- sweep, with the RegExpGuard still on the stack

Before bug 1010441 the RegExpGuard prevented the RegExpShared from being destroyed during sweep, but didn't prevent its JitCode from being destroyed.  So this is really a regression from the initial irregexp landing, bug 976446, before which the RegExpShared owned its jitcode outright.

I'm hoping that fixing this will fix bug 1013686 and bug 1013589.
Attachment #8430211 - Flags: review?(wmccloskey)
Assignee: nobody → bhackett1024
Blocks: 976446
status-firefox31: --- → unaffected
status-firefox32: --- → affected
Keywords: csectype-uaf, sec-high
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Comment 1

3 years ago
Oops, that first bug should be bug 1013586.
Attachment #8430211 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 2

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9386b73250c
(Assignee)

Comment 3

3 years ago
https://hg.mozilla.org/mozilla-central/rev/b9386b73250c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
status-b2g-v1.2: --- → unaffected
status-b2g-v1.3: --- → unaffected
status-b2g-v1.3T: --- → unaffected
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → fixed
status-firefox30: --- → unaffected
status-firefox32: affected → fixed
status-firefox-esr24: --- → unaffected
Target Milestone: --- → mozilla32
Group: javascript-core-security → core-security
Brian, is there anything that can be done to verify this fix? I'm looking for a bug file or test. I've looked at the related bugs but I'm unsure what applies here.

If not, that's fine, just let me know. Thank you.
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 5

3 years ago
(In reply to Matt Wobensmith from comment #4)
> Brian, is there anything that can be done to verify this fix? I'm looking
> for a bug file or test. I've looked at the related bugs but I'm unsure what
> applies here.
> 
> If not, that's fine, just let me know. Thank you.

Not easily, I found this by code inspection and never wrote a testcase (which can be hard to do with GC timing stuff).
Flags: needinfo?(bhackett1024)
OK, thanks Brian. I'm going to go ahead and cross it off of our list for verification, based on this info. Much appreciated.
QA Whiteboard: qe-verify-
QA Whiteboard: qe-verify-
Flags: qe-verify-
Group: core-security
You need to log in before you can comment on or make changes to this bug.