1168719 - Generic replace-malloc library

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Mike Hommey [:glandium]]

In bug 1123237, Kan-Ru used a replace-malloc library to insert callbacks after malloc functions. This prompted me to think about it some more and converge with some things I wanted to do with replace-malloc. I however don't want to block bug 1123237 more than it has been blocked, so I implemented the vital minimum bug 1123237 needs, while tackling the base work for future replace-malloc changes, without e.g. having to change the bridge again.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Generic replace-malloc library

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8611036 [details] [diff] [review]
Generic replace-malloc library

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

I'm concerned about the order of the calls for free() and realloc(). See below.

::: memory/build/replace_malloc_bridge.h
@@ +128,5 @@
> +   * library so that it can choose to dispatch to them when needed. The
> +   * details of what is dispatched when is left to the replace-malloc
> +   * library.
> +   * Returns whether a table of malloc functions that won't call into the
> +   * hooks or nullptr if setting up the hook failed.

I can't parse this sentence. Reword?

::: memory/replace/replace/ReplaceMalloc.cpp
@@ +21,5 @@
> +  RegisterHook(const char* aName, const malloc_table_t* aTable,
> +               const malloc_hook_table_t* aHookTable) override
> +  {
> +    // Can't register a hook before replace_init is called.
> +    if (!gFuncs) return nullptr;

Nit: brace these and put the then-statement on its own line. Same below.

@@ +70,5 @@
> +  int ret = gFuncs->posix_memalign(aPtr, aAlignment, aSize);
> +  if (gHookTable) {
> +    if (gHookTable->posix_memalign_hook) {
> +      return gHookTable->posix_memalign_hook(ret, aPtr, aAlignment, aSize);
> +    } else {

The coding style guide says no |else| after |return|, and you have lots of those in this file.

@@ +103,5 @@
> +  if (gHookTable) {
> +    if (gHookTable->calloc_hook) {
> +      return gHookTable->calloc_hook(ptr, aNum, aSize);
> +    } else {
> +      // TODO: properly handle integer overflow from the multiplication.

You might as well fix this now, rather than causing someone hard-to-debug grief later on...

@@ +131,5 @@
> +{
> +  gFuncs->free(aPtr);
> +  if (gHookTable) {
> +    gHookTable->free_hook(aPtr);
> +  }

I think calling the hook first is probably better. There are some things (like calling malloc_usable_size) that you can't do afterwards. In fact, passing |aPtr| to the hook after it's been freed is dangerous.

Similarly, with the realloc-hook above -- realloc is so complex that you really want both before and after hooks.

Comment 3

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> > +      // TODO: properly handle integer overflow from the multiplication.
> 
> You might as well fix this now, rather than causing someone hard-to-debug
> grief later on...

Heh, I thought I needed mfbt .cpp files to use CheckedInt, so I defered to later, but in fact, it all works with the .h.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Generic replace-malloc library

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8615050 [details] [diff] [review]
Generic replace-malloc library

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

::: memory/build/replace_malloc_bridge.h
@@ +77,5 @@
> + * Those functions are called with the arguments and results of malloc
> + * functions after they are called.
> + *   e.g. void* (*malloc_hook)(void*, size_t), etc.
> + * They can either return the result they're given, or alter it before
> + * returning it.

It would be good to explain that if you just provide |malloc| and |free| hooks they will be called in a fallback kind of way for all the other functions; you could give |realloc| as an example.

@@ +126,5 @@
>     * This method was added in version 2 of the bridge. */
>    virtual void InitDebugFd(mozilla::DebugFdRegistry&) {}
>  
> +  /* Register a list of malloc functions and hook functions to the
> +   * replace-malloc  library so that it can choose to dispatch to them

Nit: extraneous space before "library".

@@ +136,5 @@
> +   * If registration succeeded, a table of "pure" malloc functions is
> +   * returned. Those "pure" malloc functions won't call hooks.
> +   * /!\ Do not rely on registration/unregistration to be instantaneous.
> +   * Functions from a previously registered table may still be called after
> +   * RegisterHook returns.

I'd clarify this by saying "may still be called for a brief time after RegisterHook returns", or something.

::: memory/replace/replace/ReplaceMalloc.cpp
@@ +120,5 @@
> +    mozilla::CheckedInt<size_t> size = mozilla::CheckedInt<size_t>(aNum) * aSize;
> +    if (size.isValid()) {
> +      return hook_table->malloc_hook(ptr, size.value());
> +    }
> +    return hook_table->malloc_hook(ptr, size_t(-1));

Why size_t(-1)? That seems... perverse. Though I'm not sure what would be better in that case. Skipping the hook altogether?

Comment 6

[Mike Hommey [:glandium]]

(In reply to Nicholas Nethercote [:njn] from comment #5)
> > +    return hook_table->malloc_hook(ptr, size_t(-1));
> 
> Why size_t(-1)? That seems... perverse. Though I'm not sure what would be
> better in that case. Skipping the hook altogether?

That could be SIZE_MAX (which is the same), but I'm not sure it's part of what MSVC supports... although I see we're using it in mozjemalloc, in a #ifdef MOZ_MEMORY_WINDOWS.

The rationale, which I should add to a comment is that if the multiplication overflows, calloc failed, so ptr is null. But the hook might still be interested in this, and the biggest value of a size_t is not that bad an indicator of overflow: there are only 5 prime factors to 2^32-1 and 7 prime factors to 2^64-1, and none of them is going to come out of sizeof().

Comment 7

[Nicholas Nethercote [inactive]]

> That could be SIZE_MAX (which is the same), but I'm not sure it's part of
> what MSVC supports... although I see we're using it in mozjemalloc, in a
> #ifdef MOZ_MEMORY_WINDOWS.
> 
> The rationale, which I should add to a comment is that if the multiplication
> overflows, calloc failed, so ptr is null. But the hook might still be
> interested in this, and the biggest value of a size_t is not that bad an
> indicator of overflow: there are only 5 prime factors to 2^32-1 and 7 prime
> factors to 2^64-1, and none of them is going to come out of sizeof().

Fair enough. A comment would help.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d8ea70caed

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4064fc783498

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e8d8ea70caed
https://hg.mozilla.org/mozilla-central/rev/4064fc783498