segfaults when using __del__ and weakrefs

[cfbolz]

BPO	1377858
Nosy	@mwhudson, @gvanrossum, @loewis, @brettcannon, @arigo, @birkenfeld, @cfbolz
Files	minimalcrash.py: minimal example weakref_in___del__.diff: clear new weakrefs after calling the finalizer clear_weakref.diff: patch Rename README to README.rst and enhance formatting #2 clear_weakref_w_test.diff: patch Rename README to README.rst and enhance formatting #2 w/ extra code comment and test

^{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 = 'https://github.com/brettcannon'
closed_at = <Date 2008-01-24.18:59:43.514>
created_at = <Date 2005-12-10.21:20:45.000>
labels = ['interpreter-core']
title = 'segfaults when using __del__ and weakrefs'
updated_at = <Date 2008-01-24.18:59:43.514>
user = 'https://github.com/cfbolz'

bugs.python.org fields:

activity = <Date 2008-01-24.18:59:43.514>
actor = 'gvanrossum'
assignee = 'brett.cannon'
closed = True
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2005-12-10.21:20:45.000>
creator = 'Carl.Friedrich.Bolz'
dependencies = []
files = ['1843', '1844', '1845', '1846']
hgrepos = []
issue_num = 1377858
keywords = []
message_count = 12.0
messages = ['27025', '27026', '27027', '27028', '27029', '27030', '27031', '27032', '27033', '27034', '27035', '61650']
nosy_count = 8.0
nosy_names = ['mwh', 'gvanrossum', 'loewis', 'nnorwitz', 'brett.cannon', 'arigo', 'georg.brandl', 'Carl.Friedrich.Bolz']
pr_nums = []
priority = 'critical'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue1377858'
versions = ['Python 2.5']

[cfbolz]

You can segfault Python by creating a weakref to an
object in its __del__ method, storing it somewhere and
then trying to dereference the weakref afterwards. the
attached file shows the described behaviour.

[mwhudson]

Logged In: YES
user_id=6656

Hmm, I was kind of hoping this report would get more attention.

The problem is obvious if you read typeobject.c around line 660: the weakref
list is cleared before __del__ is called, so any weakrefs added during the
execution of __del__ are never informed of the object's death. One fix for this
would be to clear the weakref list _after_ calling __del__ but that led to other
mayhem in ways I haven't boethered to understand <wink> (see SF bug
bpo-742911). I guess we could just clear out any weakrefs created in __del__
without calling their callbacks.

[mwhudson]

Logged In: YES
user_id=6656

Hmm, maybe the referenced mayhem is more to do with clearing __dict__ than
calling __del__. What breaks if we do things in this order:

1. call __del__
2. clear weakrefs
3. clear __dict__

?

[birkenfeld]

Logged In: YES
user_id=1188172

Added to outstanding_crashes.py.

[brettcannon]

Logged In: YES
user_id=357491

So after staring at this crasher it seemed to me to be that
clearing the new weakrefs w/o calling their finalizers after
calling the object's finalizer was the best solution. I
couldn't think of any other good way to communicate to the
new weakrefs that the object they refer to was no longer
viable memory without doing clear_weakref() work by hand.

Attached is a patch to do this. Michael, can you have a look?

[arigo]

Logged In: YES
user_id=4771

The clear_weakref(*list) only clears the first
weakref to the object. You need a while loop
in your patch. (attached proposed fix)

Now we're left with fixing the same bug in
old-style classes (surprize surprize), and
turning the crasher into a test.

[brettcannon]

Logged In: YES
user_id=357491

After finally figuring out where *list was made NULL (and
adding a comment about it where it occurs), I added a test
to test_weakref.py . Didn't try to tackle classic classes.

[nnorwitz]

Brett, Michael, Armin, can we get this patch checked in for 2.5.1?

[brettcannon]

I have just been waiting on someone to do a final code review on it. As soon as someone else signs off I will commit it.

[loewis]

The first comment has a non-sensical (to me) phrase: "rely on part of theof the object".

Otherwise, it looks fine to me. Please apply, if you can, before 2.5c1.

[brettcannon]

rev. 53533 (for 25-maint) and rev. 53535 (trunk) have the patch with an improved comment. Py3K should eventually have its crasher file for this test deleted since classic classes will no longer be an issue.

[gvanrossum]

This got fixed for classic classes in r60057,
and backported to 2.5.2 in 60056.