Closed Bug 1332597 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox-esr45 52+ fixed
firefox50 --- wontfix
firefox51 + wontfix
firefox52 + verified
firefox-esr52 52+ fixed
firefox53 + verified
firefox54 + verified

People

(Reporter: decoder, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(6 keywords, Whiteboard: [fuzzblocker] [jsbugmon:update,bisect,ignore][adv-main52+][adv-esr45.8+])

Crash Data

Attachments

(4 files, 1 obsolete file)

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.
Assignee: nobody → jcoppeard
Attached patch bug1332597-is-marked (obsolete) — Splinter Review
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.
Attachment #8828809 - Flags: review?(sphink)
Comment on attachment 8828809 [details] [diff] [review]
bug1332597-is-marked

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

Ouch.
Attachment #8828809 - Flags: review?(sphink) → review+
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!
Flags: needinfo?(jcoppeard)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][fuzzblocker]
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.
Flags: needinfo?(jcoppeard)
Attachment #8828809 - Flags: sec-approval?
Duplicate of this bug: 1332620
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.
Attachment #8828809 - Flags: sec-approval? → sec-approval+
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.
Attachment #8828809 - Attachment is obsolete: true
Flags: needinfo?(jcoppeard)
Attachment #8829891 - Flags: review?(sphink)
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?
Attachment #8829891 - Flags: review?(sphink) → review+
Jon, can we get branch patches made and nominated?
Flags: needinfo?(jcoppeard)
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update,bisect,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 52a34f9a6cf1).
https://hg.mozilla.org/mozilla-central/rev/2a07161f7ac7
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
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.
Flags: needinfo?(jcoppeard)
Attachment #8830683 - Flags: approval-mozilla-aurora?
Attached patch bug1332597-betaSplinter Review
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.
Attachment #8830684 - Flags: approval-mozilla-beta?
Attached patch bug1332597-esr45Splinter Review
[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.
Attachment #8830685 - Flags: approval-mozilla-esr45?
Comment on attachment 8830683 [details] [diff] [review]
bug1332597-aurora

sec-critical gc fix, aurora53+
Attachment #8830683 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8830684 [details] [diff] [review]
bug1332597-beta

sec-crit gc fix, beta52+
Attachment #8830684 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
JSBugMon: This bug has been automatically verified fixed on Fx52
JSBugMon: This bug has been automatically verified fixed on Fx53
Group: javascript-core-security → core-security-release
Comment on attachment 8830685 [details] [diff] [review]
bug1332597-esr45

Sec-crit gc fix. ESR45+.
Attachment #8830685 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Whiteboard: [fuzzblocker] [jsbugmon:update,bisect,ignore] → [fuzzblocker] [jsbugmon:update,bisect,ignore][adv-main52+][adv-esr45.8+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.