setting __class__ in __del__ is bad. mmkay. negative ref count! kaboom! #49533

Activity

1. jwp commented on Feb 16, 2009

   

   jwpmannequin

   on Feb 16, 2009

   MannequinAuthor

   More actions

   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
2. jwp

   added

   interpreter-core(Objects, Python, Grammar, and Parser dirs)(Objects, Python, Grammar, and Parser dirs)

   type-crashA hard crash of the interpreter, possibly with a core dumpA hard crash of the interpreter, possibly with a core dump

   on Feb 16, 2009
3. amauryfa commented on Feb 17, 2009

   

   amauryfa

   on Feb 17, 2009

   Contributor

   More actions

   Yes, the wrong type is DECREF'd when the object is deallocated.
   Patch attached.
4. benjaminp commented on Feb 18, 2009

   

   benjaminp

   on Feb 18, 2009

   Contributor

   More actions

   The patch looks correct, but are there more places where the type is
   kept in a local variable while Python code is invoked?
5. amauryfa commented on Feb 18, 2009

   

   amauryfa

   on Feb 18, 2009

   Contributor

   More actions

   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)
6. benjaminp commented on Feb 19, 2009

   

   benjaminp

   on Feb 19, 2009

   Contributor

   More actions

   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.
7. amauryfa

   self-assigned this

   on Feb 19, 2009
8. gpshead commented on Feb 25, 2009

   

   gpshead

   on Feb 25, 2009

   Member

   More actions

   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)
9. devdanzin

   added

   release-blocker

   on Apr 22, 2009
10. benjaminp commented on Apr 25, 2009

    

    benjaminp

    on Apr 25, 2009

    Contributor

    More actions

    Fixed in r71860.
11. benjaminp

    closed this as completedon Apr 25, 2009
12. ezio-melotti

    transferred this issue fromon Apr 10, 2022