1480430 - Support dynamic hooking of memory allocator functions

Status: RESOLVED FIXED

(Core :: Memory Allocator, enhancement, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

+++ This bug was initially created as a clone of Bug #1464509 +++

In order to support hooking and unhooking of memory allocation monitoring, we need to all dynamic hooking of the allocator.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Modify jemalloc to allow dynamic replacement

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Modify jemalloc to allow dynamic replacement

fixes the jemalloc_replace_dynamic weak ref issue

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Modify jemalloc to allow dynamic replacement

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8997192 [details] [diff] [review]
Modify jemalloc to allow dynamic replacement

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

::: memory/build/mozjemalloc.cpp
@@ +4689,5 @@
>  #include "replace_malloc.h"
>  
>  #define MALLOC_DECL(name, return_type, ...) MozJemalloc::name,
>  
> +static malloc_table_t gReplaceMallocTableDefault = {

It feels like this should be `static const malloc_table_t kReplaceMallocTableDefault`

@@ +4818,5 @@
> +  if (replace_init_func) {
> +    (*replace_init_func)(&tempTable, &gReplaceMallocBridge);
> +    if (!Equals(tempTable, gReplaceMallocTableDefault)) {
> +      replace_malloc_init_funcs(&tempTable);
> +      malloc_table_t* new_table = static_cast<malloc_table_t*>(MozJemalloc::malloc(sizeof(malloc_table_t)));

You're leaking this table. It would be better for it to either be a global table, or a global pointer to a table you allocate only once. If you keep the allocation, it would be better to use the base allocator (base_alloc) rather than the "public" allocator.

@@ +4838,3 @@
>        init();                                                                  \
>      }                                                                          \
> +    return (*gReplaceMallocTable).name(ARGS_HELPER(ARGS, ##__VA_ARGS__)); \

gReplaceMallocTable-> ?

::: memory/build/mozjemalloc_types.h
@@ +56,5 @@
>  
> +struct malloc_table;
> +typedef struct malloc_table malloc_table_t;
> +struct ReplaceMallocBridge;
> +typedef void (*jemalloc_init_func)(malloc_table_t*, struct ReplaceMallocBridge**);

Please move this definition to replace_malloc.h

::: memory/build/mozmemory.h
@@ +66,5 @@
>  #define moz_create_arena() moz_create_arena_with_params(NULL)
>  #endif
>  
> +#ifdef MOZ_REPLACE_MALLOC
> +MOZ_JEMALLOC_API void jemalloc_replace_dynamic(jemalloc_init_func);

You shouldn't define this here, only in replace_malloc.h.

::: memory/build/replace_malloc_bridge.h
@@ +65,5 @@
>  #include "malloc_decls.h"
>  
>  #define MALLOC_DECL(name, return_type, ...) name##_impl_t* name;
>  
> +typedef struct malloc_table

With the other suggested changes, this should be unnecessary.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

> @@ +4818,5 @@
> > +  if (replace_init_func) {
> > +    (*replace_init_func)(&tempTable, &gReplaceMallocBridge);
> > +    if (!Equals(tempTable, gReplaceMallocTableDefault)) {
> > +      replace_malloc_init_funcs(&tempTable);
> > +      malloc_table_t* new_table = static_cast<malloc_table_t*>(MozJemalloc::malloc(sizeof(malloc_table_t)));
> 
> You're leaking this table. It would be better for it to either be a global
> table, or a global pointer to a table you allocate only once. If you keep
> the allocation, it would be better to use the base allocator (base_alloc)
> rather than the "public" allocator.

A global table or a table we only allocate once creates unavoidable races if there are multiple replacements.  A thread could have pulled the pointer from the atomic and de-reference it at an unknown point in the future.  Even a global or allocated-once table would have races on future changes since the pointers within it aren't atomic.   While in practice this is likely pretty darn safe, in theory it's dangerous.  Leaking a table on a replace (via base_alloc()) is a reasonable alternative.  Added comments.

> 
> @@ +4838,3 @@
> >        init();                                                                  \
> >      }                                                                          \
> > +    return (*gReplaceMallocTable).name(ARGS_HELPER(ARGS, ##__VA_ARGS__)); \
> 
> gReplaceMallocTable-> ?

Tried that before (and again just now).  The compiler is very unhappy with that, though in theory it seems it should work.  Leaving it alone.  Probably has to do with the Atomic<>.

 0:02.84 In file included from Unified_cpp_memory_build0.cpp:2:
 0:02.84 In file included from /home/jesup/src/mozilla/inbound/memory/build/mozjemalloc.cpp:4849:
 0:02.84 ../../../memory/build/malloc_decls.h:37:1: error: member reference type 'Atomic<malloc_table_t *, mozilla::MemoryOrdering::Relaxed, recordreplay::Behavior::DontPreserve>' is not a pointer; did you mean to use '.'?
 0:02.84 MALLOC_DECL(malloc, void*, size_t)
 0:02.84 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:02.84 /home/jesup/src/mozilla/inbound/memory/build/mozjemalloc.cpp:4847:31: note: expanded from macro 'MALLOC_DECL'
 0:02.84     return gReplaceMallocTable->name(ARGS_HELPER(ARGS, ##__VA_ARGS__)); \
 0:02.84            ~~~~~~~~~~~~~~~~~~~^
 0:02.84 In file included from Unified_cpp_memory_build0.cpp:2:
 0:02.84 In file included from /home/jesup/src/mozilla/inbound/memory/build/mozjemalloc.cpp:4849:
 0:02.84 ../../../memory/build/malloc_decls.h:37:13: error: no member named 'malloc' in 'mozilla::Atomic<malloc_table_t *, mozilla::Relaxed, mozilla::recordreplay::Behavior::DontPreserve, void>'
 0:02.84 MALLOC_DECL(malloc, void*, size_t)
 0:02.84 ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
 0:02.84 /home/jesup/src/mozilla/inbound/memory/build/mozjemalloc.cpp:4847:33: note: expanded from macro 'MALLOC_DECL'
 0:02.85     return gReplaceMallocTable->name(ARGS_HELPER(ARGS, ##__VA_ARGS__)); \
 0:02.85            ~~~~~~~~~~~~~~~~~~~  ^

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Modify jemalloc to allow dynamic replacement

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8999098 [details] [diff] [review]
Modify jemalloc to allow dynamic replacement

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

::: memory/build/mozjemalloc.cpp
@@ +4822,5 @@
> +      malloc_table_t* new_table = static_cast<malloc_table_t*>(base_alloc(sizeof(malloc_table_t)));
> +      MOZ_RELEASE_ASSERT(new_table);
> +      *new_table = tempTable;
> +      gReplaceMallocTable = new_table;
> +      // We'd like to free the old table, but we can't safely.  Another

I'm not convinced we need to leak here.

> A global table or a table we only allocate once creates unavoidable races if there are multiple replacements.  A thread could have pulled the pointer from the atomic and de-reference it at an unknown point in the future.

So? Either the pointer was previously on the default table, and that thread is going to read from that table, or the pointer was previously on the global table and it's still valid.

> Even a global or allocated-once table would have races on future changes since the pointers within it aren't atomic. While in practice this is likely pretty darn safe, in theory it's dangerous.

I think your concerns are misplaced. Worse things can happen with this API without involving the unsafety of the table switch. The more I think about it, the more I think restoring the malloc hook API in a different form would be better (the one removed in bug 1401101).

::: memory/build/replace_malloc.h
@@ +96,5 @@
>  #else
>  #define MOZ_REPLACE_PUBLIC MOZ_EXPORT
>  #endif
>  
> +struct ReplaceMallocBridge;

You don't need this forward declaration, because the struct is already defined by including replace_malloc_bridge.h earlier in this file.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

> > A global table or a table we only allocate once creates unavoidable races if there are multiple replacements.  A thread could have pulled the pointer from the atomic and de-reference it at an unknown point in the future.
> 
> So? Either the pointer was previously on the default table, and that thread
> is going to read from that table, or the pointer was previously on the
> global table and it's still valid.

So if we're not going to leak, we have to free the allocated table.  When does that become safe?  Since we don't lock usage of the table (just use an atomic to access the pointer to the table), we don't know when it's safe to release that table memory.  The same applies to ReplaceMalloc.cpp's RegisterHook() -- if we un-hook (with RegisterHook(..., nullptr) or switch to a different hook, we also can't free the old hook - we have to leak it if we want to be pedantic about safety.  It has the exact same issue - Atomic<> ptr to a function table.   The one advantage (not implemented in the old usage of ReplaceMalloc) is one could put the table in a global/static - but that uses as much memory as allocating it and leaking it, or close to, and it uses that memory even if we never hook it.

So either we wave our hands at the safety aspect by keeping it alive "for a while" and hope nothing goes wrong, switch to the old ReplaceMalloc API and use global/statics, or (in the very rare case of hooking the allocator - generally for debugging/profiling) leak a very small block of memory.  Note that the version where I touched the API for replace_init/etc also allowed me to use static tables for the function tables (but did break API compatibility).

> > Even a global or allocated-once table would have races on future changes since the pointers within it aren't atomic. While in practice this is likely pretty darn safe, in theory it's dangerous.
> 
> I think your concerns are misplaced. Worse things can happen with this API
> without involving the unsafety of the table switch. The more I think about
> it, the more I think restoring the malloc hook API in a different form would
> be better (the one removed in bug 1401101).

So this would be revert bug 1401101 (which appears to revert cleanly and build, at first look), and re-implement similar to how bug 1385953 used it.

> > +struct ReplaceMallocBridge;
> 
> You don't need this forward declaration, because the struct is already
> defined by including replace_malloc_bridge.h earlier in this file.

Missed that, thanks

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Modify jemalloc to allow dynamic replacement

In theory there's a possible race here if someone called dynamic_replace back-to-back more than once in a row - but even then the only downside would be that it might call one of the since-replaced alloc or free functions.

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 9007124 [details] [diff] [review]
Modify jemalloc to allow dynamic replacement

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

::: memory/build/mozjemalloc.cpp
@@ +4700,5 @@
>  };
> +static malloc_table_t gReplaceMallocTables[2] = {
> +  {
> +#include "malloc_decls.h"
> +  }, 

trailing whitespaces here and below.

@@ +4832,5 @@
> +  if (replace_init_func) {
> +    (*replace_init_func)(&tempTable, &gReplaceMallocBridge);
> +    if (!Equals(tempTable, gReplaceMallocTableDefault)) {
> +      replace_malloc_init_funcs(&tempTable);
> +      // flip-flop between two tables

this doesn't feel necessary, but at this point, meh.

@@ +4838,5 @@
> +      gReplaceMallocTables[gReplaceMallocIndex] = tempTable;
> +      gReplaceMallocTable = &gReplaceMallocTables[gReplaceMallocIndex];
> +      // We assume that dynamic replaces don't occur close enough for a
> +      // thread to still have old copies of the table pointer when the 2nd
> +      // replace occurs.

even if they did, that wouldn't really matter.

::: memory/build/mozmemory.h
@@ +14,5 @@
>  //   - jemalloc_purge_freed_pages
>  //   - jemalloc_free_dirty_pages
>  //   - jemalloc_thread_local_arena
>  //   - jemalloc_ptr_info
> +//   - jemalloc_replace_dynamic

functions from replace_malloc.h don't figure here.

::: memory/build/mozmemory_wrap.h
@@ +38,5 @@
>  //   - jemalloc_purge_freed_pages
>  //   - jemalloc_free_dirty_pages
>  //   - jemalloc_thread_local_arena
>  //   - jemalloc_ptr_info
> +//   - jemalloc_replace_dynamic

functions from replace_malloc.h don't figure here.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Avoid allocations on memory hooking

Minor cleanup - missed one copy of the memory-table allocation code; that caused Windows to fail the logalloc-replay check (but nothing else, oddly).  Clean on Try now.  Ready to land otherwise along with the code that uses it

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 9013187 [details] [diff] [review]
Avoid allocations on memory hooking

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

::: memory/build/mozjemalloc.cpp
@@ +4809,5 @@
>  #endif
>  #endif
>    if (!Equals(tempTable, gReplaceMallocTableDefault)) {
>      replace_malloc_init_funcs(&tempTable);
> +    gReplaceMallocIndex = (gReplaceMallocIndex+1) % 2;

whitespaces around +

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Link with libatomic for 32-bit linux

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b69c28ab39fc
Modify jemalloc to allow dynamic replacement r=glandium

Comment 15

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/b69c28ab39fc