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

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.

Note also bug 1052579 and bug 1052582.

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

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

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

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

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.

Updated patch.

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

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

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

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

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

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

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.

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 on attachment 8925064 [details] [diff] [review] bug1410132-mutex-order-check Review of attachment 8925064 [details] [diff] [review]: ----------------------------------------------------------------- Yuck. But ok.

Pushed by jcoppeard@mozilla.com: 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

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

Pushed by jcoppeard@mozilla.com: 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

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

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

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