classification
Title: weakref subclass segfault
Type: crash Stage:
Components: Interpreter Core Versions: Python 3.0, Python 2.6, Python 2.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Rhamphoryncus, amaury.forgeotdarc, jnoller, python-dev, roudkerk
Priority: normal Keywords: patch

Created on 2008-06-13 03:19 by Rhamphoryncus, last changed 2016-09-07 00:22 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
weakref_subclass_cycle.py Rhamphoryncus, 2008-06-13 04:55
python-incref-from-zero.diff Rhamphoryncus, 2008-06-13 06:44
weakref_cycle.patch amaury.forgeotdarc, 2008-06-13 22:27
Messages (19)
msg68120 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-13 03:19
$ ./python 
Python 2.6a3+ (unknown, Jun 12 2008, 20:10:55) 
[GCC 4.2.3 (Debian 4.2.3-1)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import multiprocessing.reduction
[55604 refs]
>>> 
[55604 refs]
Segmentation fault (core dumped)
msg68121 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-13 03:25
op is a KeyedRef instance.  The instance being cleared from the module
is the multiprocessing.util._afterfork_registry.

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xb7d626b0 (LWP 2287)]
0x0809a131 in _Py_ForgetReference (op=0xb7a9a814) at Objects/object.c:2022
2022            if (op == &refchain ||
(gdb) bt
#0  0x0809a131 in _Py_ForgetReference (op=0xb7a9a814) at
Objects/object.c:2022
#1  0x0809a1a0 in _Py_Dealloc (op=0xb7a9a814) at Objects/object.c:2043
#2  0x080b436e in tupledealloc (op=0xb79ad1f4) at Objects/tupleobject.c:169
#3  0x0809a1ab in _Py_Dealloc (op=0xb79ad1f4) at Objects/object.c:2044
#4  0x08065bdf in PyObject_CallFunctionObjArgs (callable=0xb79baa84)
    at Objects/abstract.c:2716
#5  0x080cabc4 in handle_callback (ref=0xb7a9a814, callback=0xb79baa84)
    at Objects/weakrefobject.c:864
#6  0x080cad6e in PyObject_ClearWeakRefs (object=0xb79bd624)
    at Objects/weakrefobject.c:910
#7  0x08168971 in func_dealloc (op=0xb79bd624) at Objects/funcobject.c:453
#8  0x0809a1ab in _Py_Dealloc (op=0xb79bd624) at Objects/object.c:2044
#9  0x080b436e in tupledealloc (op=0xb79a65f4) at Objects/tupleobject.c:169
#10 0x0809a1ab in _Py_Dealloc (op=0xb79a65f4) at Objects/object.c:2044
#11 0x080b7c26 in clear_slots (type=0x82af4e4, self=0xb7a9a814)
    at Objects/typeobject.c:821
#12 0x080b806e in subtype_dealloc (self=0xb7a9a814) at
Objects/typeobject.c:950
#13 0x0809a1ab in _Py_Dealloc (op=0xb7a9a814) at Objects/object.c:2044
#14 0x080915b2 in dict_dealloc (mp=0xb79b9674) at Objects/dictobject.c:907
#15 0x0809a1ab in _Py_Dealloc (op=0xb79b9674) at Objects/object.c:2044
#16 0x080915b2 in dict_dealloc (mp=0xb79b9494) at Objects/dictobject.c:907
#17 0x0809a1ab in _Py_Dealloc (op=0xb79b9494) at Objects/object.c:2044
#18 0x08068720 in instance_dealloc (inst=0xb79b6edc)
    at Objects/classobject.c:668
#19 0x0809a1ab in _Py_Dealloc (op=0xb79b6edc) at Objects/object.c:2044
#20 0x08090517 in insertdict (mp=0xb79a5b74, key=0xb7a9ae38,
hash=-1896994012, 
    value=0x81bdd6c) at Objects/dictobject.c:455
#21 0x08090da6 in PyDict_SetItem (op=0xb79a5b74, key=0xb7a9ae38, 
    value=0x81bdd6c) at Objects/dictobject.c:697
#22 0x08095ad3 in _PyModule_Clear (m=0xb7a88334) at
Objects/moduleobject.c:125
#23 0x08111443 in PyImport_Cleanup () at Python/import.c:479
#24 0x08120cb3 in Py_Finalize () at Python/pythonrun.c:430
#25 0x0805b618 in Py_Main (argc=1, argv=0xbfbaf434) at Modules/main.c:623
#26 0x0805a2e6 in main (argc=0, argv=0x0) at ./Modules/python.c:23
msg68122 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-13 04:06
More specific test case.
msg68123 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-13 04:18
Very specific test case, eliminating multiprocessing entirely.  It may
be an interaction between having the watched obj as its own key in the
WeakValueDictionary and the order in which the two modules are cleared.
msg68124 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-13 04:55
Specific enough yet?  Seems the WeakValueDictionary and the module
clearing aren't necessary.

A subclass of weakref is created.  The target of this weakref is added
as an attribute of the weakref.  So long as a callback is present there
will be a crash on shutdown.  However, if the callback prints the
attribute, you get a crash right then.  The weakref claims to be dead,
which shouldn't be possible when the weakref's attributes have a strong
reference to the target.
msg68126 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-13 05:14
1. MyRef is released from the module as part of shutdown
2. MyRef's subtype_dealloc DECREFs its dictptr (not clearing it, as
MyRef is dead and should be unreachable)
3. the dict DECREFs the Dummy (MyRef's target)
4. Dummy's subtype_dealloc calls PyObject_ClearWeakRefs to notify of its
demise
5. the callback is called, with the dead MyRef as an argument
6. If MyRef's dict is accessed a segfault occurs.  Presumably just
calling the callback does enough damage to explain the segfault without
accessing MyRef's dict.

As I understand, a deleted weakref doesn't call its callback.  However,
subtype_dealloc doesn't call the base type's tp_dealloc until *after* it
does everything else.  Does it need a special case for weakrefs, or
maybe a tp_predealloc slot?
msg68127 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-13 05:24
Ahh, I missed a detail: when the callback is called the weakref has a
refcount of 0, which is ICNREFed to 1 when getting put in the args, then
drops down to 0 again when the args are DECREFed (causing it to get
_Py_ForgetReference to be called a second time, which segfaults.)
msg68128 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-13 06:44
Patch to add extra sanity checks to Py_INCREF (only if Py_DEBUG is set).
 If the refcount is 0 or negative if calls Py_FatalError.  This should
catch revival bugs such as this one a little more clearly.

The patch also adds a little more checking to _Py_ForgetReference, so
it's more likely to print an error rather than segfaulting.
msg68134 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-06-13 07:32
All the versions I tried (2.4, 2.5, 2.6, 3.0) crash with the given script
msg68143 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-06-13 10:08
It seems enough to simply skip deleted weakrefs in PyObject_ClearWeakRefs.
Here is a tentative patch.
msg68146 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-06-13 11:14
A new version of the patch, which tests the case of multiple weakrefs on
the same object, that get deleted together:
tp_dealloc of one weakref calls tp_dealloc of the second weakref, which
calls tp_dealloc of the referenced object. Since the weakrefs are being
deleted, no callback should fire.
msg68173 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-13 18:32
Well, my attempt at a patch didn't work, and yours does, so I guess I
have to support yours. ;)

Can you review my python-incref-from-zero patch?  It verifies the
invariant that you need, that once an object hits a refcount of 0 it
won't get raised again.  (The possibility of __del__ makes me worry, but
it *looks* okay.)

gcmodule.c has an inline copy of handle_callbacks.  Is it possible a
collection could have the same problem we're fixing here?

Minor nit: you're asserting cbcalled, but you're not using the generic
callback, so it's meaningless.
msg68175 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-13 18:38
Ahh, it seems gcmodule already considers the weakref to be reachable
when it calls the callbacks, so it shouldn't be a problem.
msg68187 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-06-13 22:27
> you're asserting cbcalled, but you're not using the generic callback, 
> so it's meaningless.
The new patch corrects this
msg68196 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-14 01:26
Another minor nit: "if(current->ob_refcnt > 0)" should have a space
after the "if".  Otherwise it's looking good.
msg68290 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-06-16 19:15
Committed r64309, with the suggested whitespace change.
Will backport.

Concerning the INCREF patch: would a simple "assert(refcnt>0)" be enough?
msg68294 - (view) Author: Adam Olsen (Rhamphoryncus) Date: 2008-06-16 20:28
Unfortunately, Py_INCREF is sometimes used in an expression (followed by
a comma).  I wouldn't expect an assert to be valid there (and I'd want
to check ISO C to make sure it's portable, not just accepted by GCC).

I'd like if Py_INCREF and friends were made into static inline
functions, which *should* have identical performance (at least on GCC),
but that's a more significant change.
msg75772 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-11-12 01:18
This was fixed 4 months ago with r64309 (trunk, 2.6) and r64310 (2.5).
msg274690 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-07 00:22
New changeset 3d8109fe6d82 by Gregory P. Smith in branch 'default':
Correct a comment in the test referencing the wrong issue number (issue3100
https://hg.python.org/cpython/rev/3d8109fe6d82
History
Date User Action Args
2016-09-07 00:22:35python-devsetnosy: + python-dev
messages: + msg274690
2008-11-12 01:18:58amaury.forgeotdarcsetstatus: open -> closed
resolution: fixed
messages: + msg75772
2008-06-16 20:28:04Rhamphoryncussetmessages: + msg68294
2008-06-16 19:15:05amaury.forgeotdarcsetmessages: + msg68290
2008-06-14 19:30:27jnollersetnosy: + roudkerk
2008-06-14 01:40:00benjamin.petersonsetnosy: amaury.forgeotdarc, Rhamphoryncus, jnoller
2008-06-14 01:26:56Rhamphoryncussetmessages: + msg68196
2008-06-13 22:27:24amaury.forgeotdarcsetfiles: + weakref_cycle.patch
messages: + msg68187
2008-06-13 22:21:31amaury.forgeotdarcsetfiles: - weakref_cycle.patch
2008-06-13 18:38:30Rhamphoryncussetmessages: + msg68175
2008-06-13 18:32:24Rhamphoryncussetmessages: + msg68173
2008-06-13 17:40:45Rhamphoryncussetnosy: + jnoller
2008-06-13 11:14:14amaury.forgeotdarcsetfiles: + weakref_cycle.patch
messages: + msg68146
2008-06-13 11:09:50amaury.forgeotdarcsetfiles: - weakref_cycle.patch
2008-06-13 10:08:31amaury.forgeotdarcsetfiles: + weakref_cycle.patch
messages: + msg68143
2008-06-13 07:32:28amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg68134
components: + Interpreter Core, - Extension Modules
versions: + Python 2.5
2008-06-13 06:44:16Rhamphoryncussetfiles: + python-incref-from-zero.diff
keywords: + patch
messages: + msg68128
2008-06-13 05:24:24Rhamphoryncussetmessages: + msg68127
2008-06-13 05:14:51Rhamphoryncussetmessages: + msg68126
2008-06-13 04:55:39Rhamphoryncussetfiles: - inner.py
2008-06-13 04:55:36Rhamphoryncussetfiles: - outer.py
2008-06-13 04:55:24Rhamphoryncussetfiles: + weakref_subclass_cycle.py
messages: + msg68124
title: segfault with WeakValueDictionary and module clearing -> weakref subclass segfault
2008-06-13 04:19:07Rhamphoryncussettitle: segfault from multiprocessing.util.register_after_fork -> segfault with WeakValueDictionary and module clearing
2008-06-13 04:18:37Rhamphoryncussetfiles: - register_after_fork-crash.py
2008-06-13 04:18:32Rhamphoryncussetfiles: + inner.py
2008-06-13 04:18:20Rhamphoryncussetfiles: + outer.py
messages: + msg68123
2008-06-13 04:06:36Rhamphoryncussetfiles: + register_after_fork-crash.py
messages: + msg68122
title: segfault after loading multiprocessing.reduction -> segfault from multiprocessing.util.register_after_fork
2008-06-13 03:25:45Rhamphoryncussetmessages: + msg68121
2008-06-13 03:19:58Rhamphoryncuscreate