Closed Bug 1168719 Opened 10 years ago Closed 10 years ago

Generic replace-malloc library

Categories

(Core :: Memory Allocator, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Generic replace-malloc library (obsolete) — Splinter Review
Attachment #8611036 - Flags: review?(n.nethercote)
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.
Attachment #8611036 - Flags: review?(n.nethercote) → feedback+
(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.
Attachment #8611036 - Attachment is obsolete: true
Attachment #8615050 - Flags: review?(n.nethercote)
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?
Attachment #8615050 - Flags: review?(n.nethercote) → review+
(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().
> 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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1172668
Depends on: 1290645
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: