-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-31626: Fixed a bug in debug memory allocator. #3844
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-31626: Fixed a bug in debug memory allocator. #3844
Conversation
There was a write to freed memory after shrinking a memory block.
Objects/obmalloc.c
Outdated
if (q == NULL) | ||
if (q == NULL) { | ||
if (nbytes <= original_nbytes) { | ||
Py_FatalError("Shrinking reallocation is failed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the fatal error? If a shrinking realloc()
fails, the old memory is still valid (just too large).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why NULL is returned instead of the old memory? This looks as a bug in the underlying allocator. I think a load crash is better than silent memory leak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I have to think about this more. I'm confused by the recursion and I don't know what the intended behavior of api->alloc.realloc()
is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the PEP example:
void* my_realloc(void *ctx, void *ptr, size_t new_size)
{
int padding = *(int *)ctx;
return realloc(ptr, new_size + padding);
}
returning NULL and leaving the existing memory as is looks correct to me. That's how realloc()
itself behaves.
[still confused by the recursion, but that seems a separate issue.]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't call Py_FatalError() in a memory allocator.
Objects/obmalloc.c
Outdated
@@ -1477,20 +1477,18 @@ _PyMem_DebugRawRealloc(void *ctx, void *p, size_t nbytes) | |||
/* overflow: can't represent total as a Py_ssize_t */ | |||
return NULL; | |||
|
|||
if (nbytes < original_nbytes) { | |||
/* shrinking: mark old extra memory dead */ | |||
memset(q + nbytes, DEADBYTE, original_nbytes - nbytes + 2*SST); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot do that directly. If realloc() fails, you must restore original bytes.
Would it possible to allocate a new temporary memory block to save these erased bytes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can require allocating large block.
This is why I added Py_FatalError(). If realloc() fails when shrinking memory block, something is wrong with memory allocator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can require allocating large block.
I'm not sure that it's a big issue. You should be prepared for memory allocation failures when you use debug hooks since they add a large overhead on memory footprint.
If you care, you might reimplement realloc as new=alloc()+memcpy(new,old)+free(old). "free(old)" will invalidate old bytes (before freeing memory) for you if I recall correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If realloc() fails when shrinking memory block, something is wrong with memory allocator.
It is C-Standard conforming for realloc()
to fail when shrinking the size. There are legitimate reasons to reshuffle blocks when shrinking (compact the heap). If that fails, realloc()
has to signal to the caller that the requested size cannot be matched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is C-Standard conforming for
realloc()
to fail when shrinking the size.
I have not found an explicit specification of this behavior in the C Standard, but your arguments make sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that it's a big issue. You should be prepared for memory allocation failures when you use debug hooks since they add a large overhead on memory footprint.
This complicates the implementation. Could you create a new PR for your suggestion?
If you care, you might reimplement realloc as new=alloc()+memcpy(new,old)+free(old). "free(old)" will invalidate old bytes (before freeing memory) for you if I recall correctly.
I don't like this. Always returning a new pointer can hide a common bug with misusing realloc API. Debug allocator should help to find bugs, not hide them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not found an explicit specification of this behavior in the C Standard [...]
7.20.3.4 The realloc function
"The realloc function deallocates the old object pointed to by ptr and returns a
pointer to a new object that has the size specified by size."
"The realloc function returns a pointer to the new object (which may have the same
value as a pointer to the old object), or a null pointer if the new object could not be
allocated."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @skrah.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This complicates the implementation. Could you create a new PR for your suggestion?
Done: PR #4119.
I wrote a different fix: #3898 rewrites the debug hook to not call realloc() anymore, but use malloc()+free(), reusing debug hooks for malloc() and free(). |
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6. |
Removed a code that incorrectly detected in-place resizing in realloc() and wrote to freed memory. (cherry picked from commit b484d56)
GH-4191 is a backport of this pull request to the 3.6 branch. |
Removed a code that incorrectly detected in-place resizing in realloc() and wrote to freed memory.
There was a write to freed memory after shrinking a memory block.
https://bugs.python.org/issue31626