Closed Bug 1342101 Opened 3 years ago Closed 3 years ago
Crash [@ js::Is
Derived Proxy Object] or Assertion failure: Current Thread Can Access Zone(zone), at gc/Heap .h:1262
24.31 KB, text/plain
2.43 KB, patch
|Details | Diff | Splinter Review|
The following testcase crashes on mozilla-central revision 32dcdde1fc64 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --thread-count=2 --disable-oom-functions --ion-extra-checks --ion-eager --ion-aa=flow-sensitive --ion-offthread-compile=off): See attachment. Backtrace: Program terminated with signal SIGSEGV, Segmentation fault. #0 0x000000000090ba58 in js::IsDerivedProxyObject (handler=0x1b81750 <(anonymous namespace)::DebugEnvironmentProxyHandler::singleton>, obj=obj@entry=0x7f95dfede730) at js/src/vm/ProxyObject.h:143 #1 JSObject::is<js::DebugEnvironmentProxy> (this=this@entry=0x7f95dfede730) at js/src/vm/EnvironmentObject.cpp:2218 #2 0x0000000000885fc6 in JSObject::enclosingEnvironment (this=<optimized out>) at js/src/vm/EnvironmentObject-inl.h:76 #3 js::jit::BaselineFrame::callObj (this=<optimized out>) at js/src/jit/BaselineFrame-inl.h:93 #4 js::AbstractFramePtr::callObj (this=<synthetic pointer>) at js/src/vm/Stack-inl.h:498 #5 js::ArgumentsObject::MaybeForwardToCallObject (frame=..., obj=0x7f95dfede748, data=data@entry=0x7f95dfede788) at js/src/vm/ArgumentsObject.cpp:89 #6 0x000000000088b5d4 in CopyFrameArgs::maybeForwardToCallObject (data=0x7f95dfede788, obj=<optimized out>, this=0x7ffde5b2ee70) at js/src/vm/ArgumentsObject.cpp:130 #7 js::ArgumentsObject::create<CopyFrameArgs> (cx=<optimized out>, callee=..., callee@entry=..., numActuals=<optimized out>, copy=...) at js/src/vm/ArgumentsObject.cpp:319 #8 0x0000000000887a9d in js::ArgumentsObject::createExpected (cx=<optimized out>, frame=...) at js/src/vm/ArgumentsObject.cpp:332 #9 0x00000000006ff0bc in js::jit::NewArgumentsObject (cx=<optimized out>, frame=<optimized out>, res=...) at js/src/jit/VMFunctions.cpp:920 #10 0x00002f647567880d in ?? () [...] #24 0x0000000000000000 in ?? () rax 0x0 0 rbx 0x1 1 rcx 0x1b81800 28841984 rdx 0xf6a6e90b74ff8548 -673594858828495544 rsi 0x7f95dfede748 140281683765064 rdi 0x7f95dfede730 140281683765040 rbp 0x7f95ded6bab0 140281665469104 rsp 0x7ffde5b2ecc8 140728457161928 r8 0x1 1 r9 0x3 3 r10 0x7f95dfede788 140281683765128 r11 0x0 0 r12 0x7f95dfede748 140281683765064 r13 0x7f95dfede730 140281683765040 r14 0x7f95dfede788 140281683765128 r15 0x1b81a60 28842592 rip 0x90ba58 <JSObject::is<js::DebugEnvironmentProxy>() const+8> => 0x90ba58 <JSObject::is<js::DebugEnvironmentProxy>() const+8>: testb $0x10,0xa(%rdx) 0x90ba5c <JSObject::is<js::DebugEnvironmentProxy>() const+12>: je 0x90ba6c <JSObject::is<js::DebugEnvironmentProxy>() const+28> This crash bug is highly intermittent, I recommend running at least 20 times with a timeout of 5 seconds per run. In a debug build, you get the assertion and it seems to be more reliable than the opt crash. The debug assertion points to bug 1337414 but the crash there looks different and the test is easier. So I'm not entirely convinced that this is a duplicate. The crash also looks sec-critical given totally garbled address, so we should check carefully if this is a dup or not.
NI for jandem.
I can reproduce the debug assertion failure. I don't get the opt crash unfortunately. Maybe it's caused by the same bug, not sure. * We clone a lazy self-hosted function. This ends up calling FunctionScope::clone. * The Scope::create call there fails (returns nullptr): Rooted<UniquePtr<Data>> dataClone(cx, CopyScopeData<FunctionScope>(cx, dataOriginal)); if (!dataClone) return nullptr; Scope* scopeClone= Scope::create(cx, scope->kind(), enclosing, envShape); if (!scopeClone) return nullptr; dataClone->canonicalFunction.init(fun); * We call the destructor of Rooted<UniquePtr<Data>>. This ends up calling GCManagedDeletePolicy::operator(). There we do: Zone* zone = ptr->zone(); ptr here is a FunctionScope::Data* and its canonicalFunction is in the self-hosting Zone so we assert. I think the bug is that FunctionScope::clone should do |dataClone->canonicalFunction.init(fun);| before the Scope::create call.
Flags: needinfo?(jdemooij) → needinfo?(shu)
Likely a regression from bug 1263355.
I am having trouble reproducing the crash.
I am now reproducing the crash. What a roller coaster.
I am not sure how to test this.
Attachment #8842242 - Flags: review?(jdemooij) → review+
Comment on attachment 8842242 [details] [diff] [review] Move canonicalFunction.init before Scope::create in case create fails. [Security approval request comment] How easily could an exploit be constructed based on the patch? Pretty difficult. I'm not sure if it's exploitable, but to trigger the specific crash would require controlling for a particular allocation to fail. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not in my opinion. Which older supported branches are affected by this flaw? All branches (aurora, beta, release). If not all supported branches, which bug introduced the flaw? All branches. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Patch should apply. How likely is this patch to cause regressions; how much testing does it need? Will not cause regressions.
Attachment #8842242 - Flags: sec-approval?
Approved for checkin on March 21, two weeks into the new cycle.
Attachment #8842242 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #10) > Approved for checkin on March 21, two weeks into the new cycle. Does this landing happen automatically, or should I push it on March 21?
You can do it yourself or else I tend to look for them and land when the time comes otherwise.
(In reply to Shu-yu Guo [:shu] from comment #12) > (In reply to Al Billings [:abillings] from comment #10) > > Approved for checkin on March 21, two weeks into the new cycle. > > Does this landing happen automatically, or should I push it on March 21? Add the checkin-needed keyword and The Great God Ryan will use his mojo and it will be checked in. :-)
https://hg.mozilla.org/integration/mozilla-inbound/rev/514a06ddb08ca52e552124f6d23d2f053c4f7e13 Please request Aurora/Beta/ESR52 approval on this as soon as possible.
Comment on attachment 8842242 [details] [diff] [review] Move canonicalFunction.init before Scope::create in case create fails. Approval Request Comment [Feature/Bug causing the regression]: bug 1263355 (aka frontend rewrite) [User impact if declined]: crashes if OOM happens at juuust the right place [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]: no [Is the change risky?]: slightly risky [Why is the change risky/not risky?]: it's a security bug, and while I don't know how to exploit it, it's plausible an exploit that causes OOM at just the right place could do something with this crash. [String changes made/needed]: none
Comment on attachment 8842242 [details] [diff] [review] Move canonicalFunction.init before Scope::create in case create fails. Fix a sec-high & crash. Aurora54+ & Beta53+.
Comment on attachment 8842242 [details] [diff] [review] Move canonicalFunction.init before Scope::create in case create fails. sec-high fix for esr52
Attachment #8842242 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][adv-main53+][adv-esr52.1+]
Setting qe-verify- based on Shu-yu Guo's assessment on manual testing needs and the fact that this fix has automated coverage (see Comment 16).
You need to log in before you can comment on or make changes to this bug.