Closed Bug 1342101 Opened 3 years ago Closed 3 years ago

Crash [@ js::IsDerivedProxyObject] or Assertion failure: CurrentThreadCanAccessZone(zone), at gc/Heap.h:1262

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- wontfix
firefox52 - wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed
firefox55 + fixed

People

(Reporter: decoder, Assigned: shu)

References

(Blocks 1 open bug)

Details

(6 keywords, Whiteboard: [jsbugmon:ignore][adv-main53+][adv-esr52.1+])

Crash Data

Attachments

(2 files)

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.
Attached file Testcase
NI for jandem.
Flags: needinfo?(jdemooij)
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)
I am having trouble reproducing the crash.
I am now reproducing the crash. What a roller coaster.
Flags: needinfo?(shu)
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.
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][checkin on 3/21]
Attachment #8842242 - Flags: sec-approval? → sec-approval+
Duplicate of this bug: 1343876
(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.
Flags: needinfo?(shu)
Whiteboard: [jsbugmon:ignore][checkin on 3/21] → [jsbugmon:ignore]
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?
https://hg.mozilla.org/mozilla-central/rev/514a06ddb08c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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 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+
Group: javascript-core-security → core-security-release
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).
Flags: qe-verify-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.