1332597 - Crash [@ MustSkipMarking<js::jit::JitCode*>] or Assertion failure: addr % CellSize == 0, at gc/Heap.h:1168 with Workers

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 96cb95af5304 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2):

gczeal(4)
evalInWorker(`
  gczeal(2,1)(/s/);
`)



Backtrace:

 received signal SIGSEGV, Segmentation fault.
#0  MustSkipMarking<js::jit::JitCode*> (gcmarker=0x7ffff34d1b68, thing=0xe5e5e5e5e5e5e5e5) at js/src/gc/Marking.cpp:757
#1  0x0000000000aade05 in DoMarking<JSString> (thing=0xe5e5e5e5e5e5e5e5, gcmarker=0x7ffff34d1b68) at js/src/gc/Marking.cpp:790
#2  DispatchToTracer<JSString*> (trc=trc@entry=0x7ffff34d1b68, thingp=thingp@entry=0x7ffff34ea580, name=name@entry=0xe62611 "RegExpShared source") at js/src/gc/Marking.cpp:672
#3  0x0000000000aade60 in js::TraceNullableEdge<JSAtom*> (trc=trc@entry=0x7ffff34d1b68, thingp=thingp@entry=0x7ffff34ea580, name=name@entry=0xe62611 "RegExpShared source") at js/src/gc/Marking.cpp:437
#4  0x000000000091e1da in js::RegExpShared::trace (trc=<optimized out>, this=<optimized out>) at js/src/vm/RegExpObject.cpp:963
#5  js::RegExpObject::trace (trc=0x7ffff34d1b68, obj=<optimized out>) at js/src/vm/RegExpObject.cpp:193
#6  0x0000000000aab947 in js::Class::doTrace (this=<optimized out>, obj=0x7fffeed130c0, trc=0x7ffff34d1b68) at dist/include/js/Class.h:770
#7  CallTraceHook<TraverseObjectFunctor, js::GCMarker*, JSObject*&> (check=CheckGeneration::DoChecks, obj=0x7fffeed130c0, trc=0x7ffff34d1b68, f=...) at js/src/gc/Marking.cpp:1521
#8  js::GCMarker::processMarkStackTop (budget=..., this=0x7ffff34d1b68) at js/src/gc/Marking.cpp:1736
#9  js::GCMarker::drainMarkStack (this=this@entry=0x7ffff34d1b68, budget=...) at js/src/gc/Marking.cpp:1567
#10 0x00000000007e279a in js::gc::GCRuntime::drainMarkStack (phase=js::gcstats::PHASE_MARK, sliceBudget=..., this=0x7ffff34cf970) at js/src/jsgc.cpp:5249
#11 js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff34cf970, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC, lock=...) at js/src/jsgc.cpp:5854
#12 0x00000000007e3953 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff34cf970, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6176
#13 0x00000000007e3dd6 in js::gc::GCRuntime::collect (this=this@entry=0x7ffff34cf970, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6320
#14 0x00000000007e417b in js::gc::GCRuntime::gc (this=0x7ffff34cf970, gckind=<optimized out>, reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6381
#15 0x00000000007e45cf in js::gc::GCRuntime::runDebugGC (this=this@entry=0x7ffff34cf970) at js/src/jsgc.cpp:6792
#16 0x0000000000a9f6a8 in js::gc::GCRuntime::gcIfNeededPerAllocation (this=0x7ffff34cf970, cx=0x7ffff34cf000) at js/src/gc/Allocator.cpp:231
#17 0x0000000000aaf6b1 in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=0x7ffff34cf970, cx=<optimized out>, kind=<optimized out>) at js/src/gc/Allocator.cpp:192
#18 0x0000000000ab6f1a in js::Allocate<JSString, (js::AllowGC)1> (cx=cx@entry=0x7ffff34cf000) at js/src/gc/Allocator.cpp:143
#19 0x000000000096f9cf in JSThinInlineString::new_<(js::AllowGC)1> (cx=0x7ffff34cf000) at js/src/vm/String-inl.h:258
#20 js::AllocateInlineString<(js::AllowGC)1, unsigned char> (chars=<synthetic pointer>, len=<optimized out>, cx=0x7ffff34cf000) at js/src/vm/String-inl.h:31
#21 js::NewInlineString<(js::AllowGC)1, unsigned char> (chars=..., cx=0x7ffff34cf000) at js/src/vm/String-inl.h:57
#22 js::NewStringCopyNDontDeflate<(js::AllowGC)1, unsigned char> (cx=cx@entry=0x7ffff34cf000, s=0x7ffff691e1a0 "<string>", n=8) at js/src/vm/String.cpp:1272
#23 0x000000000097020a in js::NewStringCopyN<(js::AllowGC)1, unsigned char> (cx=cx@entry=0x7ffff34cf000, s=<optimized out>, n=<optimized out>) at js/src/vm/String.cpp:1322
#24 0x00000000007358ec in js::NewStringCopyN<(js::AllowGC)1> (n=<optimized out>, s=<optimized out>, cx=0x7ffff34cf000) at js/src/vm/String.h:1267
#25 js::NewStringCopyZ<(js::AllowGC)1> (s=<optimized out>, cx=0x7ffff34cf000) at js/src/vm/String.h:1287
#26 JS_NewStringCopyZ (cx=cx@entry=0x7ffff34cf000, s=<optimized out>) at js/src/jsapi.cpp:5009
#27 0x00000000007922f2 in js::ErrorToException (cx=0x7ffff34cf000, reportp=0x7fffef0fe1b0, callback=<optimized out>, userRef=0x0) at js/src/jsexn.cpp:610
#28 0x000000000079a5f1 in js::ReportErrorNumberVA (cx=0x7ffff34cf000, flags=<optimized out>, callback=0x789140 <js::GetErrorMessage(void*, unsigned int)>, userRef=0x0, errorNumber=<optimized out>, argumentsType=argumentsType@entry=js::ArgumentsAreLatin1, ap=0x7fffef0fe250) at js/src/jscntxt.cpp:700
#29 0x0000000000737afc in JS_ReportErrorFlagsAndNumberLatin1 (cx=cx@entry=0x7ffff34cf000, flags=flags@entry=0, errorCallback=errorCallback@entry=0x789140 <js::GetErrorMessage(void*, unsigned int)>, userRef=userRef@entry=0x0, errorNumber=errorNumber@entry=10) at js/src/jsapi.cpp:5795
#30 0x000000000078f531 in js::ReportValueErrorFlags (cx=cx@entry=0x7ffff34cf000, flags=flags@entry=0, errorNumber=10, spindex=spindex@entry=-3, v=..., fallback=..., fallback@entry=..., arg1=0x0, arg2=0x0) at js/src/jscntxt.cpp:838
#31 0x00000000004db1b9 in js::ReportIsNotFunction (construct=<optimized out>, numToSkip=2, v=..., cx=0x7ffff34cf000) at js/src/vm/Interpreter.cpp:297
#32 js::InternalCallOrConstruct (cx=0x7ffff34cf000, args=..., construct=<optimized out>) at js/src/vm/Interpreter.cpp:440
#33 0x00000000004ccea1 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:511
#34 Interpret (cx=0x7ffff34cf000, state=...) at js/src/vm/Interpreter.cpp:2955
#35 0x00000000004daab6 in js::RunScript (cx=cx@entry=0x7ffff34cf000, state=...) at js/src/vm/Interpreter.cpp:406
#36 0x00000000004dd037 in js::ExecuteKernel (result=0x7fffef0fed80, evalInFrame=..., newTargetValue=..., envChainArg=..., script=..., cx=0x7ffff34cf000) at js/src/vm/Interpreter.cpp:687
#37 js::Execute (cx=cx@entry=0x7ffff34cf000, script=script@entry=..., envChainArg=..., rval=0x7fffef0fed80) at js/src/vm/Interpreter.cpp:720
#38 0x000000000072c8e8 in ExecuteScript (cx=cx@entry=0x7ffff34cf000, scope=..., scope@entry=..., script=script@entry=..., rval=rval@entry=0x7fffef0fed80) at js/src/jsapi.cpp:4410
#39 0x0000000000734905 in JS_ExecuteScript (cx=cx@entry=0x7ffff34cf000, scriptArg=scriptArg@entry=..., rval=rval@entry=...) at js/src/jsapi.cpp:4436
#40 0x0000000000452ccf in WorkerMain (arg=0x7ffff33e0de0) at js/src/shell/js.cpp:3374
#41 0x0000000000455b02 in js::detail::ThreadTrampoline<void (&)(void*), WorkerInput*&>::callMain<0ul> (this=0x7ffff691e160) at js/src/threading/Thread.h:234
#42 js::detail::ThreadTrampoline<void (&)(void*), WorkerInput*&>::Start (aPack=0x7ffff691e160) at js/src/threading/Thread.h:227
#43 0x00007ffff7bc16fa in start_thread (arg=0x7fffef0ff700) at pthread_create.c:333
#44 0x00007ffff6c38b5d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
rax	0x1	1
rbx	0x7ffff34d1b68	140737275304808
rcx	0x7ffff34cf200	140737275294208
rdx	0xe5e5e5e5e5e00000	-1880844493790380032
rsi	0xe5e5e5e5e5e5e5e5	-1880844493789993499
rdi	0x7ffff34d1b68	140737275304808
rbp	0x7fffef0fdbd0	140737204181968
rsp	0x7fffef0fdbb0	140737204181936
r8	0x7fffeedfc0c0	140737201029312
r9	0x10	16
r10	0x7ffff6a000d0	140737331069136
r11	0x246	582
r12	0xe5e5e5e5e5e5e5e5	-1880844493789993499
r13	0x7ffff34ea5e0	140737275405792
r14	0x7fffef0fdf00	140737204182784
r15	0x7ffff34d1b68	140737275304808
rip	0xa9c2a3 <MustSkipMarking<js::jit::JitCode*>(js::GCMarker*, js::jit::JitCode*)+19>
=> 0xa9c2a3 <MustSkipMarking<js::jit::JitCode*>(js::GCMarker*, js::jit::JitCode*)+19>:	cmp    %rcx,0xffff8(%rdx)
   0xa9c2aa <MustSkipMarking<js::jit::JitCode*>(js::GCMarker*, js::jit::JitCode*)+26>:	mov    %rsp,%rbp


Marking s-s and sec-critical because GC is involved and we are crashing on a 0xe5 pattern here.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1332597-is-marked

IsMarked() should report true for permanent atoms and well known symbols.

What was happening here is that IsMarked() was returning false for the regexp's source ('s') and the compiled code (the RegExpShared) was being thrown away.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8828809 [details] [diff] [review]
bug1332597-is-marked

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

Ouch.

Comment 3

[Christian Holler (:decoder)]

Marking as fuzzblocker because I see many crashes and asserts going through js::RegExpShared::trace but some unreduced versions of this bug lack this frame and crash in different ways. Please land this as soon as possible to unblock fuzzing and allow the triage of possibly unrelated other GC issues with workers. Thanks!

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8828809 [details] [diff] [review]
bug1332597-is-marked

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

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?

No.

Which older supported branches are affected by this flaw?

Everything back to FF 30.

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

Bug 964059.

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

Should be simple.

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

Unlikely.

Comment 6

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

sec-approval+ for trunk to check this in WITHOUT the test. We can't check a test in for a security issue like this until we ship the fix live.

Comment 7

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b522b449f3b05ddfb4aaf7396d9996931c6a242

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out because it apparently made spidermonkey tests fail like https://treeherder.mozilla.org/logviewer.html#?job_id=71245016&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/8024174951ded3bd6457407cca53bc81d43d1982

Comment 9

[Jon Coppeard (:jonco)]

Attachment: bug1332597-is-marked v2

The problem with the previous patch is that we do want IsMarked for permanent atoms to return false in the runtime that owns them when we are shutting down and collecting everything.

Here's a different approach.  In this patch we make IsMarked return true for things that live in another runtime.  Annoyingly this means we have to pass the runtime into this function which results in a bunch of plumbing elsewhere in the engine, but it's not too bad.

The risk if we don't do this is that a GC in the runtime containing shared things (like permanent atoms) will initially clear their mark bits resulting in a window where a GC in another runtime can call IsMarked and get the result false.  AFAICS this has always been a problem.

Comment 10

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8829891 [details] [diff] [review]
bug1332597-is-marked v2

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

::: js/src/gc/Marking.h
@@ +378,5 @@
>  /*** Liveness ***/
>  
>  template <typename T>
>  bool
> +IsMarkedUnbarriered(JSRuntime* rt, T* thingp);

This patch seems good, but can you add a comment here why liveness (or whether something is marked) is (now) dependent on a runtime?

Comment 11

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a07161f7ac777fdf05a0a1057324fefa0f1d5c2

Comment 12

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

Jon, can we get branch patches made and nominated?

Comment 13

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 52a34f9a6cf1).

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2a07161f7ac7

Comment 15

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 16

[Jon Coppeard (:jonco)]

Attachment: bug1332597-aurora

Approval Request Comment
[Feature/Bug causing the regression]: Bug 964059.
[User impact if declined]: Possible crash/security vulnerability.
[Is this code covered by automated tests?]: Test not landed yet.
[Has the fix been verified in Nightly?]: N/A.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is a simple change to detect IsMarked being called on a GC thing in a another runtime.
[String changes made/needed]: None.

Comment 17

[Jon Coppeard (:jonco)]

Attachment: bug1332597-beta

Approval Request Comment
[Feature/Bug causing the regression]: Bug 964059.
[User impact if declined]: Possible crash/security vulnerability.
[Is this code covered by automated tests?]: Test not landed yet.
[Has the fix been verified in Nightly?]: N/A.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is a simple change to detect IsMarked being called on a GC thing in a another runtime.
[String changes made/needed]: None.

Comment 18

[Jon Coppeard (:jonco)]

Attachment: bug1332597-esr45

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-crit bug.
User impact if declined: Possible crash / security vulnerability.
Fix Landed on Version: 54.
Risk to taking this patch (and alternatives if risky): Low.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 8830683 [details] [diff] [review]
bug1332597-aurora

sec-critical gc fix, aurora53+

Comment 20

[Julien Cristau [:jcristau]]

Comment on attachment 8830684 [details] [diff] [review]
bug1332597-beta

sec-crit gc fix, beta52+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/602af8c8550c
https://hg.mozilla.org/releases/mozilla-beta/rev/b90333b8250f

Comment 22

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx52
JSBugMon: This bug has been automatically verified fixed on Fx53

Comment 23

[Gerry Chang [:gchang]]

Comment on attachment 8830685 [details] [diff] [review]
bug1332597-esr45

Sec-crit gc fix. ESR45+.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr45/rev/fa3268a1147e