| 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) *  |
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) * |
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) * |
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) * |
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) * |
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) * |
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)  |
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 ( https://hg.python.org/cpython/rev/3d8109fe6d82 |
|
|