classification
Title: setting __class__ in __del__ is bad. mmkay. negative ref count! kaboom!
Type: crash Stage: commit review
Components: Interpreter Core Versions: Python 3.0, Python 2.6, Python 2.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: amaury.forgeotdarc Nosy List: amaury.forgeotdarc, benjamin.peterson, gregory.p.smith, jwp
Priority: release blocker Keywords: patch

Created on 2009-02-16 21:23 by jwp, last changed 2009-04-25 00:45 by benjamin.peterson. This issue is now closed.

Files
File name Uploaded Description Edit
kaboom.py jwp, 2009-02-16 21:23
del_changes_class.patch amaury.forgeotdarc, 2009-02-17 19:04
Messages (7)
msg82272 - (view) Author: James William Pye (jwp) Date: 2009-02-16 21:23
I found this bug by misplacing a line of a code. Yes, I was doing
naughty things, but in the case of the class that led to the discovery,
it was inadvertent. :P


class something_else(object):
 pass

class foo(object):
 def __del__(self):
  self.__class__ = something_else

for _ in range(1000):
 foo()

That results in a fatal python error due to a negative reference
count(on the foo class object) in 3.0.

3.0.1 (release30-maint:69593M, Feb 13 2009, 14:48:10) -> kaboom
jwp@torch[]:org/pgfoundry/python 134% /src/build/py30/bin/python3.0
./kaboom.py
Fatal Python error: Objects/descrobject.c:10 object at 0x5221b8 has
negative ref count -1
zsh: abort      /src/build/py30/bin/python3.0 ./kaboom.py


2.6 (r26:66714, Dec 21 2008, 21:17:32) -> kaboom
jwp@torch[]:org/pgfoundry/python 0% /sw/bin/python2.6 ./kaboom.py      
        
Fatal Python error: GC object already tracked
zsh: abort      /sw/bin/python2.6 ./kaboom.py


2.5.2 (r252:60911, Jun 15 2008, 18:55:39) -> no kaboom (no asserts? eh..)
...

2.5.1 (r251:54863, Apr 15 2008, 22:57:26) -> kaboom (/usr/bin/python2.5)
jwp@torch[]:org/pgfoundry/python 0% /usr/bin/python2.5 ./kaboom.py
Assertion failed: (PyType_Check(base)), function _PyType_Lookup, file
Objects/typeobject.c, line 2035.
zsh: abort      /usr/bin/python2.5 ./kaboom.py
msg82354 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-02-17 19:04
Yes, the wrong type is DECREF'd when the object is deallocated.
Patch attached.
msg82427 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-02-18 19:55
The patch looks correct, but are there more places where the type is
kept in a local variable while Python code is invoked?
msg82439 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2009-02-18 22:51
I carefully looked at all places that store ->ob_type or Py_TYPE() in a 
local variable, and I could not find any exploit. Most places don't 
reuse the type once the method or the slot has been called.

Two places were harder to analyze: subtype_clear (but an attack would 
use __del__, and use a reference cycle: subtype_clear is never called in 
this case) and PyObject_Generic(Get|Set)Attr (the only escape path to 
python code could be through PyType_Ready; but it has already been 
called for heap types)
msg82446 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-02-19 01:37
On Wed, Feb 18, 2009 at 4:51 PM, Amaury Forgeot d'Arc
<report@bugs.python.org> wrote:
>
> Amaury Forgeot d'Arc <amauryfa@gmail.com> added the comment:
>
> I carefully looked at all places that store ->ob_type or Py_TYPE() in a
> local variable, and I could not find any exploit. Most places don't
> reuse the type once the method or the slot has been called.

Thanks for looking!

>
> Two places were harder to analyze: subtype_clear (but an attack would
> use __del__, and use a reference cycle: subtype_clear is never called in
> this case) and PyObject_Generic(Get|Set)Attr (the only escape path to
> python code could be through PyType_Ready; but it has already been
> called for heap types)

Well, I think we can deal with those if they are reported. Go ahead
and apply the patch.
msg82719 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2009-02-25 21:15
Could the PyObject_ClearWeakRefs(self); call in the middle of the lines 
del_changes_class.patch adds also be used to cause python code to set 
__del__ or __dict__ causing the wrong destructor or wrong dict to be 
DECREFed?

(I'm trying to wrap my head around the possibilities here and come up 
with a test case of that but weakref isn't behaving as i'd expect)
msg86442 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-04-25 00:45
Fixed in r71860.
History
Date User Action Args
2009-04-25 00:45:55benjamin.petersonsetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg86442
2009-04-22 14:39:59ajaksu2setpriority: release blocker
2009-02-25 21:15:06gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg82719
2009-02-19 07:28:56amaury.forgeotdarcsetkeywords: - needs review
assignee: amaury.forgeotdarc
resolution: accepted
stage: commit review
2009-02-19 01:37:50benjamin.petersonsetmessages: + msg82446
2009-02-18 22:51:06amaury.forgeotdarcsetmessages: + msg82439
2009-02-18 19:55:11benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg82427
2009-02-17 19:04:42amaury.forgeotdarcsetkeywords: + needs review, patch
files: + del_changes_class.patch
messages: + msg82354
nosy: + amaury.forgeotdarc
2009-02-16 21:23:34jwpcreate