Closed
Bug 1168719
Opened 8 years ago
Closed 8 years ago
Generic replace-malloc library
Categories
(Core :: Memory Allocator, defect)
Core
Memory Allocator
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file, 1 obsolete file)
16.17 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8611036 -
Flags: review?(n.nethercote)
![]() |
||
Comment 2•8 years ago
|
||
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+
Assignee | ||
Comment 3•8 years ago
|
||
(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.
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8611036 -
Attachment is obsolete: true
Attachment #8615050 -
Flags: review?(n.nethercote)
![]() |
||
Comment 5•8 years ago
|
||
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+
Assignee | ||
Comment 6•8 years ago
|
||
(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•8 years ago
|
||
> 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 10•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e8d8ea70caed https://hg.mozilla.org/mozilla-central/rev/4064fc783498
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in
before you can comment on or make changes to this bug.
Description
•