Closed
Bug 1342101
Opened 8 years ago
Closed 8 years ago
Crash [@ js::IsDerivedProxyObject] or Assertion failure: CurrentThreadCanAccessZone(zone), at gc/Heap.h:1262
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: decoder, Assigned: shu)
References
Details
(6 keywords, Whiteboard: [jsbugmon:ignore][adv-main53+][adv-esr52.1+])
Crash Data
Attachments
(2 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
jandem
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52+
abillings
:
sec-approval+
|
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.
Reporter | ||
Comment 1•8 years ago
|
||
Comment 3•8 years ago
|
||
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)
Comment 4•8 years ago
|
||
Likely a regression from bug 1263355.
Blocks: 1263355
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox-esr45:
--- → unaffected
status-firefox-esr52:
--- → affected
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
tracking-firefox54:
--- → ?
tracking-firefox-esr52:
--- → ?
Assignee | ||
Comment 5•8 years ago
|
||
I am having trouble reproducing the crash.
Assignee | ||
Comment 6•8 years ago
|
||
I am now reproducing the crash. What a roller coaster.
Flags: needinfo?(shu)
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8842242 -
Flags: review?(jdemooij)
Assignee | ||
Comment 8•8 years ago
|
||
I am not sure how to test this.
Updated•8 years ago
|
Attachment #8842242 -
Flags: review?(jdemooij) → review+
Updated•8 years ago
|
Assignee | ||
Comment 9•8 years ago
|
||
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?
Updated•8 years ago
|
Comment 10•8 years ago
|
||
Approved for checkin on March 21, two weeks into the new cycle.
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][checkin on 3/21]
Updated•8 years ago
|
Attachment #8842242 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 12•8 years ago
|
||
(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?
Comment 13•8 years ago
|
||
You can do it yourself or else I tend to look for them and land when the time comes otherwise.
Comment 14•8 years ago
|
||
(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. :-)
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/514a06ddb08ca52e552124f6d23d2f053c4f7e13
Please request Aurora/Beta/ESR52 approval on this as soon as possible.
status-firefox55:
--- → affected
tracking-firefox55:
--- → ?
Flags: needinfo?(shu)
Whiteboard: [jsbugmon:ignore][checkin on 3/21] → [jsbugmon:ignore]
Assignee | ||
Comment 16•8 years ago
|
||
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
Flags: needinfo?(shu)
Attachment #8842242 -
Flags: approval-mozilla-esr52?
Attachment #8842242 -
Flags: approval-mozilla-beta?
Attachment #8842242 -
Flags: approval-mozilla-aurora?
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 18•8 years ago
|
||
Comment on attachment 8842242 [details] [diff] [review]
Move canonicalFunction.init before Scope::create in case create fails.
Fix a sec-high & crash. Aurora54+ & Beta53+.
Attachment #8842242 -
Flags: approval-mozilla-beta?
Attachment #8842242 -
Flags: approval-mozilla-beta+
Attachment #8842242 -
Flags: approval-mozilla-aurora?
Attachment #8842242 -
Flags: approval-mozilla-aurora+
Comment 19•8 years ago
|
||
uplift |
Comment 20•8 years ago
|
||
uplift |
Updated•8 years ago
|
Comment 21•8 years ago
|
||
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+
Comment 22•8 years ago
|
||
uplift |
Updated•8 years ago
|
Group: javascript-core-security → core-security-release
Updated•8 years ago
|
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][adv-main53+][adv-esr52.1+]
Comment 23•8 years ago
|
||
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).
Flags: qe-verify-
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•