1410132 - Allocate all JS data in a separate jemalloc arena

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

As a first step to partitioning the heap, make js_malloc() and associated functions use a separate jemalloc arena.

Comment 1

[Jon Coppeard (:jonco)]

The rationale for having a separate heap for all JS data is to make the JS engine configuration in the browser closer to the shell configuration that we fuzz.

We also see a lot of crashes that might be caused by other parts of the browser misusing the jsmalloc heap (e.g. double-free, use-after-free, metadata corruption) and we'd like to discount that possibility.

Comment 2

[Nicholas Nethercote [inactive]]

Note also bug 1052579 and bug 1052582.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: bug1410132-separate-js-malloc-arena

Patch to create a jemalloc arena on initialisation and use this for all malloc allocation inside the engine.  The arena is destroyed on shutdown.

One issue is that since the arena is destroyed on shutdown all memory allocated in the arena must be freed by that point.  This means that we can't rely on freeing things in destructors for global data.  I had to add various clearAndFree() calls for vectors to work around this.

Another point is that the arena is not used on free but determined from the allocated cell itself.  This means that things still work when the browser passes in malloced memory to APIs like JS_NewArrayBufferWithContents which are allocated in a different arena.  In other words, not *all* JS data is allocated in the new arena, but it does make this change much simpler.  Fixing all callers of these APIs is likely to be problematic because the source of the allocations doesn't always know that they will end up being passed into the JS engine.

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8922821 [details] [diff] [review]
bug1410132-separate-js-malloc-arena

Review of attachment 8922821 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/Utility.h
@@ +23,5 @@
>  #include <execinfo.h>
>  #include <stdio.h>
>  #endif
>  
> +#ifdef MOZ_MEMORY

You shouldn't need all those ifdefs. moz_arena_* should fallback to normal malloc when mozjemalloc is disabled.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8922821 [details] [diff] [review]
bug1410132-separate-js-malloc-arena

Review of attachment 8922821 [details] [diff] [review]:
-----------------------------------------------------------------

Seems reasonable. I'm giving f+ for now because you'll need to address glandium's comments anyway.

Also, I wonder if a security person like Tom Ritter or Dan Veditz would like to see this, just to double-check the "all JS memory in one arena" strategy.

::: js/public/Utility.h
@@ +416,5 @@
>  
>  static inline void js_free(void* p)
>  {
> +#ifdef MOZ_MEMORY
> +    moz_arena_free(js::MallocArena, p);

You said "Another point is that the arena is not used on free but determined from the allocated cell itself.  This means that things still work when the browser passes in malloced memory to APIs like JS_NewArrayBufferWithContents which are allocated in a different arena."

My first thought on that was to wonder if it had an effect on performance. But judging by this code, the arena *is* provided when freeing. And that makes sense -- SM already has to have js_malloc and js_free be balanced. So I don't understand what the above quote is saying.

::: js/src/jsapi-tests/testHashTable.cpp
@@ +393,5 @@
>  #endif // defined(DEBUG)
>  
>  BEGIN_TEST(testHashTableMovableEnum)
>  {
> +    IntSet set;

Are this file's changes meant to be in this patch? They seem unrelated.

::: js/src/shell/js.cpp
@@ +4894,5 @@
>      if (sArgc < 1) {
>          JS_ReportErrorNumberASCII(cx, my_GetErrorMessage, nullptr, JSSMSG_NESTED_FAIL);
>          return false;
>      }
> +    if (!argv.append(js_strdup(sArgv[0])))

config/check_vanilla_allocations.py will look for inappropriate strdup() calls if it's passed the --aggressive flag. (Some of what it finds might be false positives, though; see the big comment at the top of that script for details.)

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> But judging by this code, the arena *is* provided when freeing.

It's provided to mozjemalloc, but ignored in the implementation.  moz_arena_free calls BaseAllocator::free, which makes no use of its mArena member.

Perhaps I should make this call plain free() here to make this more obvious.

> SM already has to have js_malloc and js_free be balanced.

This doesn't always seem to be the case e.g. for array buffer contents allocated by the browser.

> > +    IntSet set;
> 
> Are this file's changes meant to be in this patch? They seem unrelated.

Yes, this removes a global variable which could otherwise retain a pointer to malloc memory after engine shutdown.

> config/check_vanilla_allocations.py will look for inappropriate strdup()
> calls if it's passed the --aggressive flag. (Some of what it finds might be
> false positives, though; see the big comment at the top of that script for
> details.)

Thanks.  I didn't know about that.

Comment 7

[Mike Hommey [:glandium]]

(In reply to Jon Coppeard (:jonco) from comment #6)
> (In reply to Nicholas Nethercote [:njn] from comment #5)
> > But judging by this code, the arena *is* provided when freeing.
> 
> It's provided to mozjemalloc, but ignored in the implementation. 
> moz_arena_free calls BaseAllocator::free, which makes no use of its mArena
> member.

That's an implementation detail, but not the contract. Bug 1402283 will enforce the contract, so if you really intend to use free, use free. But using moz_arena_free should be preferred if possible.

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Mike Hommey [:glandium] from comment #7)
Cheers.  I'll use free() for now until we can enforce that js_free() is only used with js_malloc()ed memory.

Comment 9

[Jon Coppeard (:jonco)]

As suggested in comment 5, Tom, do you think the strategy of splitting off all SpiderMonkey allocations into their own heap is reasonable?  There is some explanation in comment 1.  This would be a first step to splitting things up further and moving all content-controlled buffers to their own heap.

Comment 10

[Jon Coppeard (:jonco)]

Attachment: bug1410132-separate-js-malloc-arena v2

Updated patch.

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

(In reply to Jon Coppeard (:jonco) from comment #9)
> As suggested in comment 5, Tom, do you think the strategy of splitting off
> all SpiderMonkey allocations into their own heap is reasonable?  There is
> some explanation in comment 1.  This would be a first step to splitting
> things up further and moving all content-controlled buffers to their own
> heap.

It seems reasonable to me. I don't see it as a security loss for sure. I don't think it's a security gain, but it's possible in some small minority of cases it could be. But I do understand the goal of this isn't to increase security, it's to lay the foundation for that work in the future. So seems fine to me!

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 8923505 [details] [diff] [review]
bug1410132-separate-js-malloc-arena v2

Review of attachment 8923505 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the mechanics, but I'm concerned about Tom's comments: "the goal of this isn't to increase security, it's to lay the foundation for that work in the future". What future work is that? I thought this *was* supposed to help by itself...

Comment 13

[Jon Coppeard (:jonco)]

(In reply to Nicholas Nethercote [:njn] from comment #12)
The part that will significantly increase security is allocating content-writeable buffers in a separate heap.  This is a first step towards that goal that puts *all* JS malloc data in its own heap.

Comment 14

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8923505 [details] [diff] [review]
bug1410132-separate-js-malloc-arena v2

Review of attachment 8923505 [details] [diff] [review]:
-----------------------------------------------------------------

Sounds good to me, and match the js_free limitations which we discussed 2 weeks ago.

This patch does not improve security but it reduces the amount of differences between an embedded/standalone SpiderMonkey.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a0101e3936
Use a separate jemalloc arena for all SpiderMonkey malloc allocations r=njn r=nbp

Comment 16

[Andreea Pavel [:apavel]]

Backed out for build failure ContainerParser.cpp:131

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=141701516&repo=mozilla-inbound&lineNumber=15466

Link to backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/831a02dbf05dc05facbae533ca474d2c4dbb7518

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc7fdeed66bc
Use a separate jemalloc arena for all SpiderMonkey malloc allocations r=njn r=nbp

Comment 18

[Narcis Beleuzu [:NarcisB]]

Backed out for for asserting /builds/worker/workspace/build/src/memory/build/mozjemalloc.cpp:4729 

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c4360c08858f21c9ec656152d3626c0dd1654ee

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=fc7fdeed66bc8a7232822bef6b87f7ad0f462169&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&selectedJob=141933988

Comment 19

[Mike Hommey [:glandium]]

Stack trace:
05:18:45     INFO -  0  libmozglue.dylib!Allocator<MozJemallocBase>::moz_arena_malloc(unsigned long, unsigned long) [mozjemalloc.cpp:fc7fdeed66bc : 4729 + 0x0]
05:18:45     INFO -  1  XUL!js::Mutex::heldMutexStack() [Utility.h:fc7fdeed66bc : 383 + 0xa]
05:18:45     INFO -  2  XUL!js::Mutex::lock() [Mutex.cpp:fc7fdeed66bc : 47 + 0x5]
05:18:45     INFO -  3  XUL!js::Thread::~Thread() [LockGuard.h:fc7fdeed66bc : 25 + 0x5]
05:18:45     INFO -  4  libsystem_c.dylib!__cxa_finalize_ranges + 0x142
05:18:45     INFO -  5  libsystem_c.dylib!exit + 0x37
05:18:45     INFO -  6  plugin-container!start + 0x3b

This points to a shutdown ordering problem, where moz_arena_dispose was called before that moz_arena_malloc.

Comment 20

[Jon Coppeard (:jonco)]

Attachment: bug1410132-mutex-order-check

What's going on is that we have a global js::Thread variable somewhere, and its destructor is running after JS_Shutdown().  This acquires a mutex, and our mutex order checks are tring to allocate memory.

I filed bug 1414310 to investigate why we have this global Thread, but for now let's skip mutex order checks before initialisation and after shutdown of the JS engine.

Comment 21

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8925064 [details] [diff] [review]
bug1410132-mutex-order-check

Review of attachment 8925064 [details] [diff] [review]:
-----------------------------------------------------------------

Yuck. But ok.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/003ff6a2a254
Skip mutex ordering checks when JS engine is not initialized r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/28c901411c1e
Use a separate jemalloc arena for all SpiderMonkey malloc allocations r=njn r=nbp

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for bustage, e.g. in spidermonkey non-unified at js/src/threading/Mutex.cpp:47: 'JS_IsInitialized' was not declared in this scope:

https://hg.mozilla.org/integration/mozilla-inbound/rev/55f9dc61e9f70361546da610e349ec20ce6e774b

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=28c901411c1ea54f48089eee095f5d86c1c94062&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry
Failure log (but also affects Android): https://treeherder.mozilla.org/logviewer.html#?job_id=142775349&repo=mozilla-inbound
/builds/worker/workspace/build/src/js/src/threading/Mutex.cpp:47:25: error: 'JS_IsInitialized' was not declared in this scope
/builds/worker/workspace/build/src/js/src/threading/Mutex.cpp:73:25: error: 'JS_IsInitialized' was not declared in this scope

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69aedd61b682
Skip mutex ordering checks when JS engine is not initialized r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdabc4808a0f
Use a separate jemalloc arena for all SpiderMonkey malloc allocations r=njn r=nbp

Comment 25

[Ionuț Goldan [:igoldan]]

Noticed these performance improvements:

== Change summary for alert #10445 (as of Wed, 08 Nov 2017 08:45:50 GMT) ==

Improvements:

  3%  tcanvasmark summary linux64 pgo e10s     11,246.50 -> 11,545.79
  2%  tcanvasmark summary linux64 opt e10s     10,540.46 -> 10,791.15

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=10445

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/69aedd61b682
https://hg.mozilla.org/mozilla-central/rev/cdabc4808a0f

Comment 27

[Ionuț Goldan [:igoldan]]

The improvements I mentioned in comment 25 are for the landing of the bug, not for the backout which followed it shortly.