[Python-Dev] RE: [Python-checkins] python/dist/src/Modules gcmodule.c,2.33.6.5,2.33.6.6 (original) (raw)

Tim Peters tim.one@comcast.net
Thu, 03 Apr 2003 23:37:47 -0500

[jhylton@users.sourceforge.net]

Modified Files: Tag: release22-maint gcmodule.c Log Message: Fix memory corruption in garbage collection. ... The problem with the previous revision is that it followed gc->gc.gcnext before calling hasfinalizer(). If hasfinalizer() gc->happened to deallocate the object FROMGC(gc->gc.gcnext), then the next time through the loop gc would point to freed memory. The fix is to always follow the next pointer after calling hasfinalizer().

Oops! I didn't see this before posting my "Boom" msg.

Note that Python 2.3 does not have this problem, because hasfinalizer() checks the tpdel slot and never runs Python code.

That part isn't so, alas: the program I posted in the "Boom" msg crashes 2.3, via the same mechanism:

return PyInstance_Check(op) ? PyObject_HasAttr(op, delstr) :
        PyType_HasFeature(op->ob_type, Py_TPFLAGS_HEAPTYPE) ?
    op->ob_type->tp_del != NULL : 0;

It's the PyInstance_Check(op) path there that's still vulnerable. I'll poke at that.

Tim, Barry, and I peed away the better part of two days tracking this down. ! next = gc->gc.gcnext; if (hasfinalizer(op)) { gclistremove(gc); gclistappend(gc, finalizers); gc->gc.gcrefs = GCMOVED; } } } --- 277,290 ---- for (; gc != unreachable; gc=next) { PyObject *op = FROMGC(gc); ! /* hasfinalizer() may result in arbitrary Python ! code being run. */ if (hasfinalizer(op)) { + next = gc->gc.gcnext; gclistremove(gc); gclistappend(gc, finalizers); gc->gc.gcrefs = GCMOVED; } + else + next = gc->gc.gcnext; } }

Are we certain that has_finalizer() can't unlink gc itself from the unreachable list? If it can, then

+ else + next = gc->gc.gcnext;

will set next to the content of free()ed memory. In fact, I believe the Boom program will suffer this fate ... yup, it does. "The problem" isn't yet really fixed in any version of Python, although I agree it's a lot better with the change above.