1052573 - Allocator API for separate heaps

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Benjamin Smedberg]

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.

Comment 1

[Mike Hommey [:glandium]]

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

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Mike Hommey [:glandium]]

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

[Mike Perry]

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.

Comment 5

[Mike Hommey [:glandium]]

(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

[Guilherme Gonçalves [:ggp]]

Attachment: 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.

Comment 7

[Mike Hommey [:glandium]]

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

Comment 8

[Mike Hommey [:glandium]]

I guess I should make this as assigned to myself, since I'm actively working on refreshing this.

Comment 9

[Mike Hommey [:glandium]]

Attachment: Bug 1052573 - Generically whitelist memalign calls from anything under memory/.

Review commit: https://reviewboard.mozilla.org/r/182458/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182458/

Comment 10

[Mike Hommey [:glandium]]

Attachment: Bug 1052573 - Separate the base allocator functions in malloc_decls.h.

malloc, free, calloc, realloc and memalign constitute some sort of
minimal interface to the allocator. posix_memalign, aligned_alloc and
valloc are already defined in terms of memalign. The remaining functions
are not related to active allocation.

Review commit: https://reviewboard.mozilla.org/r/182460/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182460/

Comment 11

[Mike Hommey [:glandium]]

Attachment: Bug 1052573 - Move macro helpers to mozjemalloc.h.

Review commit: https://reviewboard.mozilla.org/r/182462/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182462/

Comment 12

[Mike Hommey [:glandium]]

Attachment: Bug 1052573 - Add an API for allocation in separate arenas.

The implementation is not doing anything just yet. This will be done in
a followup bug.

Review commit: https://reviewboard.mozilla.org/r/182464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182464/

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8910995 [details]
Bug 1052573 - Generically whitelist memalign calls from anything under memory/.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182458/diff/1-2/

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8910996 [details]
Bug 1052573 - Separate the base allocator functions in malloc_decls.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182460/diff/1-2/

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8910997 [details]
Bug 1052573 - Move macro helpers to mozjemalloc.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182462/diff/1-2/

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8910998 [details]
Bug 1052573 - Add an API for allocation in separate arenas.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182464/diff/1-2/

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8910995 [details]
Bug 1052573 - Generically whitelist memalign calls from anything under memory/.

https://reviewboard.mozilla.org/r/182458/#review187758

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8910998 [details]
Bug 1052573 - Add an API for allocation in separate arenas.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182464/diff/2-3/

Comment 19

[Nicholas Nethercote [inactive]]

Comment on attachment 8910996 [details]
Bug 1052573 - Separate the base allocator functions in malloc_decls.h.

https://reviewboard.mozilla.org/r/182460/#review187762

Comment 20

[Nicholas Nethercote [inactive]]

Comment on attachment 8910997 [details]
Bug 1052573 - Move macro helpers to mozjemalloc.h.

https://reviewboard.mozilla.org/r/182462/#review187764

Comment 21

[Nicholas Nethercote [inactive]]

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.

Comment 22

[Mike Hommey [:glandium]]

(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?

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8910998 [details]
Bug 1052573 - Add an API for allocation in separate arenas.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182464/diff/3-4/

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8910998 [details]
Bug 1052573 - Add an API for allocation in separate arenas.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182464/diff/4-5/

Comment 25

[Nicholas Nethercote [inactive]]

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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