memoryview can set an exception in tp_clear

[serhiy-storchaka]

BPO	33713
Nosy	@pitrou, @vstinner, @skrah, @serhiy-storchaka

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2018-05-31.12:15:10.896>
labels = ['interpreter-core', '3.10', '3.9', 'type-crash', '3.11']
title = 'memoryview can set an exception in tp_clear'
updated_at = <Date 2021-10-18.20:19:58.191>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2021-10-18.20:19:58.191>
actor = 'iritkatriel'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2018-05-31.12:15:10.896>
creator = 'serhiy.storchaka'
dependencies = []
files = []
hgrepos = []
issue_num = 33713
keywords = []
message_count = 14.0
messages = ['318288', '318291', '318295', '318296', '318297', '318298', '318299', '318300', '318306', '318316', '318318', '318319', '318320', '318586']
nosy_count = 4.0
nosy_names = ['pitrou', 'vstinner', 'skrah', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue33713'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

Linked PRs

• gh-77894: Fix a crash when the GC breaks a loop containing a memoryview #123898
• [3.13] gh-77894: Fix a crash when the GC breaks a loop containing a memoryview (GH-123898) #123936
• [3.12] gh-77894: Fix a crash when the GC breaks a loop containing a memoryview (GH-123898) #123937

[serhiy-storchaka]

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 bpo-33622), 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.

[skrah]

Could you please show how tp_clear() can be called when self->exports > 0? It should not happen.

bpo-33622 is a big issue with many commits. Would it be possible to extract the relevant part?

[serhiy-storchaka]

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.

[skrah]

This looks the same as bpo-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.

[serhiy-storchaka]

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.

[skrah]

Yes, but who calls tp_clear() if the memoryview is not being deallocated?

[serhiy-storchaka]

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.

[skrah]

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.

[pitrou]

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

[skrah]

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?

[vstinner]

If the bug cannot occur, just add "assert(!PyErr_Occurred());" no?