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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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();

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Al Billings [:abillings - ex-MoCo]]

Anything we can do to get this assigned to someone as a sec-critical?

Comment 5

[Jan de Mooij [:jandem]]

Off-thread parsing so forwarding to Brian.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8465514 [details] [diff] [review]
patch

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

Oops, sorry about that.

Comment 8

[Brian Hackett [Laid off!]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8465514 [details] [diff] [review]
patch

beta32 merges to b2g32 during this cycle

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Waiting on sec-approval before approving for uplift.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8465514 [details] [diff] [review]
patch

Approvals given. Go and sin no more.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f1102854110

Comment 13

[Brian Hackett [Laid off!]]

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

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3f1102854110
https://hg.mozilla.org/mozilla-central/rev/e23798c63191

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/784c7fb4c431

Comment 17

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/22b3d83562de

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Brian Hackett [Laid off!]]

Oops, sorry about that.

https://hg.mozilla.org/releases/mozilla-esr31/rev/db04c6b7a823