Closed
Bug 1052573
Opened 10 years ago
Closed 7 years ago
Allocator API for separate heaps
Categories
(Core :: Memory Allocator, defect)
Core
Memory Allocator
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: benjamin, Assigned: glandium)
References
(Blocks 2 open bugs)
Details
Attachments
(4 files, 1 obsolete file)
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.
Reporter | ||
Updated•10 years ago
|
Blocks: heap-partitioning
Assignee | ||
Comment 1•10 years ago
|
||
jemalloc3 has an API for creating and using different arenas. But content-controlled stuff should be in the JS-heap, right? That's not in jemalloc at all.
Comment 2•10 years ago
|
||
JS objects proper are pretty small. For larger objects, and things like typed arrays, they use malloc to allocate a buffer to store data in.
Assignee | ||
Comment 3•10 years ago
|
||
Mike, CCing you to let you know of this, which is kind of related to section 3.6 of the iSEC report for the Tor Browser.
Comment 4•10 years ago
|
||
Cool, thanks. FWIW, Google's PartitionAlloc also provides partitioning capabilities like this, and also provides some additional heap metadata and freelist hardening as well:
https://chromium.googlesource.com/chromium/blink/+/master/Source/wtf/PartitionAlloc.h#30
iSEC apparently got PartitionAlloc working as a drop-in replacement for jemalloc, but obviously that does not enable the partitioning features.
I'll point Tom Ritter from iSEC at this bug as well, as he is interested in continuing this experimentation as time allows. We are also interested in picking up his work and extend it in Tor Browser, so it would be good if we can all work together on this.
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Mike Perry from comment #4)
> Cool, thanks. FWIW, Google's PartitionAlloc also provides partitioning
> capabilities like this, and also provides some additional heap metadata and
> freelist hardening as well
Note iirc jemalloc doesn't have freelists. Jemalloc3, that we don't use by default yet, has something called tcache that acts as one, per thread, but we disable it because it increases memory use too much.
As I read it, there is no difference in hardening between PartitionAlloc "Out-of-line main metadata" and the arenas metadata in jemalloc. They also say there is a metadata structure for each "partition page", which i guess is equivalent to the per-chunk headers in jemalloc, and they say nothing about those being hardened, and I doubt they are.
Anyways, this is off-topic here, maybe we should file a tracking bug for jemalloc hardening, if there needs some.
Comment 6•10 years ago
|
||
Now that we have jemalloc on by default, it's possible to use separate arenas as separate heaps via the mallocx/rallocx API, as comment 1 suggests.
Attachment #8573433 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Guilherme Gonçalves [:ggp] from comment #6)
> Created attachment 8573433 [details] [diff] [review]
> Add heap partitioning functions to mozmemory.
>
> Now that we have jemalloc on by default
jemalloc3 is not quite the default yet. Sure, it is, on central, but when that hits aurora in a few weeks, it won't be the default there.
Also, builds with jemalloc disabled (entirely) are still supported.
Assignee | ||
Comment 8•7 years ago
|
||
I guess I should make this as assigned to myself, since I'm actively working on refreshing this.
Assignee: nobody → mh+mozilla
Assignee | ||
Updated•7 years ago
|
Summary: Implement a separate heap for string/buffer memory → Allocator API for separate heaps
Assignee | ||
Updated•7 years ago
|
Attachment #8573433 -
Attachment is obsolete: true
Attachment #8573433 -
Flags: review?(mh+mozilla)
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8910995 [details]
Bug 1052573 - Generically whitelist memalign calls from anything under memory/.
https://reviewboard.mozilla.org/r/182458/#review187758
Attachment #8910995 -
Flags: review?(n.nethercote) → review+
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8910996 [details]
Bug 1052573 - Separate the base allocator functions in malloc_decls.h.
https://reviewboard.mozilla.org/r/182460/#review187762
Attachment #8910996 -
Flags: review?(n.nethercote) → review+
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8910997 [details]
Bug 1052573 - Move macro helpers to mozjemalloc.h.
https://reviewboard.mozilla.org/r/182462/#review187764
Attachment #8910997 -
Flags: review?(n.nethercote) → review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8910998 [details]
Bug 1052573 - Add an API for allocation in separate arenas.
https://reviewboard.mozilla.org/r/182464/#review187798
::: memory/build/malloc_decls.h:26
(Diff revision 3)
> MALLOC_FUNCS_MALLOC_EXTRA)
> # define MALLOC_FUNCS_JEMALLOC 4
> # define MALLOC_FUNCS_INIT 8
> # define MALLOC_FUNCS_BRIDGE 16
> +# define MALLOC_FUNCS_ARENA_ALLOC 32
> +# define MALLOC_FUNCS_ARENA (MALLOC_FUNCS_ARENA_ALLOC | 64)
64 should have a name associated with it. How about this?
# define MALLOC_FUNCS_ARENA_BASE 1
# define MALLOC_FUNCS_ARENA_ALLOC 2
# define MALLOC_FUNCS_ARENA (MALLOC_FUNCS_ARENA_BASE | MALLOC_FUNCS_ARENA_ALLOC)
::: memory/build/malloc_decls.h:118
(Diff revision 3)
> +MALLOC_DECL(moz_create_arena, arena_id_t)
> +
> +/*
> + * Dispose of the given arena. Subsequent uses of the arena will fail.
> + */
> +MALLOC_DECL(moz_dispose_arena, void, arena_id_t)
"destroy" is arguably a more common pairing with "create"?
::: memory/build/mozjemalloc.h:62
(Diff revision 3)
> +
> +/* Dummy implementation of the moz_arena_* API, falling back to a given
> + * implementation of the base allocator. */
> +template <typename T>
> +struct DummyArenaAllocator {
> + static arena_id_t moz_create_arena(void) {
Can put this function all on one line.
::: memory/build/mozjemalloc.h:66
(Diff revision 3)
> +struct DummyArenaAllocator {
> + static arena_id_t moz_create_arena(void) {
> + return 0;
> + }
> +
> + static void moz_dispose_arena(arena_id_t) {
Ditto.
Attachment #8910998 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 22•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #21)
> ::: memory/build/malloc_decls.h:118
> (Diff revision 3)
> > +MALLOC_DECL(moz_create_arena, arena_id_t)
> > +
> > +/*
> > + * Dispose of the given arena. Subsequent uses of the arena will fail.
> > + */
> > +MALLOC_DECL(moz_dispose_arena, void, arena_id_t)
>
> "destroy" is arguably a more common pairing with "create"?
"destroy" would imply the arena is actively destroyed, which it won't if there remains allocations in it, as in, some things haven't been freed (which they could be because they're still referenced somewhere).
Thought?
Flags: needinfo?(n.nethercote)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
> "destroy" would imply the arena is actively destroyed, which it won't if
> there remains allocations in it, as in, some things haven't been freed
> (which they could be because they're still referenced somewhere).
>
> Thought?
Ok, "dispose" is fine.
Flags: needinfo?(n.nethercote)
Comment 26•7 years ago
|
||
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/05ad53f12a1b
Generically whitelist memalign calls from anything under memory/. r=njn
https://hg.mozilla.org/integration/autoland/rev/c447512b45a7
Separate the base allocator functions in malloc_decls.h. r=njn
https://hg.mozilla.org/integration/autoland/rev/14342971a6c8
Move macro helpers to mozjemalloc.h. r=njn
https://hg.mozilla.org/integration/autoland/rev/a531623c4ec5
Add an API for allocation in separate arenas. r=njn
Comment 27•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/05ad53f12a1b
https://hg.mozilla.org/mozilla-central/rev/c447512b45a7
https://hg.mozilla.org/mozilla-central/rev/14342971a6c8
https://hg.mozilla.org/mozilla-central/rev/a531623c4ec5
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•7 years ago
|
Blocks: allocator-hardening
You need to log in
before you can comment on or make changes to this bug.
Description
•