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

[Tim Peters]

[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.gc_next before calling has_finalizer().  If has_finalizer()
> gc->happened to deallocate the object FROM_GC(gc->gc.gc_next), then
> the next time through the loop gc would point to freed memory.  The
> fix is to always follow the next pointer after calling
> has_finalizer().

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

> Note that Python 2.3 does not have this problem, because
> has_finalizer() checks the tp_del 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.gc_next;
>   		if (has_finalizer(op)) {
>   			gc_list_remove(gc);
>   			gc_list_append(gc, finalizers);
>   			gc->gc.gc_refs = GC_MOVED;
>   		}
>   	}
>   }
> --- 277,290 ----
>   	for (; gc != unreachable; gc=next) {
>   		PyObject *op = FROM_GC(gc);
> ! 		/* has_finalizer() may result in arbitrary Python
> ! 		   code being run. */
>   		if (has_finalizer(op)) {
> + 			next = gc->gc.gc_next;
>   			gc_list_remove(gc);
>   			gc_list_append(gc, finalizers);
>   			gc->gc.gc_refs = GC_MOVED;
>   		}
> + 		else
> + 			next = gc->gc.gc_next;
>   	}
>   }

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

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

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.