[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

> * 3.6: don't memset() --> correct, but don't detect "use after free" bug
> when a memory block is skrinked

And doesn't detect  "use after free" bug when a memory block is expanded. And 
2.7 doesn't detect this. Only 3.7 detects this.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

Change by STINNER Victor :


--
versions: +Python 2.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

STINNER Victor  added the comment:

Ok, let me summarize:

* 2.7: memset() *before* realloc() if shrinking a memory block, but crash when 
Py_FatalError() if realloc() fails on shrinking --> cannot corrupt memory, but 
can crash if realloc() fails on shrinking

* master (3.7): memset() *before* realloc() if shrinking a memory block, but 
save erased bytes, and restore erased bytes if realloc() fails --> always 
correct

* 3.6: don't memset() --> correct, but don't detect "use after free" bug when a 
memory block is skrinked

Python 3.6 release manager, Ned Deily, rejected the proposal to backport the 
complex fix from master to 3.6.


Victor (me):
> Serhiy: I added a Py_FatalError() to Python 2.7 if the case that must not 
> happen does happen. Are you ok to apply the same change for Python 3.6 (...)

Sorry, when I asked the question, I expected that Python 3.6 still erased bytes 
before realloc(). But it's not the case. I agree that Py_FatalError() would be 
a bad idea for Python 3.6.


Serhiy: Thank you for fixing Python 3.6 (don't memset() *after* realloc, which 
crashed on OpenBSD) and "fix" the feature in master (restore erased bytes if 
realloc fails)!


I close the issue. I don't think that Python 2.7 or 3.6 need further changes.

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

I prefer to keep the current code as is.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

STINNER Victor  added the comment:

Serhiy: I added a Py_FatalError() to Python 2.7 if the case that must not 
happen does happen. Are you ok to apply the same change for Python 3.6 (commit 
ed4743a2f2bb8d88f7f1aa9307794be9a44f1e0f), or do you prefer to backport your 
more complex patch using a small buffer allocated on the stack?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

STINNER Victor  added the comment:


New changeset ed4743a2f2bb8d88f7f1aa9307794be9a44f1e0f by Victor Stinner in 
branch '2.7':
bpo-31626: Fix _PyObject_DebugReallocApi() (#4310)
https://github.com/python/cpython/commit/ed4743a2f2bb8d88f7f1aa9307794be9a44f1e0f


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

STINNER Victor  added the comment:

For allocations larger than 512 bytes, PyObject_Malloc() calls 
PyMem_RawMalloc(). When debug hooks are installed, PyObject_Malloc() calls a 
debug hook which calls PyMem_RawMalloc() which calls another debug hook.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Yet one issue is left. In debug mode two debug allocators are used, the one is 
nested in the other. Is it correct?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +4271

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

There are large discussions at PR 3844 and PR 4210.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


New changeset 3cc4c53a64bdcf21431ad306eca0e568f88735a2 by Serhiy Storchaka in 
branch 'master':
bpo-31626: Mark ends of the reallocated block in debug build. (#4210)
https://github.com/python/cpython/commit/3cc4c53a64bdcf21431ad306eca0e568f88735a2


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
pull_requests: +4179

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

STINNER Victor  added the comment:

While the commit b484d5606ca76f9bbd0f5de7a6ef753400213e94 fixes a crash on 
OpenBSD and makes the code "correct" on any platforms, I still see it as a 
feature regression. Detecting buffer overflow and using freed memory are 
feature of debug hooks.

https://docs.python.org/dev/c-api/memory.html#c.PyMem_SetupDebugHooks

So yes, debug-realloc.diff is interesting.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
priority: critical -> normal

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Larry Hastings]

Larry Hastings  added the comment:

> Most, if not all, calls to _PyMem_DebugRawRealloc() are protected by
> the GIL. If there is a single thread using the memory block,
> I think that it's perfectly fine to write after it's deallocated.

I don't quite follow where the write-after-free is happening, but: C's free() 
function is *not* protected by the GIL.  So if you're running in a 
multithreaded program where other threads aren't blocked by the GIL, one of 
these other threads could very easily allocate this freshly-freed memory.  And 
if you wrote to it after the memory was allocated to this other thread, 
congrats, your program is no longer correct.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

I have removed the incorrect code in master and 3.6, this unblocks testing 
debug build on OpenBSD. I don't think it is worth to backport this change to 
3.5 and 3.4, since the bug affects only debug build.

Here is a patch which perhaps is an alternative to PR 4119, but without 
disturbing the heap allocator. It erases just few bytes at the begin and at the 
end of the allocated block (including the header and the trailer), and saves 
erased bytes in small local buffer. It does this even if the block is not 
reallocated in-place. Hence calling free() on reallocated block will be 
detected. Reading from the reallocated block will get erased bytes at the start 
and the end, this is likely will cause to loud failure too. I have tested in on 
OpenBSD.

But I'm not sure that it is worth to apply this patch. Left it on to you Victor.

--
versions:  -Python 3.4, Python 3.5
Added file: https://bugs.python.org/file47249/debug-realloc.diff

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


New changeset ece5659565e083baaee4d185ce181a98aaee7f96 by Serhiy Storchaka 
(Miss Islington (bot)) in branch '3.6':
bpo-31626: Fixed a bug in debug memory allocator. (GH-3844) (#4191)
https://github.com/python/cpython/commit/ece5659565e083baaee4d185ce181a98aaee7f96


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Roundup Robot]

Change by Roundup Robot :


--
pull_requests: +4161

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


New changeset b484d5606ca76f9bbd0f5de7a6ef753400213e94 by Serhiy Storchaka in 
branch 'master':
bpo-31626: Fixed a bug in debug memory allocator. (#3844)
https://github.com/python/cpython/commit/b484d5606ca76f9bbd0f5de7a6ef753400213e94


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Stefan Krah]

Stefan Krah  added the comment:

Ah sorry, you mean it cannot write the special bytes.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Stefan Krah]

Stefan Krah  added the comment:

> realloc() must not touch the original buffer on failure

I don't understand this: If realloc() fails, the original buffer
is perfectly valid.

--
nosy: +skrah

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Xavier de Gaye]

Change by Xavier de Gaye :


--
nosy: +xdegaye

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

The current code OBVIOUSLY is wrong. Bytes are erased if q == oldq && nbytes < 
original_nbytes. But q == oldq only if realloc() returns the new address 
2*sizeof(size_t) bytes larger than its argument. This is virtually never happen 
on other platforms because _PyMem_DebugRawRealloc() usually used with blocks 
larger than 2*sizeof(size_t) bytes and the system realloc() don't shrink the 
block at left (this is implementation detail). Thus this code is virtually dead 
on other platforms. It doesn't detect shrinking memory block in-place.

After fixing *this* bug, we have encountered with *other* bug, related to 
overwriting the freed memory.

I don't see reasons of keeping an obviously wrong code. When fix the first bug 
we will need to fix the other bug.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

STINNER Victor  added the comment:

"PR 4119 is too complex for a bugfix (especially if backport it to 3.5 and 
3.4). It can introduce other regressions."

Which kind of regression do you expect? Something like a crash?


"The performance hit is not the only issue. Allocating a temporary buffer can 
change the structure of "holes" in memory. As result some memory related bugs 
can be reproducible only in release mode."

Can you please elaborate how the exact memory layout can trigger or not bugs in 
the code?

I'm not saying that you are right or wrong. I just fail to see why the memory 
address and "holes" would trigger or not bugs.

What I can understand is a subtle behaviour change if realloc() returns the 
same memory block or a new memory block. But we cannot control that.

I'm not sure that allocating "copy" before realloc() would impact the behaviour 
of realloc(). The common case is that a memory block is a few bytes smaller, 
and so the realloc returns the same memory block, but now becomes able to use 
the unallocated bytes for a new allocation later, no?


"Maybe it is enough to erase only few bytes at the start and end of the freed 
area. The copy can be saved in local variables, without involving the heap. 
This solution still will be enough complex, and I think it can be applied only 
to 3.7. But the bug should be fixed in all affected versions."

If we must make a choice, I would prefer to keep the current behaviour: make 
sure that unallocated bytes are erased. Catching code reading unallocated bytes 
is the most important feature of debug hooks, no?

I dislike the idea of only erasing "some" bytes: this change may prevent to 
detect some bugs.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

STINNER Victor  added the comment:

"the bug should be fixed in all affected versions"

I don't understand why you insist to change Python 3.4 and Python 3.5. IMHO 
this issue only impacts OpenBSD.


"The current code in not correct on all platforms. We don't know how many of 
random bugs, hangs and crashes on other platforms are caused by this bug. I'm 
not surprised that it was caught on OpenBSD since the robustness and security 
of software is the goal of OpenBSD."

I'm not aware of these "random bugs, hangs and crashes on other platforms". Did 
you hear someone complaining about random crashes in Python?

We are running the Python suite multiple times per time on a large farm of 
buildbot workers. I  never saw the crashes that you mentionned.

IMHO it's very unlikely or impossible that _PyMem_DebugRawRealloc() erases 
bytes of a memory block that was just unallocated while another thread uses 
this new memory block for a new allocation. Most, if not all, calls to 
_PyMem_DebugRawRealloc() are protected by the GIL. If there is a single thread 
using the memory block, I think that it's perfectly fine to write after it's 
deallocated.

Well, maybe I'm plain wrong, and it's possible that shrinking a memory block 
makes the unallocator memory block really unaccessible, and that the memcpy() 
after the realloc() triggers a big crash. But I would like to *see* such crash 
to really be convinced :-)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

In this case it would be safe to not erase bytes at all.

PR 4119 is too complex for a bugfix (especially if backport it to 3.5 and 3.4). 
It can introduce other regressions. The performance hit is not the only issue. 
Allocating a temporary buffer can change the structure of "holes" in memory. As 
result some memory related bugs can be reproducible only in release mode.

Maybe it is enough to erase only few bytes at the start and end of the freed 
area. The copy can be saved in local variables, without involving the heap. 
This solution still will be enough complex, and I think it can be applied only 
to 3.7. But the bug should be fixed in all affected versions.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

STINNER Victor  added the comment:

> The current code in not correct on all platforms.

My PR 4119 only changes the behaviour on OpenBSD, but I'm not sure about that. 
Maybe it's simpler to apply this fix on all platforms, as I asked on the PR 
itself?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

STINNER Victor  added the comment:

> PR 3844 restores the behavior of 2.7 and 3.3. I propose to merge it first, 
> and develop other enhancements later.

Please don't. This PR reintroduced a bug that I fixed in bpo-18408:

commit c4266360fc70745d49b09f2c29cda91c1a007525
Author: Victor Stinner 
Date:   Tue Jul 9 00:44:43 2013 +0200

Issue #18408: Fix _PyMem_DebugRealloc()

Don't mark old extra memory dead before calling realloc(). realloc() can 
fail
and realloc() must not touch the original buffer on failure.

So mark old extra memory dead only on success if the new buffer did not move
(has the same address).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

The current code in not correct on all platforms. We don't know how many of 
random bugs, hangs and crashes on other platforms are caused by this bug. I'm 
not surprised that it was caught on OpenBSD since the robustness and security 
of software is the goal of OpenBSD.

PR 3844 restores the behavior of 2.7 and 3.3. I propose to merge it first, and 
develop other enhancements later.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

STINNER Victor  added the comment:

"I tried to build CPython on 64-bit OpenBSD. It was built successfully, but 
tests crash."

It's the first time that anyone complains about _PyMem_DebugRawRealloc(). The 
behaviour seems to be very specific to OpenBSD.

Even if the current code is "weird", it works very well and is effecient.

I proposed attached PR 4119 which makes _PyMem_DebugRawRealloc() behaves 
correctly: erase bytes *before* calling realloc(), but keeps a copy if 
realloc() fails. My PR only changes the behaviour on OpenBSD. It keeps the 
current behaviour (erase bytes *after* realloc, if realloc succeeded) on other 
platforms for performance reasons.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +4089

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

STINNER Victor  added the comment:

> Using nested _PyMem_DebugRawRealloc() looks suspicions to me. This may be a 
> bug.

PyObject_Malloc() calls PyMem_RawMalloc() for allocations larger than 512 bytes.

When debug hooks are enabled, PyObject_Malloc() and PyMem_RawMalloc() both call 
_PyMem_DebugRawRealloc(). The behaviour that you saw is expected.

It was simpler to reuse _PyMem_DebugRawRealloc() PyObject and PyMem_Raw 
allocator families, rather than duplicating the code.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Ping.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +3870

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue31626] Writing in freed memory in _PyMem_DebugRawRealloc() after shrinking a memory block

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Using nested _PyMem_DebugRawRealloc() looks suspicions to me. This may be a bug.

But even without nested _PyMem_DebugRawRealloc() writing to the extra memory 
after using realloc() looks wrong to me. This can break other non-trivial 
system allocators which write an information past the allocated block. This can 
cause a segfault if unused memory pages are returned to OS.

After creating the PR I have found that it literally restores the code of 2.7 
and 3.3. 3.4 and later contain this bug. The bug looks enough serious to me for 
fixing it in 3.4 and 3.5.

--
nosy: +larry
title: Crash in _PyUnicode_DecodeUnicodeEscape on OpenBSD -> Writing in freed 
memory in _PyMem_DebugRawRealloc() after shrinking a memory block
versions: +Python 3.4, Python 3.5, Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com