GH-127705: better double free message. (GH-130785)

* Add location information when accessing already closed stackref

* Add #def option to track closed stackrefs to provide precise information for use after free and double frees.

Author: markshannon

Include/internal/pycore_interp.h

@@ -296,7 +296,10 @@ struct _is {
 
 #if !defined(Py_GIL_DISABLED) && defined(Py_STACKREF_DEBUG)
     uint64_t next_stackref;
-    _Py_hashtable_t *stackref_debug_table;
+    _Py_hashtable_t *open_stackrefs_table;
+#  ifdef Py_STACKREF_CLOSE_DEBUG
+    _Py_hashtable_t *closed_stackrefs_table;
+#  endif
 #endif
 };
 

Include/internal/pycore_stackref.h

@@ -7,6 +7,11 @@ extern "C" {
 // Define this to get precise tracking of stackrefs.
 // #define Py_STACKREF_DEBUG 1
 
+// Define this to get precise tracking of closed stackrefs.
+// This will use unbounded memory, as it can only grow.
+// Use this to track double closes in short-lived programs
+// #define Py_STACKREF_CLOSE_DEBUG 1
+
 #ifndef Py_BUILD_CORE
 #  error "this header requires Py_BUILD_CORE define"
 #endif
@@ -64,7 +69,7 @@ typedef union _PyStackRef {
 #define Py_TAG_BITS 0
 
 PyAPI_FUNC(PyObject *) _Py_stackref_get_object(_PyStackRef ref);
-PyAPI_FUNC(PyObject *) _Py_stackref_close(_PyStackRef ref);
+PyAPI_FUNC(PyObject *) _Py_stackref_close(_PyStackRef ref, const char *filename, int linenumber);
 PyAPI_FUNC(_PyStackRef) _Py_stackref_create(PyObject *obj, const char *filename, int linenumber);
 PyAPI_FUNC(void) _Py_stackref_record_borrow(_PyStackRef ref, const char *filename, int linenumber);
 extern void _Py_stackref_associate(PyInterpreterState *interp, PyObject *obj, _PyStackRef ref);
@@ -111,10 +116,11 @@ _PyStackRef_AsPyObjectBorrow(_PyStackRef ref, const char *filename, int linenumb
 #define PyStackRef_AsPyObjectBorrow(REF) _PyStackRef_AsPyObjectBorrow((REF), __FILE__, __LINE__)
 
 static inline PyObject *
-PyStackRef_AsPyObjectSteal(_PyStackRef ref)
+_PyStackRef_AsPyObjectSteal(_PyStackRef ref, const char *filename, int linenumber)
 {
-    return _Py_stackref_close(ref);
+    return _Py_stackref_close(ref, filename, linenumber);
 }
+#define PyStackRef_AsPyObjectSteal(REF) _PyStackRef_AsPyObjectSteal((REF), __FILE__, __LINE__)
 
 static inline _PyStackRef
 _PyStackRef_FromPyObjectNew(PyObject *obj, const char *filename, int linenumber)
@@ -140,11 +146,12 @@ _PyStackRef_FromPyObjectImmortal(PyObject *obj, const char *filename, int linenu
 #define PyStackRef_FromPyObjectImmortal(obj) _PyStackRef_FromPyObjectImmortal(_PyObject_CAST(obj), __FILE__, __LINE__)
 
 static inline void
-PyStackRef_CLOSE(_PyStackRef ref)
+_PyStackRef_CLOSE(_PyStackRef ref, const char *filename, int linenumber)
 {
-    PyObject *obj = _Py_stackref_close(ref);
+    PyObject *obj = _Py_stackref_close(ref, filename, linenumber);
     Py_DECREF(obj);
 }
+#define PyStackRef_CLOSE(REF) _PyStackRef_CLOSE((REF), __FILE__, __LINE__)
 
 static inline _PyStackRef
 _PyStackRef_DUP(_PyStackRef ref, const char *filename, int linenumber)

Objects/object.c

@@ -2984,7 +2984,7 @@ _Py_Dealloc(PyObject *op)
     destructor dealloc = type->tp_dealloc;
 #ifdef Py_DEBUG
     PyThreadState *tstate = _PyThreadState_GET();
-#ifndef Py_GIL_DISABLED
+#if !defined(Py_GIL_DISABLED) && !defined(Py_STACKREF_DEBUG)
     /* This assertion doesn't hold for the free-threading build, as
      * PyStackRef_CLOSE_SPECIALIZED is not implemented */
     assert(tstate->current_frame == NULL || tstate->current_frame->stackpointer != NULL);

Python/pystate.c

@@ -676,13 +676,22 @@ init_interpreter(PyInterpreterState *interp,
         .malloc = malloc,
         .free = free,
     };
-    interp->stackref_debug_table = _Py_hashtable_new_full(
+    interp->open_stackrefs_table = _Py_hashtable_new_full(
         _Py_hashtable_hash_ptr,
         _Py_hashtable_compare_direct,
         NULL,
         NULL,
         &alloc
     );
+#  ifdef Py_STACKREF_CLOSE_DEBUG
+    interp->closed_stackrefs_table = _Py_hashtable_new_full(
+        _Py_hashtable_hash_ptr,
+        _Py_hashtable_compare_direct,
+        NULL,
+        NULL,
+        &alloc
+    );
+#  endif
     _Py_stackref_associate(interp, Py_None, PyStackRef_None);
     _Py_stackref_associate(interp, Py_False, PyStackRef_False);
     _Py_stackref_associate(interp, Py_True, PyStackRef_True);
@@ -901,9 +910,13 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
     Py_CLEAR(interp->builtins);
 
 #if !defined(Py_GIL_DISABLED) && defined(Py_STACKREF_DEBUG)
+#  ifdef Py_STACKREF_CLOSE_DEBUG
+    _Py_hashtable_destroy(interp->closed_stackrefs_table);
+    interp->closed_stackrefs_table = NULL;
+#  endif
     _Py_stackref_report_leaks(interp);
-    _Py_hashtable_destroy(interp->stackref_debug_table);
-    interp->stackref_debug_table = NULL;
+    _Py_hashtable_destroy(interp->open_stackrefs_table);
+    interp->open_stackrefs_table = NULL;
 #endif
 
     if (tstate->interp == interp) {

Python/stackrefs.c

@@ -47,33 +47,51 @@ _Py_stackref_get_object(_PyStackRef ref)
     if (ref.index >= interp->next_stackref) {
         _Py_FatalErrorFormat(__func__, "Garbled stack ref with ID %" PRIu64 "\n", ref.index);
     }
-    TableEntry *entry = _Py_hashtable_get(interp->stackref_debug_table, (void *)ref.index);
+    TableEntry *entry = _Py_hashtable_get(interp->open_stackrefs_table, (void *)ref.index);
     if (entry == NULL) {
         _Py_FatalErrorFormat(__func__, "Accessing closed stack ref with ID %" PRIu64 "\n", ref.index);
     }
     return entry->obj;
 }
 
 PyObject *
-_Py_stackref_close(_PyStackRef ref)
+_Py_stackref_close(_PyStackRef ref, const char *filename, int linenumber)
 {
     PyInterpreterState *interp = PyInterpreterState_Get();
     if (ref.index >= interp->next_stackref) {
-        _Py_FatalErrorFormat(__func__, "Garbled stack ref with ID %" PRIu64 "\n", ref.index);
+        _Py_FatalErrorFormat(__func__, "Invalid StackRef with ID %" PRIu64 " at %s:%d\n", (void *)ref.index, filename, linenumber);
+
     }
     PyObject *obj;
     if (ref.index <= LAST_PREDEFINED_STACKREF_INDEX) {
         // Pre-allocated reference to None, False or True -- Do not clear
-        TableEntry *entry = _Py_hashtable_get(interp->stackref_debug_table, (void *)ref.index);
+        TableEntry *entry = _Py_hashtable_get(interp->open_stackrefs_table, (void *)ref.index);
         obj = entry->obj;
     }
     else {
-        TableEntry *entry = _Py_hashtable_steal(interp->stackref_debug_table, (void *)ref.index);
+        TableEntry *entry = _Py_hashtable_steal(interp->open_stackrefs_table, (void *)ref.index);
         if (entry == NULL) {
+#ifdef Py_STACKREF_CLOSE_DEBUG
+            entry = _Py_hashtable_get(interp->closed_stackrefs_table, (void *)ref.index);
+            if (entry != NULL) {
+                _Py_FatalErrorFormat(__func__,
+                    "Double close of ref ID %" PRIu64 " at %s:%d. Referred to instance of %s at %p. Closed at %s:%d\n",
+                    (void *)ref.index, filename, linenumber, entry->classname, entry->obj, entry->filename, entry->linenumber);
+            }
+#endif
             _Py_FatalErrorFormat(__func__, "Invalid StackRef with ID %" PRIu64 "\n", (void *)ref.index);
         }
         obj = entry->obj;
         free(entry);
+#ifdef Py_STACKREF_CLOSE_DEBUG
+        TableEntry *close_entry = make_table_entry(obj, filename, linenumber);
+        if (close_entry == NULL) {
+            Py_FatalError("No memory left for stackref debug table");
+        }
+        if (_Py_hashtable_set(interp->closed_stackrefs_table, (void *)ref.index, close_entry) < 0) {
+            Py_FatalError("No memory left for stackref debug table");
+        }
+#endif
     }
     return obj;
 }
@@ -90,7 +108,7 @@ _Py_stackref_create(PyObject *obj, const char *filename, int linenumber)
     if (entry == NULL) {
         Py_FatalError("No memory left for stackref debug table");
     }
-    if (_Py_hashtable_set(interp->stackref_debug_table, (void *)new_id, entry) < 0) {
+    if (_Py_hashtable_set(interp->open_stackrefs_table, (void *)new_id, entry) < 0) {
         Py_FatalError("No memory left for stackref debug table");
     }
     return (_PyStackRef){ .index = new_id };
@@ -103,9 +121,17 @@ _Py_stackref_record_borrow(_PyStackRef ref, const char *filename, int linenumber
         return;
     }
     PyInterpreterState *interp = PyInterpreterState_Get();
-    TableEntry *entry = _Py_hashtable_get(interp->stackref_debug_table, (void *)ref.index);
+    TableEntry *entry = _Py_hashtable_get(interp->open_stackrefs_table, (void *)ref.index);
     if (entry == NULL) {
-        _Py_FatalErrorFormat(__func__, "Invalid StackRef with ID %" PRIu64 "\n", (void *)ref.index);
+#ifdef Py_STACKREF_CLOSE_DEBUG
+        entry = _Py_hashtable_get(interp->closed_stackrefs_table, (void *)ref.index);
+        if (entry != NULL) {
+            _Py_FatalErrorFormat(__func__,
+                "Borrow of closed ref ID %" PRIu64 " at %s:%d. Referred to instance of %s at %p. Closed at %s:%d\n",
+                (void *)ref.index, filename, linenumber, entry->classname, entry->obj, entry->filename, entry->linenumber);
+        }
+#endif
+        _Py_FatalErrorFormat(__func__, "Invalid StackRef with ID %" PRIu64 " at %s:%d\n", (void *)ref.index, filename, linenumber);
     }
     entry->filename_borrow = filename;
     entry->linenumber_borrow = linenumber;
@@ -121,7 +147,7 @@ _Py_stackref_associate(PyInterpreterState *interp, PyObject *obj, _PyStackRef re
     if (entry == NULL) {
         Py_FatalError("No memory left for stackref debug table");
     }
-    if (_Py_hashtable_set(interp->stackref_debug_table, (void *)ref.index, (void *)entry) < 0) {
+    if (_Py_hashtable_set(interp->open_stackrefs_table, (void *)ref.index, (void *)entry) < 0) {
         Py_FatalError("No memory left for stackref debug table");
     }
 }
@@ -147,7 +173,7 @@ void
 _Py_stackref_report_leaks(PyInterpreterState *interp)
 {
     int leak = 0;
-    _Py_hashtable_foreach(interp->stackref_debug_table, report_leak, &leak);
+    _Py_hashtable_foreach(interp->open_stackrefs_table, report_leak, &leak);
     if (leak) {
         Py_FatalError("Stackrefs leaked.");
     }