Skip to content

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

Merged

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Oct 1, 2017

There was a write to freed memory after shrinking a memory block.

https://bugs.python.org/issue31626

There was a write to freed memory after shrinking a memory block.
if (q == NULL)
if (q == NULL) {
if (nbytes <= original_nbytes) {
Py_FatalError("Shrinking reallocation is failed");
Copy link
Contributor

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).

Copy link
Member Author

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.

Copy link
Contributor

@skrah skrah Oct 1, 2017

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.

Copy link
Contributor

@skrah skrah Oct 1, 2017

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.]

Copy link
Member

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.

@@ -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);
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Contributor

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."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @skrah.

Copy link
Member

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.

@vstinner
Copy link
Member

vstinner commented Oct 5, 2017

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().

@serhiy-storchaka serhiy-storchaka merged commit b484d56 into python:master Oct 31, 2017
@miss-islington
Copy link
Contributor

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

@serhiy-storchaka serhiy-storchaka deleted the debug-realloc-deadbytes branch October 31, 2017 12:05
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 31, 2017
Removed a code that incorrectly detected in-place resizing in realloc()
 and wrote to freed memory.
(cherry picked from commit b484d56)
@bedevere-bot
Copy link

GH-4191 is a backport of this pull request to the 3.6 branch.

serhiy-storchaka pushed a commit that referenced this pull request Oct 31, 2017
Removed a code that incorrectly detected in-place resizing in realloc()
 and wrote to freed memory.
(cherry picked from commit b484d56)
embray pushed a commit to embray/cpython that referenced this pull request Nov 9, 2017
Removed a code that incorrectly detected in-place resizing in realloc()
 and wrote to freed memory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants