Closed Bug 1518001 Opened 5 years ago Closed 5 years ago

Assertion failure: currentEnd_ - position_ <= NurseryChunkUsableSize

Categories

(Core :: JavaScript: GC, defect, P1)

Product:
Core
Component:
JavaScript: GC
Version:
65 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox-esr60 66+ fixed
firefox65 --- wontfix
firefox66 + fixed
firefox67 + fixed

People

(Reporter: bc, Assigned: jonco)

References

(
URL
)

Details

(Keywords: assertion, reproducible, sec-high, Whiteboard: [post-critsmash-triage][adv-main66+][adv-esr60.6+])

Crash Data

Attachments

(2 files)

bug1518001-free-space-crash
2.42 KB, patch
sfink
: review+
RyanVM
: approval-mozilla-beta+
abillings
: sec-approval+
Details | Diff | Splinter Review
Reset nursery position when it's disabled,
1.70 KB, patch
RyanVM
: approval-mozilla-esr60+
Details | Diff | Splinter Review 
1. https://jsfiddle.net/talon4567/0chsbLgr/ with Nightly or Beta debug
   Wait

2. Assertion failure: currentEnd_ - position_ <= NurseryChunkUsableSize, at z:/build/build/src/js/src/gc/Nursery.h:316

Operating system: Windows NT
                  10.0.17134 
CPU: amd64
     family 6 model 45 stepping 2
     2 CPUs

GPU: UNKNOWN

Crash reason:  EXCEPTION_BREAKPOINT
Crash address: 0x7ffb51f2c937
Assertion: Unknown assertion type 0x00000000
Process uptime: 165 seconds

Thread 0 (crashed)
 0  xul.dll!js::Nursery::needIdleTimeCollection() [Nursery.cpp:e0a4fe89a7b0a36b246f79121d7e4fd70bd898b7 : 697 + 0x35]
    rax = 0x00007ffb542aa0ce   rdx = 0x00007ffb8184a650
    rcx = 0x00007ffb71eeeac8   rbx = 0x00000247bb81eb20
    rsi = 0x00000247b1eea000   rdi = 0x0000000000000001
    rbp = 0x0000000000000001   rsp = 0x000000c43cbfeba0
     r8 = 0x000000c43cbfce98    r9 = 0x000000c43cbfe471
    r10 = 0x0000000000000000   r11 = 0x000000c43cbfea40
    r12 = 0x00000247b1e1bb00   r13 = 0x0000000000000000
    r14 = 0x00000000ffffffff   r15 = 0x000000c43cbff23f
    rip = 0x00007ffb51f2c937
    Found by: given as instruction pointer in context
 1  xul.dll!mozilla::CycleCollectedJSContext::IsIdleGCTaskNeeded() [CycleCollectedJSContext.cpp:e0a4fe89a7b0a36b246f79121d7e4fd70bd898b7 : 428 + 0xa]
    rbx = 0x00000247bb81eb20   rbp = 0x0000000000000001
    rsp = 0x000000c43cbfebd0   r12 = 0x00000247b1e1bb00
    r13 = 0x0000000000000000   r14 = 0x00000000ffffffff
    r15 = 0x000000c43cbff23f   rip = 0x00007ffb4be3812a
    Found by: call frame info
 2  xul.dll!XPCJSContext::AfterProcessTask(unsigned int) [XPCJSContext.cpp:e0a4fe89a7b0a36b246f79121d7e4fd70bd898b7 : 1254 + 0xa]
    rbx = 0x00000247bb81eb20   rbp = 0x0000000000000001
    rsp = 0x000000c43cbfec20   r12 = 0x00000247b1e1bb00
    r13 = 0x0000000000000000   r14 = 0x00000000ffffffff
    r15 = 0x000000c43cbff23f   rip = 0x00007ffb4cce2014
    Found by: call frame info
 3  xul.dll!nsThread::ProcessNextEvent(bool,bool *) [nsThread.cpp:e0a4fe89a7b0a36b246f79121d7e4fd70bd898b7 : 1215 + 0xe]
    rbx = 0x00000247bb81eb20   rbp = 0x0000000000000001
    rsp = 0x000000c43cbfec60   r12 = 0x00000247b1e1bb00
    r13 = 0x0000000000000000   r14 = 0x00000000ffffffff
    r15 = 0x000000c43cbff23f   rip = 0x00007ffb4bf20eda
    Found by: call frame info

Windows 10 x86_64 with 8G of RAM, Fedora 29 x86_64 with 16G. Didn't crash Nightly opt on Fedora locally.

Test case to supposedly crash chrome which is similar enough to other oom test cases we've seen.

txt = "a";
while(1){
    txt = txt += "a";
}

s-s since it is gc and other gc/nursery bugs were s-s.
Group: core-security → javascript-core-security

Possibly related to nursery allocation of strings?

Flags: needinfo?(sphink)

I didn't think nursery strings were enabled by default.

The problem might be that we're not clearing position_ when we disable the nursery.

Priority: -- → P1
Keywords: sec-high

Jon, can you work on this?

Flags: needinfo?(jcoppeard)
Assignee: nobody → jcoppeard

This fixes the crash for me.

Flags: needinfo?(jcoppeard)
Attachment #9042523 - Flags: review?(sphink)
Attachment #9042523 - Flags: review?(sphink) → review+

Comment on attachment 9042523 [details] [diff] [review]
bug1518001-free-space-crash

Security Approval Request

How easily could an exploit be constructed based on the patch?

I don't see a way, since it requires disabling the nursery to hit, and at that point you can't use the now-invalid position to cause any mischief. I think it's just a rogue assert. Unless there's a way to force-enable the nursery. Otherwise, it seems like that would only happen during self-hosting initialization or runtime shutdown.

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?

all

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

None

Do you have backports for the affected branches?

No

If not, how different, hard to create, and risky will they be?

I wouldn't be surprised if the needIdleTimeCollection() function conflicted in minor ways. That check is really just to avoid the assert, but it may require some rebasing. (Overall, the patch is tiny, so it's not going to be a lot of work.)

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.

Flags: needinfo?(sphink)
Attachment #9042523 - Flags: sec-approval?

sec-approval+ for trunk.
Can we get beta and ESR60 patches nominated as well?

status-firefox65: affected → wontfix
status-firefox67: --- → affected
status-firefox-esr60: --- → affected
tracking-firefox66: --- → +
tracking-firefox67: --- → +
tracking-firefox-esr60: --- → 66+
Attachment #9042523 - Flags: sec-approval? → sec-approval+

Comment on attachment 9042523 [details] [diff] [review]
bug1518001-free-space-crash

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

(old; nursery GC)

User impact if declined

Probably none? Though in the future it's possible we might disable the nursery for idle processes or something, which might make this exploitable.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

No

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

Simple change, zeroing out something that could be used with a stale value.

String changes made/needed

Attachment #9042523 - Flags: approval-mozilla-beta?

(same patch backports to beta cleanly; esr60 may need some work)

Backport for esr60. Looks like the isEnabled() check in shouldCollect() is not needed, since it doesn't bother to check whether it should collect. It just collects, possibly needlessly.
Assignee: jcoppeard → sphink
Status: NEW → ASSIGNED

Oops, didn't need to reassign to myself. I'm just landing it.

Assignee: sphink → jcoppeard

Comment on attachment 9043759 [details] [diff] [review]
Reset nursery position when it's disabled,

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

backporting ease, some small potential for security hole

User impact if declined

Fix Landed on Version

67

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

initializing uninitialized value

String or UUID changes made by this patch

none

Attachment #9043759 - Flags: approval-mozilla-esr60?

Waiting for this to land on m-c before we take it on beta/esr60.

Flags: needinfo?(sphink)

https://hg.mozilla.org/mozilla-central/rev/82d0e288504d

Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox67: affected → fixed
Flags: needinfo?(sphink) → in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Comment on attachment 9042523 [details] [diff] [review]
bug1518001-free-space-crash

[Triage Comment]
Approved for 66.0b10 now that it's on m-c.
Attachment #9042523 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Comment on attachment 9043759 [details] [diff] [review]
Reset nursery position when it's disabled,

Fixes a JS sec bug. Baked on Nightly and Beta. Approved for 60.6esr.
Attachment #9043759 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main66+][adv-esr60.6+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: