New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 738925 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug



Sign in to add a comment

macOS: Partition alloc [potentially] leaks wired memory over time.

Project Member Reported by erikc...@chromium.org, Jul 3 2017

Issue description

Each time mmap(hint, ...) is called with |hint| = [a large number], 1-2 wired pages are permanently leaked in the 3rd and 4th level page table in the kernel. The memory is not reclaimed when the region is munmap()ed, only when the process is killed.

See v8 bug/fix:
https://chromium-review.googlesource.com/c/557958/

partition alloc currently passes a large number for |hint|: https://cs.chromium.org/chromium/src/base/allocator/partition_allocator/address_space_randomization.cc?gsn=GetRandomPageBase&l=90

When I run my test page:
https://bugs.chromium.org/p/chromium/issues/detail?id=700928#c111

and use "vmmap -v -interleaved", I count 19 unique partition alloc [4gb] regions. Is this number expected to change over time? If so, each time a unique region is created, we'll leak memory.

According to this documentation, it seems whether the |hint| is consistent is actually dependent on the consumer of partition alloc? And if a null hint is passed, then we will *definitely* leak a page.
https://cs.chromium.org/chromium/src/base/allocator/partition_allocator/page_allocator.cc?gsn=GetRandomPageBase&l=128

Can someone from partition alloc team investigate? I suspect we may need to change this interface to use a less randomized |hint| if |null| is passed as the requested address.

 
 
Cc: palmer@chromium.org
Are you talking about address space leak? Or physical memory leak?

If you're talking about the address space leak, it's expected. PartitionAlloc is designed in a way in which it never releases address spaces allocated (for security reasons).


This is a physical memory leak.

Comment 3 by tasak@google.com, Jul 5 2017

I think, we need the same change as v8's. 
- When allocating pages for BlinkGC (and CallStack) or for DirectMap, AllocPages is invoked with nullptr address.
- When allocating normal pages, root->next_super_page is initialized to be 0. So firstly AllocPages is invoked with nullptr address.

Comment 4 by gov...@chromium.org, Jul 11 2017

A friendly reminder that M61 branch is coming soon on 07/20! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix ASAP to trunk. This way we branch M61 from a high quality trunk. Thank you.

Comment 5 by ajha@chromium.org, Jul 14 2017

Gentle ping to get an update on this issue marked as Blocker.
Labels: -M-61 M-62
I don't have time to look at this in M61. Punting to M62.

Comment 7 by palmer@chromium.org, Jul 17 2017

Cc: tsepez@chromium.org
Owner: palmer@chromium.org
Status: Assigned (was: Untriaged)
Your bug is tagged as Release block Stable. 

M62 is branching soon and We will be taking only CRITICAL merges. Please plan accordingly.
palmer@,

Friendly ping to get an update on this issue as it is marked as Blocker.

Thanks..
I don't think it should be considered a blocker, since the macOS behavior is not new and has not been a fatal problem thus far.

However, I am writing a CL to handle it now; it'll be up for review soon.
Status: Started (was: Assigned)
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/91e40c30a02b7b42a7bb8c5dc3a3eb0b19fad4a9

commit 91e40c30a02b7b42a7bb8c5dc3a3eb0b19fad4a9
Author: Chris Palmer <palmer@chromium.org>
Date: Sun Sep 03 21:42:58 2017

Avoid leaking wired pages on macOS due to ASLR.

BUG= 738925 

Change-Id: I84dddfb7fa7c03c40c62c5efa366a743c5bb8b33
Reviewed-on: https://chromium-review.googlesource.com/641979
Reviewed-by: Erik Chen <erikchen@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Chris Palmer <palmer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499421}
[modify] https://crrev.com/91e40c30a02b7b42a7bb8c5dc3a3eb0b19fad4a9/base/allocator/partition_allocator/address_space_randomization.cc

Status: Fixed (was: Started)
Labels: Merge-TBD
[Auto-generated comment by a script] We noticed that this issue is targeted for M-62; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-62 label, otherwise remove Merge-TBD label. Thanks.
Labels: -Merge-TBD -ReleaseBlock-Stable -M-62 M-63
I'd rather let this roll out via the usual waterfall process. (I don't think it was really ever a RBS.)

Sign in to add a comment