Allocating a new weakref object can cause existing weakref objects for

the same object to be collected by the cyclic GC support if they are
only referenced by a cycle.  If the weakref being collected was one of
the weakrefs without callbacks, some local variables for the
constructor became invalid and have to be re-computed.

The test caused a segfault under a debug build without the fix applied.

2 files changed, 48 insertions, 3 deletions

diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py
index f732d7ba81..00cd7ae04f 100644
--- a/Lib/test/test_weakref.py
+++ b/Lib/test/test_weakref.py
@@ -1,3 +1,4 @@
+import gc
 import sys
 import unittest
 import UserList
@@ -591,6 +592,37 @@ class ReferencesTestCase(TestBase):
         gc.collect()
         self.assertEqual(alist, [])
 
+    def test_gc_during_ref_creation(self):
+        self.check_gc_during_creation(weakref.ref)
+
+    def test_gc_during_proxy_creation(self):
+        self.check_gc_during_creation(weakref.proxy)
+
+    def check_gc_during_creation(self, makeref):
+        thresholds = gc.get_threshold()
+        gc.set_threshold(1, 1, 1)
+        gc.collect()
+        class A:
+            pass
+
+        def callback(*args):
+            pass
+
+        referenced = A()
+
+        a = A()
+        a.a = a
+        a.wr = makeref(referenced)
+
+        try:
+            # now make sure the object and the ref get labeled as
+            # cyclic trash:
+            a = A()
+            a.wrc = weakref.ref(referenced, callback)
+
+        finally:
+            gc.set_threshold(*thresholds)
+
 class Object:
     def __init__(self, arg):
         self.arg = arg
diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c
index cf0316a50f..575a928f75 100644
--- a/Objects/weakrefobject.c
+++ b/Objects/weakrefobject.c
@@ -630,16 +630,23 @@ PyWeakref_NewRef(PyObject *ob, PyObject *callback)
         /* return existing weak reference if it exists */
         result = ref;
     if (result != NULL)
-        Py_XINCREF(result);
+        Py_INCREF(result);
     else {
+        /* Note: new_weakref() can trigger cyclic GC, so the weakref
+           list on ob can be mutated.  This means that the ref and
+           proxy pointers we got back earlier may have been collected,
+           so we need to compute these values again before we use
+           them. */
         result = new_weakref(ob, callback);
         if (result != NULL) {
             if (callback == NULL) {
                 insert_head(result, list);
             }
             else {
-                PyWeakReference *prev = (proxy == NULL) ? ref : proxy;
+                PyWeakReference *prev;
 
+                get_basic_refs(*list, &ref, &proxy);
+                prev = (proxy == NULL) ? ref : proxy;
                 if (prev == NULL)
                     insert_head(result, list);
                 else
@@ -672,8 +679,13 @@ PyWeakref_NewProxy(PyObject *ob, PyObject *callback)
         /* attempt to return an existing weak reference if it exists */
         result = proxy;
     if (result != NULL)
-        Py_XINCREF(result);
+        Py_INCREF(result);
     else {
+        /* Note: new_weakref() can trigger cyclic GC, so the weakref
+           list on ob can be mutated.  This means that the ref and
+           proxy pointers we got back earlier may have been collected,
+           so we need to compute these values again before we use
+           them. */
         result = new_weakref(ob, callback);
         if (result != NULL) {
             PyWeakReference *prev;
@@ -682,6 +694,7 @@ PyWeakref_NewProxy(PyObject *ob, PyObject *callback)
                 result->ob_type = &_PyWeakref_CallableProxyType;
             else
                 result->ob_type = &_PyWeakref_ProxyType;
+            get_basic_refs(*list, &ref, &proxy);
             if (callback == NULL)
                 prev = ref;
             else