Closed
Bug 1168719
Opened 10 years ago
Closed 10 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•10 years ago
|
||
Attachment #8611036 -
Flags: review?(n.nethercote)
![]() |
||
Comment 2•10 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•10 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•10 years ago
|
||
Attachment #8611036 -
Attachment is obsolete: true
Attachment #8615050 -
Flags: review?(n.nethercote)
![]() |
||
Comment 5•10 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•10 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•10 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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e8d8ea70caed
https://hg.mozilla.org/mozilla-central/rev/4064fc783498
Status: NEW → RESOLVED
Closed: 10 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
•