Closed Bug 1402174 Opened 2 years ago Closed 2 years ago

Allocator API for separate heaps

Categories

(Core :: Memory Allocator, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files)

+++ This bug was initially created as a clone of Bug #1052573 +++

We would like to have a separate heap for strings, buffers, and other data which can be controlled by content. This will separate it from "normal" C++ allocations and make a variety of heap-spray attacks much more difficult.

I think that at a minimum, we just need a single data heap which is separate from the main malloc heap. But if it's possible to have an arbitrary number of heaps by creating a heap object and pointing to it, that might provide more flexibility later.

-----

Bug 1052573 adds the API. This is about actually making it do something.
Comment on attachment 8911014 [details]
Bug 1402174 - Move AlignedAllocator around, so that calloc, realloc and free and grouped with malloc and memalign.

https://reviewboard.mozilla.org/r/182478/#review187810
Attachment #8911014 - Flags: review?(n.nethercote) → review+
Comment on attachment 8911015 [details]
Bug 1402174 - Merge imalloc and icalloc into a single function.

https://reviewboard.mozilla.org/r/182480/#review187812

::: memory/build/mozjemalloc.cpp:3345
(Diff revision 1)
> -	if (size <= arena_maxclass)
> -		return choose_arena(size)->Malloc(size, false);
> -	else
> -		return (huge_malloc(size, false));
> -}
> +  }
> -
> +  return huge_malloc(aSize, aZero);

I'd use ?: here.

::: memory/build/mozjemalloc.cpp:4684
(Diff revision 1)
>  
>    if (aSize == 0) {
>      aSize = 1;
>    }
>  
> -  ret = imalloc(aSize);
> +  ret = imalloc(aSize, false);

`/* zero = */ false` would be nice.

::: memory/build/mozjemalloc.cpp:4742
(Diff revision 1)
>      /* size_t overflow. */
>      ret = nullptr;
>      goto RETURN;
>    }
>  
> -  ret = icalloc(num_size);
> +  ret = imalloc(num_size, true);

ditto

::: memory/build/mozjemalloc.cpp:4773
(Diff revision 1)
>      }
>    } else {
>      if (malloc_init()) {
>        ret = nullptr;
>      } else {
> -      ret = imalloc(aSize);
> +      ret = imalloc(aSize, false);

ditto
Attachment #8911015 - Flags: review?(n.nethercote) → review+
Comment on attachment 8911016 [details]
Bug 1402174 - Add an arena argument to imalloc, ipalloc and iralloc.

https://reviewboard.mozilla.org/r/182482/#review187814

::: memory/build/mozjemalloc.cpp:3963
(Diff revision 1)
>  
> -        /* Try to avoid moving the allocation. */
> +  /* Try to avoid moving the allocation. */
> -	if (size < small_min) {
> -		if (oldsize < small_min &&
> -		    ffs((int)(pow2_ceil(size) >> (TINY_MIN_2POW + 1)))
> -		    == ffs((int)(pow2_ceil(oldsize) >> (TINY_MIN_2POW + 1))))
> +  if (aSize < small_min) {
> +    if (aOldSize < small_min &&
> +        ffs((int)(pow2_ceil(aSize) >> (TINY_MIN_2POW + 1)))
> +        == ffs((int)(pow2_ceil(aOldSize) >> (TINY_MIN_2POW + 1)))) {

== on previous line

::: memory/build/mozjemalloc.cpp:3969
(Diff revision 1)
> -                        goto IN_PLACE; /* Same size class. */
> +      goto IN_PLACE; /* Same size class. */
> -	} else if (size <= small_max) {
> -		if (oldsize >= small_min && oldsize <= small_max &&
> -		    (QUANTUM_CEILING(size) >> opt_quantum_2pow)
> -		    == (QUANTUM_CEILING(oldsize) >> opt_quantum_2pow))
> +    }
> +  } else if (aSize <= small_max) {
> +    if (aOldSize >= small_min && aOldSize <= small_max &&
> +        (QUANTUM_CEILING(aSize) >> opt_quantum_2pow)
> +        == (QUANTUM_CEILING(aOldSize) >> opt_quantum_2pow)) {

ditto

::: memory/build/mozjemalloc.cpp:4029
(Diff revision 1)
> -	oldsize = isalloc(ptr);
> +  oldsize = isalloc(aPtr);
>  
> -	if (size <= arena_maxclass)
> -		return (arena_ralloc(ptr, size, oldsize));
> -	else
> -		return (huge_ralloc(ptr, size, oldsize));
> +  if (aSize <= arena_maxclass) {
> +    return arena_ralloc(aPtr, aSize, oldsize, aArena);
> +  }
> +  return huge_ralloc(aPtr, aSize, oldsize);

I'd use ?: here too.
Attachment #8911016 - Flags: review?(n.nethercote) → review+
Comment on attachment 8911017 [details]
Bug 1402174 - Add a helper class implementing the base allocator functions for a given arena.

https://reviewboard.mozilla.org/r/182484/#review187816

::: memory/build/mozjemalloc.cpp:4693
(Diff revision 1)
> +
> +#define MALLOC_FUNCS MALLOC_FUNCS_MALLOC_BASE
> +#include "malloc_decls.h"
> +
> +  BaseAllocator(arena_t* aArena)
> +  : mArena(aArena)

Indent the ':' by 2 spaces. Or put the whole function on a single line.
Attachment #8911017 - Flags: review?(n.nethercote) → review+
Comment on attachment 8911018 [details]
Bug 1402174 - Initial actual implementation of the moz_arena_* API.

https://reviewboard.mozilla.org/r/182486/#review187818

The TODOs in this patch are disconcerting. I trust you will address them soon.

::: commit-message-a5185:8
(Diff revision 2)
> +Things left for followups:
> +- Full cleanup of disposed arenas: bug 1364359.
> +- Random arena Ids: bug #######.
> +- Forcing the arena to match on realloc and free: bug #######.
> +- Make it impossible to use arenas not created with moz_create_arena
> +  with moz_arena_* functions: bug #######.

Please fill these bug numbers in before landing!
Attachment #8911018 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #8)
> Comment on attachment 8911015 [details]
> Bug 1402174 - Merge imalloc and icalloc into a single function.
> 
> https://reviewboard.mozilla.org/r/182480/#review187812
> 
> ::: memory/build/mozjemalloc.cpp:3345
> (Diff revision 1)
> > -	if (size <= arena_maxclass)
> > -		return choose_arena(size)->Malloc(size, false);
> > -	else
> > -		return (huge_malloc(size, false));
> > -}
> > +  }
> > -
> > +  return huge_malloc(aSize, aZero);
> 
> I'd use ?: here.

Note this part changes in the next patch, making it a bad choice to do this in this one.
Comment on attachment 8911018 [details]
Bug 1402174 - Initial actual implementation of the moz_arena_* API.

https://reviewboard.mozilla.org/r/182486/#review187818

The goal here is to have something out the door early in the cycle. The followups are why the code is NIGHTLY_BUILD only for now, but I do intend to address everything before the end of this cycle.
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/14dac365b5f6
Move AlignedAllocator around, so that calloc, realloc and free and grouped with malloc and memalign. r=njn
https://hg.mozilla.org/integration/autoland/rev/5bba0584a7d8
Merge imalloc and icalloc into a single function. r=njn
https://hg.mozilla.org/integration/autoland/rev/e356ac32e297
Add an arena argument to imalloc, ipalloc and iralloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/c589ad71d9ca
Add a helper class implementing the base allocator functions for a given arena. r=njn
https://hg.mozilla.org/integration/autoland/rev/b23c870c7e45
Initial actual implementation of the moz_arena_* API. r=njn
Backed out for frequently asserting rbp_i_cmp > 0, at mozjemalloc.cpp:2381:

https://hg.mozilla.org/integration/autoland/rev/c298db9e14fb60576b38398db72b4f77ed2a5307
https://hg.mozilla.org/integration/autoland/rev/59fe562e84ff70658a0627866240923ae990fd96
https://hg.mozilla.org/integration/autoland/rev/60d2aa745bb275ad705f98e667fe450d8b0788f8
https://hg.mozilla.org/integration/autoland/rev/cc3be123aa1ff51ba5c6af8ab3484cedc3e467b1
https://hg.mozilla.org/integration/autoland/rev/1ca53af21d0d46808dd8c892ad1d54e8a0ef300f

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b23c870c7e45e3b9cc3f519eba1d9e58389a3c3d&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=132681336&repo=autoland

[task 2017-09-22T13:02:06.691Z] 13:02:06     INFO - TEST-START | docshell/test/browser/browser_click_link_within_view_source.js
[task 2017-09-22T13:02:07.479Z] 13:02:07     INFO - GECKO(2711) | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpRs80hH.mozrunner/runtests_leaks_tab_pid2922.log
[task 2017-09-22T13:02:07.762Z] 13:02:07     INFO - GECKO(2711) | --DOCSHELL 0xe5a09c00 == 0 [pid = 2895] [id = {ade3fc54-7d47-4f25-b6df-584fda505e9b}]
[task 2017-09-22T13:02:09.634Z] 13:02:09     INFO - GECKO(2711) | --DOMWINDOW == 3 (0xda702800) [pid = 2895] [serial = 1] [outer = (nil)] [url = file:///builds/worker/workspace/build/tests/mochitest/browser/docshell/test/browser/test-form_sjis.html]
[task 2017-09-22T13:02:09.654Z] 13:02:09     INFO - GECKO(2711) | [Child 2895, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 403
[task 2017-09-22T13:02:09.657Z] 13:02:09     INFO - GECKO(2711) | [Child 2895, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 403
[task 2017-09-22T13:02:09.659Z] 13:02:09     INFO - GECKO(2711) | [Child 2895, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 403
[task 2017-09-22T13:02:09.662Z] 13:02:09     INFO - GECKO(2711) | [Child 2895, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 403
[task 2017-09-22T13:02:09.663Z] 13:02:09     INFO - GECKO(2711) | [Child 2895, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 403
[task 2017-09-22T13:02:09.664Z] 13:02:09     INFO - GECKO(2711) | [Child 2895, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 403
[task 2017-09-22T13:02:09.670Z] 13:02:09     INFO - GECKO(2711) | [Child 2895, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 403
[task 2017-09-22T13:02:09.675Z] 13:02:09     INFO - GECKO(2711) | [Child 2895, Main Thread] WARNING: '!mMainThread', file /builds/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 403
[task 2017-09-22T13:02:09.682Z] 13:02:09     INFO - GECKO(2711) | --DOMWINDOW == 2 (0xda70a400) [pid = 2895] [serial = 2] [outer = (nil)] [url = about:blank]
[task 2017-09-22T13:02:09.684Z] 13:02:09     INFO - GECKO(2711) | --DOMWINDOW == 1 (0xda138000) [pid = 2895] [serial = 3] [outer = (nil)] [url = file:///builds/worker/workspace/build/tests/mochitest/browser/docshell/test/browser/test-form_sjis.html]
[task 2017-09-22T13:02:09.692Z] 13:02:09     INFO - GECKO(2711) | --DOMWINDOW == 0 (0xd7b16400) [pid = 2895] [serial = 4] [outer = (nil)] [url = file:///builds/worker/workspace/build/tests/mochitest/browser/docshell/test/browser/test-form_sjis.html]
[task 2017-09-22T13:02:09.919Z] 13:02:09     INFO - GECKO(2711) | nsStringStats
[task 2017-09-22T13:02:09.926Z] 13:02:09     INFO - GECKO(2711) |  => mAllocCount:          18618
[task 2017-09-22T13:02:09.927Z] 13:02:09     INFO - GECKO(2711) |  => mReallocCount:          111
[task 2017-09-22T13:02:09.929Z] 13:02:09     INFO - GECKO(2711) |  => mFreeCount:           18618
[task 2017-09-22T13:02:09.931Z] 13:02:09     INFO - GECKO(2711) |  => mShareCount:          11923
[task 2017-09-22T13:02:09.937Z] 13:02:09     INFO - GECKO(2711) |  => mAdoptCount:           1878
[task 2017-09-22T13:02:09.941Z] 13:02:09     INFO - GECKO(2711) |  => mAdoptFreeCount:       1878
[task 2017-09-22T13:02:09.943Z] 13:02:09     INFO - GECKO(2711) |  => Process ID: 2895, Thread ID: 4147058432
[task 2017-09-22T13:02:10.258Z] 13:02:10     INFO - GECKO(2711) | ++DOCSHELL 0xe5b09c00 == 1 [pid = 2922] [id = {e6b7dbbc-e5c2-4eeb-a1ed-14fa5ade012f}]
[task 2017-09-22T13:02:10.455Z] 13:02:10     INFO - GECKO(2711) | ++DOMWINDOW == 1 (0xda802800) [pid = 2922] [serial = 1] [outer = (nil)]
[task 2017-09-22T13:02:10.598Z] 13:02:10     INFO - GECKO(2711) | --DOMWINDOW == 2 (0xdaf0b800) [pid = 2798] [serial = 37] [outer = (nil)] [url = about:blank]
[task 2017-09-22T13:02:10.659Z] 13:02:10     INFO - GECKO(2711) | ++DOMWINDOW == 2 (0xda80a400) [pid = 2922] [serial = 2] [outer = 0xda802800]
[task 2017-09-22T13:02:12.052Z] 13:02:12     INFO - GECKO(2711) | --DOMWINDOW == 10 (0xe4484000) [pid = 2711] [serial = 29] [outer = (nil)] [url = about:blank]
[task 2017-09-22T13:02:12.292Z] 13:02:12     INFO - GECKO(2711) | --DOMWINDOW == 0 (0xd979a000) [pid = 2821] [serial = 51] [outer = (nil)] [url = http://mochi.test:8888/browser/docshell/test/browser/file_bug852909.pdf]
[task 2017-09-22T13:02:12.311Z] 13:02:12     INFO - GECKO(2711) | ++DOMWINDOW == 3 (0xda23c400) [pid = 2922] [serial = 3] [outer = 0xda802800]
[task 2017-09-22T13:02:12.494Z] 13:02:12     INFO - GECKO(2711) | [Child 2922, Main Thread] WARNING: stylo: Web Components not supported yet: file /builds/worker/workspace/build/src/dom/base/nsDocument.cpp, line 6391
[task 2017-09-22T13:02:12.498Z] 13:02:12     INFO - GECKO(2711) | [Child 2922, Main Thread] WARNING: stylo: Web Components not supported yet: file /builds/worker/workspace/build/src/dom/base/nsDocument.cpp, line 6391
[task 2017-09-22T13:02:12.532Z] 13:02:12     INFO - GECKO(2711) | Assertion failure: rbp_i_cmp > 0, at /builds/worker/workspace/build/src/memory/build/mozjemalloc.cpp:2381
[task 2017-09-22T13:02:14.776Z] 13:02:14     INFO - GECKO(2711) | --DOCSHELL 0xd7f60000 == 0 [pid = 2845] [id = {dc2f52fa-1636-4001-88c8-1370ccc24046}]
[task 2017-09-22T13:02:15.300Z] 13:02:15     INFO - GECKO(2711) | --DOMWINDOW == 1 (0xd7f61000) [pid = 2845] [serial = 140] [outer = (nil)] [url = about:blank]
[task 2017-09-22T13:02:16.209Z] 13:02:16     INFO - GECKO(2711) | ###!!! [Parent][MessageChannel] Error: (msgtype=0x150081,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
[task 2017-09-22T13:02:16.347Z] 13:02:16     INFO - GECKO(2711) | ++DOCSHELL 0xcb7c4c00 == 5 [pid = 2711] [id = {8d72137a-cd92-4523-96a1-90d7a8faaf8a}]
[task 2017-09-22T13:02:16.350Z] 13:02:16     INFO - GECKO(2711) | ++DOMWINDOW == 11 (0xcb7c7c00) [pid = 2711] [serial = 47] [outer = (nil)]
[task 2017-09-22T13:02:16.445Z] 13:02:16     INFO - GECKO(2711) | ++DOMWINDOW == 12 (0xcc0cf400) [pid = 2711] [serial = 48] [outer = 0xcb7c7c00]
[task 2017-09-22T13:02:16.745Z] 13:02:16     INFO - GECKO(2711) | ++DOMWINDOW == 13 (0xcd533c00) [pid = 2711] [serial = 49] [outer = 0xcb7c7c00]
[task 2017-09-22T13:02:16.828Z] 13:02:16     INFO - GECKO(2711) | [Parent 2711, Gecko_IOThread] WARNING: waitpid failed pid:2922 errno:10: file /builds/worker/workspace/build/src/ipc/chromium/src/base/process_util_posix.cc, line 276
[task 2017-09-22T13:02:19.261Z] 13:02:19     INFO - GECKO(2711) | --DOMWINDOW == 0 (0xd8679c00) [pid = 2845] [serial = 141] [outer = (nil)] [url = about:blank]
[task 2017-09-22T13:03:07.493Z] 13:03:07     INFO - GECKO(2711) | --DOMWINDOW == 12 (0xd7315000) [pid = 2711] [serial = 37] [outer = (nil)] [url = chrome://browser/content/browser.xul]
[task 2017-09-22T13:03:15.597Z] 13:03:15     INFO - GECKO(2711) | --DOMWINDOW == 11 (0xcc0cf400) [pid = 2711] [serial = 48] [outer = (nil)] [url = about:blank]
[task 2017-09-22T13:03:15.599Z] 13:03:15     INFO - GECKO(2711) | --DOMWINDOW == 10 (0xd7318c00) [pid = 2711] [serial = 38] [outer = (nil)] [url = about:blank]
[task 2017-09-22T13:03:37.769Z] 13:03:37     INFO - TEST-INFO | started process screentopng
[task 2017-09-22T13:03:40.655Z] 13:03:40     INFO - TEST-INFO | screentopng: exit 0
[task 2017-09-22T13:03:40.660Z] 13:03:40     INFO - Buffered messages logged at 13:02:06
[task 2017-09-22T13:03:40.663Z] 13:03:40     INFO - Entering test bound test_click_link_within_view_source
[task 2017-09-22T13:03:40.666Z] 13:03:40     INFO - Buffered messages logged at 13:02:12
[task 2017-09-22T13:03:40.668Z] 13:03:40     INFO - Console message: [JavaScript Warning: "Use of nsIFile in content process is deprecated." {file: "resource://gre/modules/FileUtils.jsm" line: 174}]
[task 2017-09-22T13:03:40.677Z] 13:03:40     INFO - Buffered messages logged at 13:02:16
[task 2017-09-22T13:03:40.680Z] 13:03:40     INFO - Console message: [JavaScript Error: "remote browser crashed while on file:///builds/worker/workspace/build/tests/mochitest/browser/docshell/test/browser/file_click_link_within_view_source.html
[task 2017-09-22T13:03:40.682Z] 13:03:40     INFO - " {file: "chrome://mochikit/content/mochitest-e10s-utils.js" line: 8}]
[task 2017-09-22T13:03:40.684Z] 13:03:40     INFO - e10s_init/<@chrome://mochikit/content/mochitest-e10s-utils.js:8:5
[task 2017-09-22T13:03:40.688Z] 13:03:40     INFO - EventListener.handleEvent*EventTargetInterposition.methods.addEventListener@resource://gre/modules/RemoteAddonsParent.jsm:668:5
[task 2017-09-22T13:03:40.701Z] 13:03:40     INFO - interposeProperty/desc.value@jar:file:///builds/worker/workspace/build/application/firefox/omni.ja!/components/multiprocessShims.js:157:52
[task 2017-09-22T13:03:40.706Z] 13:03:40     INFO - e10s_init@chrome://mochikit/content/mochitest-e10s-utils.js:6:3
[task 2017-09-22T13:03:40.712Z] 13:03:40     INFO - testInit@chrome://mochikit/content/browser-test.js:101:5
[task 2017-09-22T13:03:40.714Z] 13:03:40     INFO - setTimeout handler*@chrome://mochikit/content/browser-test.js:25:3
[task 2017-09-22T13:03:40.715Z] 13:03:40     INFO - 
[task 2017-09-22T13:03:40.721Z] 13:03:40     INFO - Buffered messages finished
[task 2017-09-22T13:03:40.729Z] 13:03:40     INFO - TEST-UNEXPECTED-FAIL | docshell/test/browser/browser_click_link_within_view_source.js | Test timed out -
Flags: needinfo?(mh+mozilla)
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/2cee088f5e73
Move AlignedAllocator around, so that calloc, realloc and free and grouped with malloc and memalign. r=njn
https://hg.mozilla.org/integration/autoland/rev/f1994fc758be
Merge imalloc and icalloc into a single function. r=njn
https://hg.mozilla.org/integration/autoland/rev/ce9d9f4124d6
Add an arena argument to imalloc, ipalloc and iralloc. r=njn
https://hg.mozilla.org/integration/autoland/rev/6c8cc804f12d
Add a helper class implementing the base allocator functions for a given arena. r=njn
https://hg.mozilla.org/integration/autoland/rev/9966b8c1720c
Initial actual implementation of the moz_arena_* API. r=njn
Flags: needinfo?(mh+mozilla)
You need to log in before you can comment on or make changes to this bug.