Allocator API for separate heaps

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
4 years ago
6 months ago

People

(Reporter: benjamin, Assigned: glandium)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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

4 years ago
Blocks: 1052575
(Assignee)

Comment 1

4 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.
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

4 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

4 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

4 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.
Created attachment 8573433 [details] [diff] [review]
Add heap partitioning functions to mozmemory.

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

4 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)

Updated

a year ago
Depends on: 1368932
(Assignee)

Updated

a year ago
Depends on: 1369626
(Assignee)

Comment 8

a year 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

a year ago
Summary: Implement a separate heap for string/buffer memory → Allocator API for separate heaps
(Assignee)

Updated

a year ago
Attachment #8573433 - Attachment is obsolete: true
Attachment #8573433 - Flags: review?(mh+mozilla)
(Assignee)

Updated

a year ago
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1402174
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

a year 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

a year 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

a year 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

a year 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

a year 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)
> "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

a year 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

a year 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
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Updated

a year ago
Depends on: 1402647

Updated

6 months ago
Blocks: 1446036
You need to log in before you can comment on or make changes to this bug.