Closed Bug 1052573 Opened 10 years ago Closed 7 years ago

Allocator API for separate heaps

Categories

(Core :: Memory Allocator, defect)

defect
Not set
normal

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.
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.
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.
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.
(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.
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)
(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.
Depends on: 1368932
Depends on: 1369626
I guess I should make this as assigned to myself, since I'm actively working on refreshing this.
Assignee: nobody → mh+mozilla
Summary: Implement a separate heap for string/buffer memory → Allocator API for separate heaps
Attachment #8573433 - Attachment is obsolete: true
Attachment #8573433 - Flags: review?(mh+mozilla)
Blocks: 1402174
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 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 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 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+
(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)
> "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)
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
Depends on: 1402647
You need to log in before you can comment on or make changes to this bug.