[issue25525] Deallocation scheme for memoryview is unsafe

[Stefan Krah]

Stefan Krah added the comment:

Thanks, no big problem.  The thing is that the parts I wrote
(Modules/_decimal/*, Objects/memoryobject.c, Modules/_testbuffer.c)
have been audited rather heavily and have 100% code coverage
with a privately maintained patch that inserts allocation
failures.

There are even tests in test_memoryview() / test_buffer() for
breaking up reference cycles.

So I'd really prefer not to have things labeled as potential issues
when there are none. That also applies to the PyMem_NEW() "potential
overflow" fix in _testbuffer.c where overflow was impossible.

Thus, I'll probably revert that at some point, not out of
hostility, but just because it's not my approach to software
development (in the _testbuffer case, I view the 'len' parameter
as a constrained type in the Ada sense where 0 <= len <= ndim = 64).

--

___
Python tracker 

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

[issue25525] Deallocation scheme for memoryview is unsafe

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Strange bug. I can't attach any file.

--

___
Python tracker 

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

[issue25525] Deallocation scheme for memoryview is unsafe

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Sorry for my bad wording. I wasn't going to blame or insult anybody.

--

___
Python tracker 

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

[issue25525] Deallocation scheme for memoryview is unsafe

[Stefan Krah]

Stefan Krah added the comment:

First of all, the premise "exports > 0" in your example looks wrong
to me.  The deallocation process for the first view should start precisely when 
it no longer has any exports.

In fact, the check for "exports > 0" is for the case when
memoryview.release() is called from the Python level.


Secondly, even if it did happen (show the code path leading
to that!), BufferError would be set in memory_clear() and the
garbage collector would throw a FatalError, i.e. step 5+ would
not be reached.


Lastly, I don't find it very diplomatic to use language like "Deallocation 
scheme for memoryview is complex and unsafe. It
crashes with chained memoryviews..." when you don't seem to
a test case or a clear concept of how the alleged bug should
occur.

--
resolution:  -> not a bug
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

[issue25525] Deallocation scheme for memoryview is unsafe

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

WTF? Where is my patch?

--

___
Python tracker 

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

[issue25525] Deallocation scheme for memoryview is unsafe

[Serhiy Storchaka]

New submission from Serhiy Storchaka:

Deallocation scheme for memoryview is complex and unsafe. It crashes with 
chained memoryviews (issue25498), but I suppose deallocating unchained 
memoryview can crash too if the memoryview itself had exported buffers 
(self->exports != 0).

Both memoryview and ManagedBuffer support garbage collector. If there is a 
reference to memoryview from reference loop, memoryview becomes a part of the 
reference loop.

refloop -> memoryview -> ManagedBuffer -> obj

First garbage collector calls tp_clear for all objects in the loop 
(memory_clear() for memoryview).
If self->exports != 0 for memoryview, _memory_release() fails and the 
_Py_MEMORYVIEW_RELEASED flag is not set. However following Py_CLEAR(self->mbuf) 
deallocates ManagedBuffer and set self->mbuf to NULL. Then memoryview's owner 
releases memoryview, and it is deallocated (memory_dealloc). _memory_release 
reads self->mbuf->exports, but self->mbuf is NULL. Segmentation fault.

Following patch makes deallocation scheme for memoryview simpler and more 
reliable.

1) memory_clear does nothing if self->exports != 0. The buffer will be released 
when memoryview's owner release the memoryview.

2) ManagedBuffer no longer supports garbage collector. This prevents buffer 
releasing before memoryview or its owner are cleared.

--
components: Interpreter Core
messages: 253803
nosy: serhiy.storchaka, skrah
priority: normal
severity: normal
stage: patch review
status: open
title: Deallocation scheme for memoryview is unsafe
type: behavior
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

[issue25525] Deallocation scheme for memoryview is unsafe

[Stefan Krah]

Stefan Krah added the comment:

The "chained memoryviews" you refer to are a hack and simply
aren't supported. Please stop spreading FUD.

--

___
Python tracker 

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

[issue25525] Deallocation scheme for memoryview is unsafe

[Martin Panter]

Martin Panter added the comment:

Did you forget your patch? :)

--
nosy: +martin.panter

___
Python tracker 

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

[issue25525] Deallocation scheme for memoryview is unsafe

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Yes, the "chained memoryviews" in ctypes are a hack and it will be eliminated. 
But this hack exposed possible weak point in memoryview.

--

___
Python tracker 

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