Closed Bug 1037666 Opened 6 years ago Closed 6 years ago

Crash [@ js::BarrieredBase<js::types::TypeObject*>::operator->] or Crash [@ js::ScriptSourceObject::initFromOptions] with offThread script

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla34
Tracking Status
firefox31 --- wontfix
firefox32 + fixed
firefox33 + fixed
firefox34 --- fixed
firefox-esr24 --- unaffected
firefox-esr31 32+ fixed
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: decoder, Assigned: bhackett1024)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update,bisect][adv-main32+][adv-esr31.1+])

Crash Data

Attachments

(2 files)

The following testcase crashes on mozilla-central revision e1a037c085d1 (threadsafe build, run with --fuzzing-safe --thread-count=2 --ion-eager):


var g = newGlobal();
gczeal(2);
offThreadCompileScript('Error()');
g.runOffThreadScript();
Crash trace:


Program received signal SIGSEGV, Segmentation fault.
js::ScriptSourceObject::initFromOptions (cx=0x1648c50, source=(js::ScriptSourceObject * const) 0x4b4b4b4b4b4b4b4b Cannot access memory at address 0x4b4b4b4b4b4b4b4b, options=...) at /srv/repos/mozilla-central/js/src/jsscript.cpp:1313
1313        source->setReservedSlot(ELEMENT_SLOT, element);
#0  js::ScriptSourceObject::initFromOptions (cx=0x1648c50, source=(js::ScriptSourceObject * const) 0x4b4b4b4b4b4b4b4b Cannot access memory at address 0x4b4b4b4b4b4b4b4b, options=...) at /srv/repos/mozilla-central/js/src/jsscript.cpp:1313
#1  0x000000000084906e in finish (cx=0x1648c50, this=0x1794710) at /srv/repos/mozilla-central/js/src/vm/HelperThreads.cpp:215
#2  js::GlobalHelperThreadState::finishParseTask (this=<optimized out>, maybecx=0x1648c50, rt=0x162c2d0, token=<optimized out>) at /srv/repos/mozilla-central/js/src/vm/HelperThreads.cpp:801
#3  0x0000000000758ab8 in JS::FinishOffThreadScript (maybecx=0x1648c50, rt=<optimized out>, token=<optimized out>) at /srv/repos/mozilla-central/js/src/jsapi.cpp:4593
#4  0x000000000040b5f6 in runOffThreadScript (cx=0x1648c50, argc=<optimized out>, vp=0x7fffffffc8d8) at /srv/repos/mozilla-central/js/src/shell/js.cpp:3845
#5  0x0000000000846e52 in CallJSNative (args=..., native=0x40b570 <runOffThreadScript(JSContext*, unsigned int, jsval*)>, cx=0x1648c50) at /srv/repos/mozilla-central/js/src/jscntxtinlines.h:230
#6  js::Invoke (cx=0x1648c50, args=..., construct=<optimized out>) at /srv/repos/mozilla-central/js/src/vm/Interpreter.cpp:461
#7  0x0000000000847cdb in js::Invoke (cx=0x1648c50, thisv=..., fval=..., argc=0, argv=<optimized out>, rval=...) at /srv/repos/mozilla-central/js/src/vm/Interpreter.cpp:517
rax     0x1     1
rbx     0x1648c50       23366736
rcx     0x0     -1266637395197952
rdx     0x0     0
rsi     0x1648c50       23366736
rdi     0x178bc50       24689744
rbp     0x1794718       24725272
rsp     0xffffc0a0      140737488339104
r8      0x1000  4096
r9      0x40    64
r10     0x38    56
r11     0x206   518
r12     0x1648cb8       23366840
r13     0xffffc1d0      140737488339408
r14     0x38    56
r15     0x4b4b4b4b      5425512962855750475
rip     0x78db34 <js::ScriptSourceObject::initFromOptions(JSContext*, JS::Handle<js::ScriptSourceObject*>, JS::ReadOnlyCompileOptions const&)+196>
=> 0x78db34 <js::ScriptSourceObject::initFromOptions(JSContext*, JS::Handle<js::ScriptSourceObject*>, JS::ReadOnlyCompileOptions const&)+196>:  mov    (%r15),%rax


Looks like a use-after-free, marking sec-critical. If this is shell-only, please remove the security rating.
Crash Signature: [@ js::BarrieredBase<js::types::TypeObject*>::operator->] or Crash [@ js::ScriptSourceObject::initFromOptions] → [@ js::BarrieredBase<js::types::TypeObject*>::operator->] [@ js::ScriptSourceObject::initFromOptions]
Whiteboard: [jsbugmon:update,bisect]
Group: javascript-core-security
So what I see on that testcase is that in js::ParseTask::finish we have this->script dead and swept.

What's supposed to keep that alive?
Anything we can do to get this assigned to someone as a sec-critical?
Flags: needinfo?(jdemooij)
Off-thread parsing so forwarding to Brian.
Flags: needinfo?(jdemooij) → needinfo?(bhackett1024)
Attached patch patchSplinter Review
I think this is a regression from rev 4745f8a481a4 (bug 993438).  When finishing parse tasks we need to change references to builtin objects like Array.prototype from the parse compartment's object to the target compartment's object.  We used to fetch the target compartment's builtins purely, which couldn't GC, and now use a call which creates them on demand and can GC.  The original intent with off thread parsing was that we should finish compilations in the same compartment we triggered them in.  I guess that's been relaxed, or maybe it was never quite right to begin with (before 4745f8a481a4 we would null crash I guess if finishing a script in a compartment without all the right builtin objects constructed already.)

The script in a ParseTask is kept alive by the usedByExclusiveThread bit in its zone, which prevents the GC from being able to inspect or collect anything from that zone.  This bit is cleared immediately before the builtins are constructed, so the attached patch just reverses the order of these operations.
Assignee: nobody → bhackett1024
Attachment #8465514 - Flags: review?(wmccloskey)
Flags: needinfo?(bhackett1024)
Comment on attachment 8465514 [details] [diff] [review]
patch

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

Oops, sorry about that.
Attachment #8465514 - Flags: review?(wmccloskey) → review+
Comment on attachment 8465514 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 993438
User impact if declined: GC crash hazard
Risk to taking this patch (and alternatives if risky): none

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

31+

If not all supported branches, which bug introduced the flaw?

bug 993438

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

trivial

How likely is this patch to cause regressions; how much testing does it need?

none
Attachment #8465514 - Flags: sec-approval?
Attachment #8465514 - Flags: approval-mozilla-esr31?
Attachment #8465514 - Flags: approval-mozilla-beta?
Attachment #8465514 - Flags: approval-mozilla-b2g32?
Attachment #8465514 - Flags: approval-mozilla-aurora?
Comment on attachment 8465514 [details] [diff] [review]
patch

beta32 merges to b2g32 during this cycle
Attachment #8465514 - Flags: approval-mozilla-b2g32?
Waiting on sec-approval before approving for uplift.
Comment on attachment 8465514 [details] [diff] [review]
patch

Approvals given. Go and sin no more.
Attachment #8465514 - Flags: sec-approval?
Attachment #8465514 - Flags: sec-approval+
Attachment #8465514 - Flags: approval-mozilla-esr31?
Attachment #8465514 - Flags: approval-mozilla-esr31+
Attachment #8465514 - Flags: approval-mozilla-beta?
Attachment #8465514 - Flags: approval-mozilla-beta+
Attachment #8465514 - Flags: approval-mozilla-aurora?
Attachment #8465514 - Flags: approval-mozilla-aurora+
Followup to also leave the parse task return on a premature error return path (otherwise, if we hit this path we could leak the contents of the parse task zone).

https://hg.mozilla.org/integration/mozilla-inbound/rev/e23798c63191
https://hg.mozilla.org/mozilla-central/rev/3f1102854110
https://hg.mozilla.org/mozilla-central/rev/e23798c63191
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c8cf25aa827
https://hg.mozilla.org/releases/mozilla-beta/rev/784c7fb4c431

I folded the follow-up patch into the original landing. Sorry if I jumped the gun on pushing to inbound :(. Also, besides needing to patch jsworkers.cpp, there's enough context differences on esr31 that I'd prefer you do the backport for that branch :)
The esr31 push is busted, please push a fix or I'll backout before EOD.

https://tbpl.mozilla.org/php/getParsedLog.php?id=45540943&tree=Mozilla-Esr31
Flags: needinfo?(bhackett1024)
Oops, sorry about that.

https://hg.mozilla.org/releases/mozilla-esr31/rev/db04c6b7a823
Flags: needinfo?(bhackett1024)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][adv-main32+][adv-esr31.1+]
Group: javascript-core-security
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.