The attached patch causes a segfault while running test_urllib:

$ ./python -m test test_urllib
[1/1] test_urllib
<test.test_urllib.FakeHTTPConnection object at 0xb66b0fbc>
<test.test_urllib.FakeHTTPConnection object at 0xb66b76fc>
<test.test_urllib.FakeHTTPConnection object at 0xb66b7a44>
<test.test_urllib.FakeHTTPConnection object at 0xb66b7f84>
<test.test_urllib.FakeHTTPConnection object at 0xb66c2034>
<test.test_urllib.FakeHTTPConnection object at 0xb66c2034>
Fatal Python error: Segmentation fault

Current thread 0xb77de8d0:
  File "/home/wolf/dev/py/py3k/Lib/http/client.py", line 451 in close
  File "/home/wolf/dev/py/py3k/Lib/urllib/request.py", line 1119 in do_open
  File "/home/wolf/dev/py/py3k/Lib/urllib/request.py", line 1166 in http_open
  File "/home/wolf/dev/py/py3k/Lib/urllib/request.py", line 347 in _call_chain
  File "/home/wolf/dev/py/py3k/Lib/urllib/request.py", line 387 in _open
  File "/home/wolf/dev/py/py3k/Lib/urllib/request.py", line 369 in open
  File "/home/wolf/dev/py/py3k/Lib/urllib/request.py", line 138 in urlopen
  File "/home/wolf/dev/py/py3k/Lib/test/test_urllib.py", line 184 in test_url_fragment
[...]
Segmentation fault

The failure seems to be fairly random, for example if I uncomment the tests at the bottom of test_urllib, the segfault happens only in non-verbose mode.  Also adding a few print()s to debug the issue or making other unrelated changes changes the behavior (e.g. it fails only in verbose mode but not in normal mode).

gdb shows that the failure is in Objects/object.c, at line 981 in the _PyObject_GenericGetAttrWithDict function.  The problem seems to be that 'descr' is invalid and descr->ob_type results in an error:

$ gdb -args ./python -m test test_urllib
GNU gdb (Ubuntu/Linaro 7.2-1ubuntu11) 7.2
[...]
Reading symbols from /home/wolf/dev/py/py3k/python...done.
(gdb) run
Starting program: /home/wolf/dev/py/py3k/python -m test test_urllib
[Thread debugging using libthread_db enabled]
[1/1] test_urllib
[New Thread 0xb77e9b70 (LWP 5227)]
<test.test_urllib.FakeHTTPConnection object at 0xb6ec6dfc>

Program received signal SIGSEGV, Segmentation fault.
0x0805c7d8 in _PyObject_GenericGetAttrWithDict (obj=<FakeSocket(io_refs=1) at remote 0xb6ec31e4>, name='close', dict=0x0) at Objects/object.c:982
982             f = descr->ob_type->tp_descr_get;
(gdb) p *(PyUnicodeObject*)descr
$1 = {ob_base = {_ob_next = 0xdbdbdbdb, _ob_prev = 0xdbdbdbdb, ob_refcnt = -606348324, ob_type = 0xdbdbdbdb}, length = -606348325, str = 0xdbdbdbdb, 
  hash = -606348325, state = -606348325, defenc = <unknown at remote 0xdbdbdbdb>}
(gdb) 

FakeHTTPConnection is a subclass of http.client.HTTPConnection, but I also got segfaults in FakeSocket, which is an io.BytesIO subclass.
FWIW I found this while trying to fix ResourceWarnings.  In #12133 I proposed a patch that fixes a ResourceWarnings (included in segfault.diff), and while trying the same approach in urlretrieve (adding a finally: http_conn.close()) and running test_urllib I got the segfault.
Tested on 3.3 only.

This occurs when running the GC and then calling the finalization of an IO object, which temporarily resurrects the object to call its close() method:

#0  0x00007fc20bc1609b in raise () from /lib64/libpthread.so.0
#1  0x00000000005328e7 in faulthandler_fatal_error (signum=11) at ./Modules/faulthandler.c:299
#2  <signal handler called>
#3  0x00000000004190c7 in _PyObject_GenericGetAttrWithDict (obj=<FakeSocket(io_refs=1) at remote 0x7fc2069d8190>, 
    name='close', dict=0x0) at Objects/object.c:982
#4  0x0000000000419581 in PyObject_GenericGetAttr (obj=<FakeSocket(io_refs=1) at remote 0x7fc2069d8190>, name=
    'close') at Objects/object.c:1046
#5  0x0000000000418a38 in PyObject_GetAttr (v=<FakeSocket(io_refs=1) at remote 0x7fc2069d8190>, name='close')
    at Objects/object.c:831
#6  0x00000000004818b4 in PyEval_EvalFrameEx (f=
    Frame 0x7fc2065ee9e8, for file /home/antoine/cpython/default/Lib/http/client.py, line 451, in close (self=<HTTPResponse(fp=<FakeSocket(io_refs=1) at remote 0x7fc2069d8190>, status='UNKNOWN', will_close='UNKNOWN', chunk_left='UNKNOWN', length='UNKNOWN', headers=None, reason='UNKNOWN', version='UNKNOWN', debuglevel=0, msg=None, chunked='UNKNOWN', _method='GET') at remote 0x7fc20688a220>), throwflag=0) at Python/ceval.c:2268
#7  0x0000000000487a2f in PyEval_EvalCodeEx (_co=<code at remote 0x7fc206992d30>, globals=
    {'BadStatusLine': <type at remote 0x1b21a80>, '__cached__': '/home/antoine/cpython/default/Lib/http/__pycache__/client.cpython-32.pyc', '_UNKNOWN': 'UNKNOWN', 'SWITCHING_PROTOCOLS': 101, 'urlsplit': <function at remote 0x7fc2069d66d8>, 'InvalidURL': <type at remote 0x1b1d490>, 'collections': <module at remote 0x7fc20bef58d0>, 'SERVICE_UNAVAILABLE': 503, 'CREATED': 201, '__file__': '/home/antoine/cpython/default/Lib/http/client.py', 'PROCESSING': 102, 'PRECONDITION_FAILED': 412, 'ssl': <module at remote 0x7fc2068588d0>, 'NOT_ACCEPTABLE': 406, 'HTTPConnection': <type at remote 0x1b01f90>, 'NotConnected': <type at remote 0x1b1c5d0>, 'MULTI_STATUS': 207, 'OK': 200, 'UnknownProtocol': <type at remote 0x1b1d830>, '_CS_REQ_STARTED': 'Request-started', 'email': <module at remote 0x7fc20698c858>, 'NOT_IMPLEMENTED': 501, 'CannotSendRequest': <type at remote 0x1aff730>, 'BAD_GATEWAY': 502, 'CannotSendHeader': <type at remote 0x1affad0>, 'LENGTH_REQUIRED': 411, 'parse_headers': <function at remote 0x7fc20692dec0>, 'HTTP_...(truncated), locals=0x0, args=0x7fc20688a868, argcount=1, kws=0x0, kwcount=0, defs=0x0, defcount=0, kwdefs=0x0, closure=0x0)
    at Python/ceval.c:3301
#8  0x0000000000576a36 in function_call (func=<function at remote 0x7fc2068c9900>, arg=
    (<HTTPResponse(fp=<FakeSocket(io_refs=1) at remote 0x7fc2069d8190>, status='UNKNOWN', will_close='UNKNOWN', chunk_left='UNKNOWN', length='UNKNOWN', headers=None, reason='UNKNOWN', version='UNKNOWN', debuglevel=0, msg=None, chunked='UNKNOWN', _method='GET') at remote 0x7fc20688a220>,), kw=0x0) at Objects/funcobject.c:629
#9  0x000000000053f975 in PyObject_Call (func=<function at remote 0x7fc2068c9900>, arg=
    (<HTTPResponse(fp=<FakeSocket(io_refs=1) at remote 0x7fc2069d8190>, status='UNKNOWN', will_close='UNKNOWN', chunk_left='UNKNOWN', length='UNKNOWN', headers=None, reason='UNKNOWN', version='UNKNOWN', debuglevel=0, msg=None, chunked='UNKNOWN', _method='GET') at remote 0x7fc20688a220>,), kw=0x0) at Objects/abstract.c:2149
#10 0x000000000055c1a1 in method_call (func=<function at remote 0x7fc2068c9900>, arg=
    (<HTTPResponse(fp=<FakeSocket(io_refs=1) at remote 0x7fc2069d8190>, status='UNKNOWN', will_close='UNKNOWN', chunk_left='UNKNOWN', length='UNKNOWN', headers=None, reason='UNKNOWN', version='UNKNOWN', debuglevel=0, msg=None, chunked='UNKNOWN', _method='GET') at remote 0x7fc20688a220>,), kw=0x0) at Objects/classobject.c:319
#11 0x000000000053f975 in PyObject_Call (func=<method at remote 0x7fc20686aba0>, arg=(), kw=0x0)
    at Objects/abstract.c:2149
#12 0x000000000054054c in PyObject_CallMethodObjArgs (callable=<method at remote 0x7fc20686aba0>, name='close')
    at Objects/abstract.c:2350
#13 0x0000000000514f2f in _PyIOBase_finalize (self=
    <HTTPResponse(fp=<FakeSocket(io_refs=1) at remote 0x7fc2069d8190>, status='UNKNOWN', will_close='UNKNOWN', chunk_left='UNKNOWN', length='UNKNOWN', headers=None, reason='UNKNOWN', version='UNKNOWN', debuglevel=0, msg=None, chunked='UNKNOWN', _method='GET') at remote 0x7fc20688a220>) at ./Modules/_io/iobase.c:222
#14 0x0000000000515143 in iobase_dealloc (self=0x7fc20688a220) at ./Modules/_io/iobase.c:285
#15 0x000000000042fc54 in subtype_dealloc (self=
    <HTTPResponse(fp=<FakeSocket(io_refs=1) at remote 0x7fc2069d8190>, status='UNKNOWN', will_close='UNKNOWN', chunk_left='UNKNOWN', length='UNKNOWN', headers=None, reason='UNKNOWN', version='UNKNOWN', debuglevel=0, msg=None, chunked='UNKNOWN', _method='GET') at remote 0x7fc20688a220>) at Objects/typeobject.c:968
#16 0x000000000041acb1 in _Py_Dealloc (op=
    <HTTPResponse(fp=<FakeSocket(io_refs=1) at remote 0x7fc2069d8190>, status='UNKNOWN', will_close='UNKNOWN', chunk_left='UNKNOWN', length='UNKNOWN', headers=None, reason='UNKNOWN', version='UNKNOWN', debuglevel=0, msg=None, chunked='UNKNOWN', _method='GET') at remote 0x7fc20688a220>) at Objects/object.c:1693
#17 0x0000000000572b04 in frame_dealloc (f=
    Frame 0x1b6b890, for file /home/antoine/cpython/default/Lib/http/client.py, line 1046, in getresponse ())
#18 0x000000000041acb1 in _Py_Dealloc (op=
    Frame 0x1b6b890, for file /home/antoine/cpython/default/Lib/http/client.py, line 1046, in getresponse ())
    at Objects/object.c:1693
#19 0x00000000004cb7a6 in tb_dealloc (tb=0x7fc2068693a8) at Python/traceback.c:50
#20 0x000000000041acb1 in _Py_Dealloc (op=<traceback at remote 0x7fc2068693a8>) at Objects/object.c:1693
#21 0x00000000004cb72d in tb_dealloc (tb=0x7fc2068691c8) at Python/traceback.c:49
#22 0x000000000041acb1 in _Py_Dealloc (op=<traceback at remote 0x7fc2068691c8>) at Objects/object.c:1693
#23 0x00000000005650af in BaseException_clear (self=0x7fc2069ee960) at Objects/exceptions.c:59
#24 0x000000000056524f in BaseException_dealloc (self=0x7fc2069ee960) at Objects/exceptions.c:69
#25 0x000000000042fc54 in subtype_dealloc (self=<BadStatusLine at remote 0x7fc2069ee960>)
    at Objects/typeobject.c:968
#26 0x000000000041acb1 in _Py_Dealloc (op=<BadStatusLine at remote 0x7fc2069ee960>) at Objects/object.c:1693
#27 0x00000000005651c1 in BaseException_clear (self=0x7fc2069f5990) at Objects/exceptions.c:61
#28 0x00000000005666fa in EnvironmentError_clear (self=0x7fc2069f5990) at Objects/exceptions.c:617
#29 0x0000000000566783 in EnvironmentError_dealloc (self=0x7fc2069f5990) at Objects/exceptions.c:624
#30 0x000000000042fc54 in subtype_dealloc (self=<URLError at remote 0x7fc2069f5990>) at Objects/typeobject.c:968
#31 0x000000000041acb1 in _Py_Dealloc (op=<URLError at remote 0x7fc2069f5990>) at Objects/object.c:1693
#32 0x000000000058e04e in PyDict_Clear (op={}) at Objects/dictobject.c:926
#33 0x0000000000591042 in dict_tp_clear (op={}) at Objects/dictobject.c:1928
#34 0x00000000004da4da in delete_garbage (collectable=0x7fff7ccb0060, old=0x83f680) at Modules/gcmodule.c:739
#35 0x00000000004daa0c in collect (generation=2) at Modules/gcmodule.c:897
#36 0x00000000004dad54 in gc_collect (self=<module at remote 0x7fc20891d510>, args=(), kws=0x0)
    at Modules/gcmodule.c:1034

Here is a quick script to reproduce. It seems that the type of the object has to be caught in a reference cycle for this to happen: apparently the descriptors start being deallocated but the method cache isn't updated yet (?).

Ezio, in the meantime, you can simply put the FakeSocket declaration at the top level.

Looking through Antoine's example code. When garbage is collected, the subtype and its tp_dict are cleared before the instance object itself. When the dict is cleared as part of the garbage collection, the methods get deallocated but the method cache is not updated. That way the lookup for the "close" method results in a cache hit for an invalid pointer.

I'm not at all knowledgeable to understand whether it is right for the type dictionary to be cleared before instances of that type (then either the finalizer for IOBase should work around this case, or the cache should be updated beforehand), or there is something to be done to ensure a correct clearing order.

Also I can't think of any other example of a C type, inheritable from Python code, that calls another method in the destructor: is this specific to IO? Please note that the example code fails even when inheriting from the C type directly (_io._IOBase).

Invalidating the cache in subtype_clear prevents the "close" method to be called in the collected object. I'm not sure that's the right place where to put the PyType_Modified, but apparently it works.

The attached test segfaults (and passes with the patch). It wasn't clear to me where to put the test (it is a typeobject issue triggered by io) but Antoine on IRC agreed it would make sense to add it to test_io anyway.

Python 2.7 is affected too by the bug, but it doesn't trigger with _PyIOBase_finalize because it first checks for "closed" but the lookup fails (not sure why) so it doesn't try to call "close". On Python 3.3 the lookup for "closed" returned a valid descriptor from the method cache even after the type is cleared.

Thank you very much!

New changeset cd40ea4087b0 by Antoine Pitrou in branch '3.2':
Issue #12149: Update the method cache after a type's dictionnary gets
http://hg.python.org/cpython/rev/cd40ea4087b0

New changeset 5992cbbedf59 by Antoine Pitrou in branch 'default':
Issue #12149: Update the method cache after a type's dictionnary gets
http://hg.python.org/cpython/rev/5992cbbedf59

New changeset 9618303852da by Antoine Pitrou in branch '2.7':
Issue #12149: Update the method cache after a type's dictionnary gets
http://hg.python.org/cpython/rev/9618303852da

Should be fixed now. Thanks again Davide for the patch.

Oooh, an interesting and complex bug with an one-liner fix!

Please reopen this bug as the fix is wrong.

This fix merely hides the symptoms of _PyType_Lookup returning a dead object, by calling PyType_Modified() frequently, thus ensuring the type method cache is almost always invalidated.
This results in a significant slow down in pystones (~4%) due to a large slowdown (~60%) in _PyType_Lookup.

I don't know what the root cause is, but it could be:
A failure to call PyType_Modified() at the correct point (probably somewhere in iobase.c)
or it could be:
Deallocation not conforming to topographical ordering (ie. instance first then class), due to a cycle involving a borrowed reference.
(Or it could be something else)

Mark, it's been a long time since I went through this bug and don't remember the details. Are you sure subtype_dealloc should not call PyType_Modified? It looked like the appropriate place at the time. In the example the reference cycle was introduced on purpose by the Python program. Maybe we can try and explore the reference graph again?

I understood while posting the patch that this would have impacted performance. But deallocation of a subtype is not something that happens a lot in most programs. An overall 4% looks huge though.

Absolutely. subtype_dealloc deals with deallocation of subtype 
*instances*, not the types themselves.

 > Maybe we can try and explore the reference graph again?
This sort of thing is one of the reasons that the cycle GC does not call 
any finalisers. Attempting to do finalisation during deallocation is 
asking for trouble, and it seems to be pervasive in CPython.
I'm surprised this sort of bug is not more common.

But subtype_dealloc deals with instances not classes, and deallocation 
of instances may happen many millions of times.

Thanks for the writeup, Mark. Here is a patch, calling PyType_Modified in the type's (not the instance's) tp_clear.

What's happening is that the cycle GC calls type_clear to clear the type, but the method-cache is not invalidated.
I have added a call to PyType_Modified in type_clear (as well as type_set_name and type_set_qualname, which also modify the type).

Patch is attached.

Beat me to it, Antoine!

Don't forget type_set_name and type_set_qualname.

> I have added a call to PyType_Modified in type_clear (as well as
> type_set_name and type_set_qualname, which also modify the type).

Can __name__ and __qualname__ be looked up through the method cache?

As much as I hate being wrong, I must concur with you. ;) Thank you, Mark.

Antoine Pitrou wrote:
> Antoine Pitrou <pitrou@free.fr> added the comment:
> 
>> I have added a call to PyType_Modified in type_clear (as well as
>> type_set_name and type_set_qualname, which also modify the type).
> 
> Can __name__ and __qualname__ be looked up through the method cache?

__name__ and __qualname__ are not looked up through the method cache,
but their descriptors could be.
Changing the descriptors would be caught elsewhere,
so there is no need to to add any PyType_Modified to
type_set_name and type_set_qualname.

However, calls to PyType_Modified will never cause any errors,
and if only used for genuine type modifications, will cause no 
discernible performance degradation.

Since PyType_Modified is generally called whenever a type is modified, 
it is likely to act as a guardian for any future optimisations that 
require classes to be unchanged.

Thus, given these two reasons, it seems wise to call PyType_Modified 
anywhere the type is modified, however minor that modification appears 
to be.

> Since PyType_Modified is generally called whenever a type is modified, 
> it is likely to act as a guardian for any future optimisations that 
> require classes to be unchanged.
> 
> Thus, given these two reasons, it seems wise to call PyType_Modified 
> anywhere the type is modified, however minor that modification appears 
> to be.

Well, unless we start reviewing all the places where a type might be
directly modified, I'm not sure there's much point in adding
PyType_Modified to those two.

New changeset b196bcd7c34f by Antoine Pitrou in branch '3.2':
Fix the fix for issue #12149: it was incorrect, although it had the side
http://hg.python.org/cpython/rev/b196bcd7c34f

New changeset 195bfd4c3ea1 by Antoine Pitrou in branch 'default':
Fix the fix for issue #12149: it was incorrect, although it had the side
http://hg.python.org/cpython/rev/195bfd4c3ea1

New changeset da1d7f8cd487 by Antoine Pitrou in branch '2.7':
Fix the fix for issue #12149: it was incorrect, although it had the side
http://hg.python.org/cpython/rev/da1d7f8cd487

Hopefully this is fixed for good now. Thank you!