classification
Title: memoryview can set an exception in tp_clear
Type: crash Stage:
Components: Interpreter Core Versions: Python 3.8, Python 3.7, Python 3.6, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: pitrou, serhiy.storchaka, skrah, vstinner
Priority: normal Keywords:

Created on 2018-05-31 12:15 by serhiy.storchaka, last changed 2018-06-03 21:37 by skrah.

Messages (14)
msg318288 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-05-31 12:15
The tp_clear handler of memoryview can set an exception when fail to release the buffer. An exception in tp_clear is not expected and caused a crash in the garbage collector. In the master branch it will cause just writing a traceback to stderr (see issue33622), but in any case it would be better to handle the failure locally in memoryview. I don't know what is the best solution: silencing an error, writing a traceback with more detailed information, or resurrecting the buffer object.
msg318291 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2018-05-31 12:32
Could you please show how tp_clear() can be called when self->exports > 0? It should not happen.

#33622 is a big issue with many commits.  Would it be possible to extract the relevant part?
msg318295 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-05-31 12:41
I don't know how to reproduce a failure in tp_clear(). I just can't prove that it never fails. Maybe it is needed a bug in the implementation of the buffer protocol in third-party extension.

If it should not happen then we can just add

    assert(!PyErr_Occurred());

or

    if (PyErr_Occurred()) {
        PyErr_WriteUnraisable(NULL);
    }

It is better to crash in memoryview.c than in the garbage collector if this crash is caused by incorrect buffer protocol implementation.
msg318296 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2018-05-31 12:47
This looks the same as #25525.  I think it cannot happen, and no one has ever reported an actual issue for 6 years now.

You *really* need to show a reproducer if you assert that something can crash.
msg318297 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-05-31 12:49
See the delete_garbage() function line 770 in Modules/gcmodule.c for changes in the master branch relevant to this issue. See Py_FatalError() in the collect() function at line 974 for a crash.
msg318298 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2018-05-31 12:53
Yes, but who calls tp_clear() if the memoryview is not being deallocated?
msg318299 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-05-31 13:03
The GC calls tp_clear() if the memoryview is a part of the reference loop.

a = [memoryview(...)]
a.append(a)
del a

The GC will call tp_clear() of the list or the memoryview. What will be called first is not specified.
msg318300 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2018-05-31 13:06
The point is that *no garbage collection is triggered* if self->exports > 0.

It would be a major bug if it were and I suspect it would be reported within a week.  Fortunately, no such bug has been reported in 6 years.
msg318306 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-05-31 13:23
Serhiy is right about the theoretical concern here.  However, it's probably quite difficult to find a concrete situation where this occurs, because we're talking about mbuf_clear and the managerbuffer object can't really get involved in a reference cycle by itself (not in normal use anyway): the memoryview object does, but it's a different thing.

By the way: PyBuffer_Release() returns void, so IMO it's a bug if it can return with an exception set.  We should fix that rather than focus on mbuf_clear().
msg318316 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2018-05-31 14:15
Well, the example would need exports:

>>> a = [bytes()]
>>> a.append(memoryview(a[0]))
>>> a.append(memoryview(a[1]))
>>> a.append(a)
>>> a
[b'', <memory at 0x1ad21f8>, <memory at 0x1b52348>, [...]]


The first memoryview has one export, so its refcount > 0.

Do I fundamentally misunderstand tp_clear() and tp_clear() can be called on objects with refcount > 0?
msg318318 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-05-31 14:18
If the bug cannot occur, just add "assert(!PyErr_Occurred());" no?
msg318319 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2018-05-31 14:19
Well yes, I still want to understand tp_clear(). :)

The docs are a bit vague.
msg318320 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2018-05-31 14:20
Yes, tp_clear can be called with refcount > 0.  It's exactly why it's
separate from tp_dealloc, actually :-)
msg318586 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2018-06-03 21:37
Okay that makes sense. :)

I looked a bit at the gc code. A consumer object always has one
reference to a memoryview with an export, which isn't visited. So
it looks to me that the gc_refs of that memoryview cannot fall to 0.

So memory_clear() isn't called in that case, but mbuf_clear() is, which
is known and expected to handle mbuf->exports >= 0.

Indeed let's perhaps just add "if (self->exports > 0) return 0" to memory_clear() if those assumptions are too complex.
History
Date User Action Args
2018-06-03 21:37:07skrahsetmessages: + msg318586
2018-05-31 14:20:54pitrousetmessages: + msg318320
2018-05-31 14:19:42skrahsetmessages: + msg318319
2018-05-31 14:18:12vstinnersetnosy: + vstinner
messages: + msg318318
2018-05-31 14:15:09skrahsetmessages: + msg318316
2018-05-31 13:23:58pitrousetmessages: + msg318306
2018-05-31 13:06:58skrahsetmessages: + msg318300
2018-05-31 13:03:11serhiy.storchakasetmessages: + msg318299
2018-05-31 12:53:12skrahsetmessages: + msg318298
2018-05-31 12:49:39serhiy.storchakasetmessages: + msg318297
2018-05-31 12:47:53skrahsetmessages: + msg318296
2018-05-31 12:41:23serhiy.storchakasetmessages: + msg318295
2018-05-31 12:32:54skrahsetnosy: + pitrou
messages: + msg318291
2018-05-31 12:15:10serhiy.storchakacreate