gh-122306: Fix circular reference issue between MemoryView and PickleBuffer during garbage collection

[marko1616]

Issues

• Issue:Crash after error in PickleBuffer #122306

Main Causes

1. Creating a circular reference through a constructed exception, preventing garbage collection of PickleBuffer and memory objects
2. Calling gc.collect()
3. During the collection of the memory object, PyMemoryViewObject->mbuf is cleared for the first time (mbuf is set to a null pointer)
4. When collecting PickleBuffer, memory_dealloc is called again through picklebuf_clear, at which point an exception occurs because mbuf was set to a null pointer in step 3

Next

I have no idea if this is a good way to fix this.But thank for any dev who reviewed this PR.

• Issue: Crash after error in PickleBuffer #122306

[ghost]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[vstinner]

cc @serhiy-storchaka

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[encukou]

I also don't know a good way to fix this, but a leak with an unraisable error message is better than a crash.

I plan to merge in about a week if there are no objections.

[serhiy-storchaka]

This change needs a test. test_non_continuous_buffer in Lib/test/pickletester.py can be an indirect test (it should be modified for this), but since this change does not use pickle.PickleBuffer, it needs more direct test that only uses memoryview.

I am not sure that this is a right solution, but the test will help us to decide.

[marko1616]

This change needs a test. test_non_continuous_buffer in Lib/test/pickletester.py can be an indirect test (it should be modified for this), but since this change does not use pickle.PickleBuffer, it needs more direct test that only uses memoryview.

I am not sure that this is a right solution, but the test will help us to decide.

It's really hard to trigger the crash using just memoryview because we first need to construct an exported buffer that will clear the view. I tried, but I failed.
I'm consider adding following test in Lib/test/pickletester.py.

deftest_circular_reference_gc(self):
mv=memoryview(b"foobar")
error=Noneunraisable_exception=Nonedefcustom_unraisablehook(unraisable):
nonlocalunraisable_exceptionunraisable_exception=unraisabledeffunc():
try:
pb=pickle.PickleBuffer(mv)
raiseCustomErrorexceptCustomErrorase:
# Ensure `error` references the exceptionerror=efunc()
delmvprev_unraisablehook=sys.unraisablehooksys.unraisablehook=custom_unraisablehooksupport.gc_collect()
self.assertEqual(unraisable_exception.exc_type, BufferError)
self.assertIsInstance(unraisable_exception.exc_value, BufferError)
self.assertEqual(str(unraisable_exception.exc_value), 'memoryview has 1 exported buffer')
sys.unraisablehook=prev_unraisablehook

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[serhiy-storchaka]

#123898 uses a different solution. Trying to break a reference loop is a normal situation, it should not produce error reports (the user cannot do anything about this in any case), so _memory_release() should not even be called if self->exports != 0. That PR also contains tests.