bpo-31626: Fix _PyObject_DebugReallocApi() #4310
Conversation
|
As part of efforts to make Python memory allocators "correct", I propose to "fix" _PyObject_DebugReallocApi(). For the rationale, see: |
|
@skrah: In the PR #3844, you wrote "Why the fatal error? If a shrinking realloc() fails, the old memory is still valid (just too large)." @serhiy-storchaka wrote a smart and correct implementation in PR #4210 for the master branch: save erased bytes, to be able to restore them on failure. But this implementation is complex. Our Python 3.6 release manager, @ned-deily, is against backporting this "correct" fix to Python 3.6: "My reaction is that the risk due to complexity of the changes outweigh the benefits so I would agree with @serhiy-storchaka that we should not backport this." In debug mode, Python 2 always erased bytes before calling realloc(). If realloc() fails on shrinking a memory block... the original memory block is left modfied (with erased bytes). My PR makes the code failing with a fatal error in this case which should never happen. So I propose to be optimistic and use the Python 2 code in Python 3.6, but fix the code to trigger a fatal error if realloc() fails whereas it should never fail :-) |
|
Once this PR will be merged, I will prepare a PR for Python 3.6. |
|
I'm not sure that it is worth to make the code crashing in maintained release. If the shrinking realloc() fails in real world, there is a chance that the third party code will just fail with MemoryError. Or will never read erased memory. With this change it will crash immediately. This doesn't look an enhancement or bug fix. |
Objects/obmalloc.c
Outdated
| @@ -1562,8 +1562,14 @@ _PyObject_DebugReallocApi(char api, void *p, size_t nbytes) | |||
| * but we live with that. | |||
| */ | |||
| q = (uchar *)PyObject_Realloc(q - 2*SST, total); | |||
| if (q == NULL) | |||
| if (q == NULL) { | |||
| if (nbytes < original_nbytes) { | |||
There was a problem hiding this comment.
Should this be <=? I know it's checking the memset logic above, but I think we assume the "shrinking" to the same size always works, too.
There was a problem hiding this comment.
Right. I changed the code for <=
Objects/obmalloc.c
Outdated
| if (nbytes < original_nbytes) { | ||
| /* bpo-31626: the memset() above expects that realloc never fails | ||
| on shrinking a memory block. */ | ||
| Py_FatalError("Shrinking reallocation is failed"); |
_PyObject_DebugReallocApi() now calls Py_FatalError() if realloc() fails to shrink a memory block. Call Py_FatalError() because _PyObject_DebugReallocApi() erased freed bytes *before* realloc(), expecting that realloc() *cannot* fail to shrink a memory block.
|
@serhiy-storchaka: "If the shrinking realloc() fails in real world, there is a chance that the third party code will just fail with MemoryError. Or will never read erased memory." If the application uses the memory block after the realloc failure, the application can crash because of a bug in Python memory allocation. @serhiy-storchaka: "This doesn't look an enhancement or bug fix." realloc() must leave the memory block unchanged on failure. It's not the cause currently, so it's a bug and my change is a bug fix. |
|
@benjaminp: So, what do you think of this PR? |
|
I will merge this PR this week if nobody complains against it. |
_PyObject_DebugReallocApi() now calls Py_FatalError() if realloc()
fails to shrink a memory block.
Call Py_FatalError() because _PyObject_DebugReallocApi() erased freed
bytes before realloc(), expecting that realloc() cannot fail to
shrink a memory block.
https://bugs.python.org/issue31626