Ensure that HeldClass bound methods don't hit released memory.

HeldClass instances have a different lifetime than regular classes - things like
bound methods on them store a reference to them via pointer. This allows us to do things like

	l = ListOf(SomeHeldClass)([...])

	l[10].f()

and allow 'f' to modify the contents of the list in interepreted as well as compiled code,
even though the list is holding H as packed memory.

This is, of course, very dangerous! Such a reference will be invalidated by resizing the list,
for instance (really, anything that moves the memory), following the standard semantics of
C++.

However, we also had a bug where you could just write

	SomeHeldClass().f()

and crash, because the intermediate 'f' was not holding SomeHeldClass() alive.

This change fixes this by doing two things:

	(1) we ensure that bound methods keep the object being bound alive for the
	    duration of the instruction in the parent frame. This follows the same
            semantic model we have for temporary 'refTo' instances
        (2) we cause 'f' to keep the temporary alive, and then throw an exception if
            we are the last refcount to the object alive.

Author: braxtonmckee

typed_python/FunctionType.cpp

@@ -172,7 +172,8 @@ PyObject* Function::Overload::buildFunctionObj(Type* closureType, instance_ptr c
                         newCellContents.steal(
                             PyInstance::extractPythonObject(
                                 ((TypedCellType*)bindingValue.type())->get(bindingValue.data()),
-                                ((TypedCellType*)bindingValue.type())->getHeldType()
+                                ((TypedCellType*)bindingValue.type())->getHeldType(),
+                                false
                             )
                         );
                     } else {

typed_python/Instance.cpp

@@ -84,6 +84,12 @@ Instance::Instance(instance_ptr p, Type* t) : mLayout(nullptr) {
 Instance::~Instance() {
     if (mLayout && mLayout->refcount.fetch_sub(1) == 1) {
         mLayout->type->destroy(mLayout->data);
+
+        if (mLayout->type->bytecount() >= 8) {
+            // write a terminator so that we can see that this memory was destroyed.
+            ((uint64_t*)mLayout->data)[0] = 0xdeadbeef;
+        }
+
         tp_free(mLayout);
     }
 }

typed_python/PyAlternativeInstance.cpp

@@ -166,7 +166,7 @@ PyObject* PyAlternativeInstance::tp_getattr_concrete(PyObject* pyAttrName, const
         return extractPythonObject(
             heldT->eltPtr(heldData, ix),
             heldT->getTypes()[ix]
-            );
+        );
     }
 
     return PyInstance::tp_getattr_concrete(pyAttrName, attrName);

typed_python/PyBoundMethodInstance.cpp

@@ -22,6 +22,17 @@ BoundMethod* PyBoundMethodInstance::type() {
 }
 
 PyObject* PyBoundMethodInstance::tp_call_concrete(PyObject* args, PyObject* kwargs) {
+    if (mKeepalive) {
+        if (mKeepalive->ob_refcnt == 1) {
+            PyErr_Format(
+                PyExc_RuntimeError,
+                "BoundMethod pointing at HeldClass 'self' would have crashed in compiled code."
+            );
+
+            return NULL;
+        }
+    }
+
     Function* f = type()->getFunction();
     Type* firstArgType = type()->getFirstArgType();
 

typed_python/PyClassInstance.cpp

@@ -564,9 +564,19 @@ PyObject* PyClassInstance::tpGetattrGeneric(
                 throw std::runtime_error("BoundMethod of HeldClass should take a RefTo");
             }
 
-            return PyInstance::initializePythonRepresentation(method, [&](instance_ptr methodData) {
+            PyObject* res = PyInstance::initializePythonRepresentation(method, [&](instance_ptr methodData) {
                 method->copy_constructor(methodData, (instance_ptr)&heldData);
             });
+
+            // the function reference holds 'self' as a RefTo. If 'self' is a temporary that
+            // doesn't exist in a variable (say, it was returned as a new object from a
+            // function) then this refto could end up pointing at the wrong data.
+            ((PyInstance*)res)->setKeepalive(self);
+
+            // ensure that self is kept alive for the duration of the instruction.
+            PyTemporaryReferenceTracer::keepaliveForCurrentInstruction(self);
+
+            return res;
         }
     }
 

typed_python/PyFunctionInstance.cpp

@@ -569,7 +569,7 @@ std::pair<bool, PyObject*> PyFunctionInstance::dispatchFunctionCallToCompiledSpe
         }
     });
 
-    return std::pair<bool, PyObject*>(true, (PyObject*)extractPythonObject(result.data(), result.type()));
+    return std::pair<bool, PyObject*>(true, (PyObject*)extractPythonObject(result.data(), result.type(), false));
 }
 
 

typed_python/PyInstance.cpp

@@ -120,7 +120,7 @@ PyMethodDef* PyInstance::typeMethodsConcrete(Type* t) {
 void PyInstance::tp_dealloc(PyObject* self) {
     PyInstance* wrapper = (PyInstance*)self;
 
-    wrapper->mContainingInstance.~Instance();
+    wrapper->teardown();
 
     Py_TYPE(self)->tp_free((PyObject*)self);
 }
@@ -277,14 +277,9 @@ PyObject* PyInstance::extractPythonObject(instance_ptr data, Type* eltType, bool
             // this allows us to mimic the behavior of the compiler when we're
             // using these objects: by default, we never get naked reftos anywhere
             // we can find them.
-            PyObject* res = PyInstance::initializeTemporaryRef(eltType, data);
-
-            PyThreadState *tstate = PyThreadState_GET();
-            PyFrameObject *f = tstate->frame;
 
-            if (f) {
-                PyTemporaryReferenceTracer::traceObject(res, f);
-            }
+            PyObject* res = PyInstance::initializeTemporaryRef(eltType, data);
+            PyTemporaryReferenceTracer::traceObject(res);
 
             return res;
         }
@@ -315,8 +310,8 @@ PyObject* PyInstance::extractPythonObject(instance_ptr data, Type* eltType, bool
     });
 }
 
-PyObject* PyInstance::extractPythonObject(const Instance& instance) {
-    return extractPythonObject(instance.data(), instance.type());
+PyObject* PyInstance::extractPythonObject(const Instance& instance, bool createTemporaryRef) {
+    return extractPythonObject(instance.data(), instance.type(), createTemporaryRef);
 }
 
 PyObject* PyInstance::extractPythonObjectConcrete(Type* eltType, instance_ptr data) {

typed_python/PyInstance.hpp

@@ -78,16 +78,41 @@ class PyInstance {
     // we may be a temporary ref to a type
     instance_ptr mTemporaryRefTo;
 
+    PyObject* mKeepalive;
+
     // initialize our fields after we have called 'tp_alloc'
     void initializeEmpty() {
         mIteratorFlag = -1;
         mIteratorOffset = -1;
         mContainerSize = -1;
         mTemporaryRefTo = nullptr;
+        mKeepalive = nullptr;
 
         new (&mContainingInstance) Instance();
     }
 
+    // called when this python object is destroyed. Gives us a chance to do cleanup actions
+    void teardown() {
+        mContainingInstance.~Instance();
+
+        if (mKeepalive) {
+            Py_DECREF(mKeepalive);
+            mKeepalive = nullptr;
+        }
+    }
+
+    void setKeepalive(PyObject* toKeepAlive) {
+        if (toKeepAlive) {
+            Py_INCREF(toKeepAlive);
+        }
+
+        if (mKeepalive) {
+            Py_DECREF(mKeepalive);
+        }
+
+        mKeepalive = toKeepAlive;
+    }
+
     void resolveTemporaryReference();
 
     template<class T>
@@ -347,8 +372,8 @@ class PyInstance {
         mContainingInstance = Instance(type, i);
     }
 
-    static PyObject* fromInstance(const Instance& instance) {
-        return extractPythonObject(instance.data(), instance.type());
+    static PyObject* fromInstance(const Instance& instance, bool createTemporaryRef=true) {
+        return extractPythonObject(instance.data(), instance.type(), createTemporaryRef);
     }
 
     PyInstance* duplicate() {
@@ -403,7 +428,7 @@ class PyInstance {
     //instances.
     static PyObject* extractPythonObject(instance_ptr data, Type* eltType, bool createTemporaryRef=true);
 
-    static PyObject* extractPythonObject(const Instance& instance);
+    static PyObject* extractPythonObject(const Instance& instance, bool createTemporaryRef=true);
 
     //if we have a python representation that we want to use for this object, override and return not-NULL.
     //otherwise, this version takes over and returns a PyInstance wrapper for the object

typed_python/PyTemporaryReferenceTracer.cpp

@@ -17,7 +17,7 @@
 #include "PyTemporaryReferenceTracer.hpp"
 
 
-int PyTemporaryReferenceTracer::globalTraceFun(PyObject* obj, PyFrameObject* frame, int what, PyObject* arg) {
+int PyTemporaryReferenceTracer::globalTraceFun(PyObject* dummyObj, PyFrameObject* frame, int what, PyObject* arg) {
     if (frame != globalTracer.mostRecentEmptyFrame) {
         auto it = globalTracer.frameToHandles.find(frame);
 
@@ -26,9 +26,12 @@ int PyTemporaryReferenceTracer::globalTraceFun(PyObject* obj, PyFrameObject* fra
         } else {
             globalTracer.mostRecentEmptyFrame = nullptr;
 
-            for (auto obj: it->second) {
-                ((PyInstance*)obj)->resolveTemporaryReference();
-                decref(obj);
+            for (auto objAndAction: it->second) {
+                if (objAndAction.second == TraceAction::ConvertTemporaryReference) {
+                    ((PyInstance*)objAndAction.first)->resolveTemporaryReference();
+                }
+
+                decref(objAndAction.first);
             }
 
             globalTracer.frameToHandles.erase(it);
@@ -54,25 +57,58 @@ int PyTemporaryReferenceTracer::globalTraceFun(PyObject* obj, PyFrameObject* fra
     return res;
 }
 
+void PyTemporaryReferenceTracer::installGlobalTraceHandlerIfNecessary() {
+    // ensure that the global trace handler is installed
+    PyThreadState *tstate = PyThreadState_GET();
+
+    // this swallows the reference we're holding on 'tracer' into the function itself
+    if (tstate->c_tracefunc != globalTraceFun) {
+        globalTracer.priorTraceFunc = tstate->c_tracefunc;
+        globalTracer.priorTraceFuncArg = incref(tstate->c_traceobj);
+
+        PyEval_SetTrace(PyTemporaryReferenceTracer::globalTraceFun, nullptr);
+    }
+}
 
 void PyTemporaryReferenceTracer::traceObject(PyObject* o, PyFrameObject* f) {
     // mark that we're going to trace
-    globalTracer.frameToHandles[f].push_back(incref(o));
+    globalTracer.frameToHandles[f].push_back(std::make_pair(incref(o), TraceAction::ConvertTemporaryReference));
 
-    // reset the
     if (globalTracer.mostRecentEmptyFrame == f) {
         globalTracer.mostRecentEmptyFrame = nullptr;
     }
 
+    installGlobalTraceHandlerIfNecessary();
+}
+
+void PyTemporaryReferenceTracer::keepaliveForCurrentInstruction(PyObject* o, PyFrameObject* f) {
+    // mark that we're going to trace
+    globalTracer.frameToHandles[f].push_back(std::make_pair(incref(o), TraceAction::Decref));
+
+    if (globalTracer.mostRecentEmptyFrame == f) {
+        globalTracer.mostRecentEmptyFrame = nullptr;
+    }
+
+    installGlobalTraceHandlerIfNecessary();
+}
+
+void PyTemporaryReferenceTracer::traceObject(PyObject* o) {
     PyThreadState *tstate = PyThreadState_GET();
+    PyFrameObject *f = tstate->frame;
 
-    // this swallows the reference we're holding on 'tracer' into the function itself
-    if (tstate->c_tracefunc != globalTraceFun) {
-        globalTracer.priorTraceFunc = tstate->c_tracefunc;
-        globalTracer.priorTraceFuncArg = incref(tstate->c_traceobj);
+    if (f) {
+        PyTemporaryReferenceTracer::traceObject(o, f);
+    }
+}
 
-        PyEval_SetTrace(PyTemporaryReferenceTracer::globalTraceFun, nullptr);
+void PyTemporaryReferenceTracer::keepaliveForCurrentInstruction(PyObject* o) {
+    PyThreadState *tstate = PyThreadState_GET();
+    PyFrameObject *f = tstate->frame;
+
+    if (f) {
+        PyTemporaryReferenceTracer::keepaliveForCurrentInstruction(o, f);
     }
 }
 
+
 PyTemporaryReferenceTracer PyTemporaryReferenceTracer::globalTracer;

typed_python/PyTemporaryReferenceTracer.hpp

@@ -19,6 +19,11 @@
 #include "PyInstance.hpp"
 #include <unordered_map>
 
+enum class TraceAction {
+    ConvertTemporaryReference,
+    Decref
+};
+
 class PyTemporaryReferenceTracer {
 public:
     PyTemporaryReferenceTracer() :
@@ -27,7 +32,7 @@ class PyTemporaryReferenceTracer {
         priorTraceFuncArg(nullptr)
     {}
 
-    std::unordered_map<PyFrameObject*, std::vector<PyObject*> > frameToHandles;
+    std::unordered_map<PyFrameObject*, std::vector<std::pair<PyObject*, TraceAction> > > frameToHandles;
 
     // the most recent frame we touched that has nothing in it
     PyFrameObject* mostRecentEmptyFrame;
@@ -43,4 +48,12 @@ class PyTemporaryReferenceTracer {
     // the next time we have an instruction in 'frame', trigger 'o' to become
     // a non-temporary reference
     static void traceObject(PyObject* o, PyFrameObject* frame);
+
+    static void traceObject(PyObject* o);
+
+    static void installGlobalTraceHandlerIfNecessary();
+
+    static void keepaliveForCurrentInstruction(PyObject* o);
+
+    static void keepaliveForCurrentInstruction(PyObject* o, PyFrameObject* frame);
 };