1472734 - Assertion failure: js::CurrentThreadIsGCSweeping(), at js/src/gc/DeletePolicy.h:78 with OOM

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 23885c14f025 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe):

try {
    oomTest(function() {
      eval(`
        function eval(source) {
          offThreadCompileModule(source);
          minorgc();
        }
        eval("");
      `);
    });
} catch (exc) {}


Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000000000c871e1 in js::GCManagedDeletePolicy<js::ModuleScope::Data>::operator() (this=<optimized out>, constPtr=0x7ffff48ce070) at js/src/gc/DeletePolicy.h:78
#1  mozilla::UniquePtr<js::ModuleScope::Data, JS::DeletePolicy<js::ModuleScope::Data> >::reset (aPtr=0x0, this=0x7ffff5efc5e8) at dist/include/mozilla/UniquePtr.h:343
#2  mozilla::UniquePtr<js::ModuleScope::Data, JS::DeletePolicy<js::ModuleScope::Data> >::~UniquePtr (this=0x7ffff5efc5e8, __in_chrg=<optimized out>) at dist/include/mozilla/UniquePtr.h:288
#3  js::DispatchWrapper<mozilla::UniquePtr<js::ModuleScope::Data, JS::DeletePolicy<js::ModuleScope::Data> > >::~DispatchWrapper (this=0x7ffff5efc5e0, __in_chrg=<optimized out>) at dist/include/js/RootingAPI.h:793
#4  JS::Rooted<mozilla::UniquePtr<js::ModuleScope::Data, JS::DeletePolicy<js::ModuleScope::Data> > >::~Rooted (this=0x7ffff5efc5d0, __in_chrg=<optimized out>) at dist/include/js/RootingAPI.h:1009
#5  js::ModuleScope::create (cx=0x7ffff5efe0b0, dataArg=..., dataArg@entry=..., module=..., module@entry=..., enclosing=..., enclosing@entry=...) at js/src/vm/Scope.cpp:1153
#6  0x0000000000eba631 in js::frontend::BytecodeEmitter::EmitterScope::<lambda(JSContext*, js::HandleScope)>::operator() (enclosing=..., cx=<optimized out>, __closure=<synthetic pointer>) at js/src/frontend/BytecodeEmitter.cpp:1377
#7  js::frontend::BytecodeEmitter::EmitterScope::internScope<js::frontend::BytecodeEmitter::EmitterScope::enterModule(js::frontend::BytecodeEmitter*, js::frontend::ModuleSharedContext*)::<lambda(JSContext*, js::HandleScope)> > (createScope=..., bce=0x7ffff5efce20, this=0x7ffff5efc7e0) at js/src/frontend/BytecodeEmitter.cpp:620
#8  js::frontend::BytecodeEmitter::EmitterScope::internBodyScope<js::frontend::BytecodeEmitter::EmitterScope::enterModule(js::frontend::BytecodeEmitter*, js::frontend::ModuleSharedContext*)::<lambda(JSContext*, js::HandleScope)> > (createScope=..., bce=0x7ffff5efce20, this=0x7ffff5efc7e0) at js/src/frontend/BytecodeEmitter.cpp:634
#9  js::frontend::BytecodeEmitter::EmitterScope::enterModule (this=0x7ffff5efc7e0, bce=0x7ffff5efce20, modulesc=0x7ffff5efc9b0) at js/src/frontend/BytecodeEmitter.cpp:1378
#10 0x0000000000ecf256 in js::frontend::BytecodeEmitter::emitScript (this=0x7ffff5efce20, body=body@entry=0x7ffff5ffb050) at js/src/frontend/BytecodeEmitter.cpp:5217
#11 0x0000000000ed7e19 in BytecodeCompiler::compileModule (this=this@entry=0x7ffff5efd350) at js/src/frontend/BytecodeCompiler.cpp:419
#12 0x0000000000ed8391 in js::frontend::CompileModule (cx=<optimized out>, optionsInput=..., srcBuf=..., alloc=..., sourceObjectOut=sourceObjectOut@entry=0x7ffff5efdef0) at js/src/frontend/BytecodeCompiler.cpp:712
#13 0x0000000000b8883b in js::ModuleParseTask::parse (this=0x7ffff4985c00, cx=<optimized out>) at js/src/vm/HelperThreads.cpp:520
#14 0x0000000000b84682 in js::HelperThread::handleParseWorkload (this=<optimized out>, locked=...) at js/src/vm/HelperThreads.cpp:2108
#15 0x0000000000b7336c in js::HelperThread::threadLoop (this=this@entry=0x7ffff5f05c00) at js/src/vm/HelperThreads.cpp:2397
#16 0x0000000000b73520 in js::HelperThread::ThreadMain (arg=0x7ffff5f05c00) at js/src/vm/HelperThreads.cpp:1891
#17 0x0000000000b96962 in js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::callMain<0ul> (this=0x7ffff5f18050) at js/src/threading/Thread.h:242
#18 js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::Start (aPack=0x7ffff5f18050) at js/src/threading/Thread.h:235
#19 0x00007ffff7bc16ba in start_thread (arg=0x7ffff5eff700) at pthread_create.c:333
#20 0x00007ffff6c383dd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:109
rax	0x0	0
rbx	0x7ffff48ce070	140737296261232
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7ffff5efc660	140737319519840
rsp	0x7ffff5efc5b0	140737319519664
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff5eff700	140737319532288
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x0	0
r13	0x7ffff5efe0b0	140737319526576
r14	0x7ffff5efce20	140737319521824
r15	0x7ffff5efc698	140737319519896
rip	0xc871e1 <js::ModuleScope::create(JSContext*, JS::Handle<js::ModuleScope::Data*>, JS::Handle<js::ModuleObject*>, JS::Handle<js::Scope*>)+641>
=> 0xc871e1 <js::ModuleScope::create(JSContext*, JS::Handle<js::ModuleScope::Data*>, JS::Handle<js::ModuleObject*>, JS::Handle<js::Scope*>)+641>:	movl   $0x0,0x0
   0xc871ec <js::ModuleScope::create(JSContext*, JS::Handle<js::ModuleScope::Data*>, JS::Handle<js::ModuleObject*>, JS::Handle<js::Scope*>)+652>:	ud2


Marking s-s because this GC assertion is known to be associated with security problems.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/461ae2080686
user:        Jon Coppeard
date:        Fri Jun 15 15:05:06 2018 -0700
summary:     Bug 1468867 - Move heap state to JSRuntime r=sfink

This iteration took 318.875 seconds to run.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1472734-delete-policy

What I didn't realise when reviewing bug 1466633 was that we can use GCManagedDeletePolicy off-thread so even if JS::CurrentThreadIsHeapCollecting() returns true we may still want to clear the edges.

I don't like this GCManagedDeletePolicy thing.  It's not intuitive and it's been a source of several bugs.  However I can't think of anything better right now.

This patch removes the heap state check and changes the assertion to allow ClearEdgesTracer during GC, to accommodate the situation when you delete something with this policy during sweeping.

Comment 3

[Julien Cristau [:jcristau]]

[Tracking Requested - why for this release]: new sec-high

Comment 4

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

Comment on attachment 8989396 [details] [diff] [review]
bug1472734-delete-policy

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

::: js/src/gc/DeletePolicy.h
@@ +73,5 @@
>      void operator()(const T* constPtr) {
>          if (constPtr) {
>              auto ptr = const_cast<T*>(constPtr);
> +            gc::ClearEdgesTracer trc;
> +            ptr->trace(&trc);

Ok, so if this is called from offthread, I agree that we won't see a nursery object.

But this does require that T::trace() be callable offthread; is that a known restriction?

It also means that whatever trace() does, like checking bitmaps and ShouldMark<T*>() and whatever else, needs to be doable offthread. That doesn't seem too bad, since iiuc all objects encountered should be in a zone that is exclusively for that thread's use at this point in time?

Still, would it be easier/safer to check if we're offthread, and skip tracing in that case?

I'm probably just not up to speed on the threading model here, since this is all pre-existing.

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #4)
> But this does require that T::trace() be callable offthread; is that a known
> restriction?

You should be able to call trace() on a T you own even if you are off thread.  But I guess that's never been stated explicitly.

> It also means that whatever trace() does, like checking bitmaps and
> ShouldMark<T*>() and whatever else, needs to be doable offthread. That
> doesn't seem too bad, since iiuc all objects encountered should be in a zone
> that is exclusively for that thread's use at this point in time?

Yes it does, and yes the thread should only be accessing things created in the zone which it has exclusive access to.

> Still, would it be easier/safer to check if we're offthread, and skip
> tracing in that case?

I'd like it to run the barriers because (at least in the pre-barrier case) we assert that it is safe to skip the barrier if we're off thread.  I'd like to assert this for the post barrier too.

Comment 6

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

Comment on attachment 8989396 [details] [diff] [review]
bug1472734-delete-policy

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

Thanks, that all makes sense.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8989396 [details] [diff] [review]
bug1472734-delete-policy

I'm not 100% sure this would cause any problems if it happened in the wild, but keeping the s-s classification just in case.

[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? FF 62.

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

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? This is a pretty simple change, so unlikely.

Comment 8

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

sec-approval+ for trunk. Can we get a beta patch nominated so we can fix it in all affected places?

Comment 9

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e72e7f5db9a668895f1081050e8ecb33a480de16

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e72e7f5db9a6

Comment 11

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 12

[Jon Coppeard (:jonco)]

Comment on attachment 8989396 [details] [diff] [review]
bug1472734-delete-policy

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1466633.
[User impact if declined]: Possible crash / security vulnerability.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[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 small change to revert to the previous behaviour plus relax an assertion.  The new codepath is covered by tests.
[String changes made/needed]: None.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8989396 [details] [diff] [review]
bug1472734-delete-policy

Crash fix, let's land it for beta 9 or 10.

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/be6250630b54

Comment 15

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx62