From a8266300d7180b3bc4bba3cf6caea22f61b08ab1 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 2 Sep 2019 14:57:05 +0100 Subject: [PATCH 1/4] bpo-38009: Do not call weakref callbacks that are being collected --- Include/cpython/objimpl.h | 2 ++ Modules/gcmodule.c | 2 +- Objects/weakrefobject.c | 24 ++++++++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/Include/cpython/objimpl.h b/Include/cpython/objimpl.h index f121922bc42ced..0d62adaaa99ea3 100644 --- a/Include/cpython/objimpl.h +++ b/Include/cpython/objimpl.h @@ -97,6 +97,8 @@ typedef struct { #define _PyGC_SET_FINALIZED(o) \ _PyGCHead_SET_FINALIZED(_Py_AS_GC(o)) +#define _PyObject_GC_IS_COLLECTING(op) \ + ((((PyGC_Head*)op)->_gc_prev & _PyGC_PREV_MASK_COLLECTING) != 0) PyAPI_FUNC(PyObject *) _PyObject_GC_Malloc(size_t size); PyAPI_FUNC(PyObject *) _PyObject_GC_Calloc(size_t size); diff --git a/Modules/gcmodule.c b/Modules/gcmodule.c index 134f6d168c9616..1f3181470977bc 100644 --- a/Modules/gcmodule.c +++ b/Modules/gcmodule.c @@ -72,7 +72,7 @@ module gc static inline int gc_is_collecting(PyGC_Head *g) { - return (g->_gc_prev & PREV_MASK_COLLECTING) != 0; + return _PyObject_GC_IS_COLLECTING(g); } static inline void diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index daee476444a4f7..2d2217b63ef4cc 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -1,4 +1,5 @@ #include "Python.h" +#include "objimpl.h" #include "structmember.h" @@ -874,6 +875,29 @@ PyWeakref_GetObject(PyObject *ref) static void handle_callback(PyWeakReference *ref, PyObject *callback) { + /* A weak reference may try to invoke a callback object that is being + * cleaned (tp_clear) by the garbage collector and it may be in an + * inconsistent state. As the garbage collector explicitly does not + * invoke callbacks that are part of the same cycle isolate as the + * weak reference (pretending that the weak reference was destroyed first), + * we should act in the same way here. + * + * For example, consider the following scenario: + * + * - F is a function. + * - An object O is in a cycle with F. + * - O has a weak reference with F as a callback. + * + * When running the garbage collector, is possible to end in this function + * if the tp_clear of F decrements the references of O, invoking the weak + * reference callback that will try to call F, which is in an incosistent + * state as is in the middle of its tp_clear and some internal fields may + * be NULL. */ + + if (PyObject_IS_GC(callback) && _PyObject_GC_IS_COLLECTING(callback)) { + return; + } + PyObject *cbresult = _PyObject_CallOneArg(callback, (PyObject *)ref); if (cbresult == NULL) From 012df46ec376978b60a8a657f68fa7c73d765628 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 9 Sep 2019 17:17:51 +0100 Subject: [PATCH 2/4] Update the warning message and the comment --- Objects/weakrefobject.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 2d2217b63ef4cc..24a597e09d7136 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -875,26 +875,28 @@ PyWeakref_GetObject(PyObject *ref) static void handle_callback(PyWeakReference *ref, PyObject *callback) { - /* A weak reference may try to invoke a callback object that is being + /* If the garbage collector support is not properly implemented on + * some classes that are involved in a reference cycle, a weak + * reference may try to invoke a callback object that is being * cleaned (tp_clear) by the garbage collector and it may be in an - * inconsistent state. As the garbage collector explicitly does not - * invoke callbacks that are part of the same cycle isolate as the - * weak reference (pretending that the weak reference was destroyed first), - * we should act in the same way here. + * inconsistent state. As the garbage collector explicitly does + * not invoke callbacks that are part of the same cycle isolate as + * the weak reference (pretending that the weak reference was + * destroyed first), we should act in the same way here. * - * For example, consider the following scenario: - * - * - F is a function. - * - An object O is in a cycle with F. - * - O has a weak reference with F as a callback. - * - * When running the garbage collector, is possible to end in this function - * if the tp_clear of F decrements the references of O, invoking the weak - * reference callback that will try to call F, which is in an incosistent - * state as is in the middle of its tp_clear and some internal fields may + * When running the garbage collector pass over a generation, is + * possible to end in this function if the tp_clear of a function + * decrements the references of some internal object that is + * weak-referenced, invoking the weak reference callback that will + * try to call the function, which is in an incosistent state as + * is in the middle of its tp_clear and some internal fields may * be NULL. */ if (PyObject_IS_GC(callback) && _PyObject_GC_IS_COLLECTING(callback)) { + PyErr_WarnEx(PyExc_RuntimeWarning, "A weak reference" + " was triying to execute a callback to a function that is being cleared by" + " the garbage collector.\n Some C extension class in the dependence" + " chain is probably not implementing correctly the garbage collector support.", 1); return; } From 1324fb52d1f278ce8ee6770f6699170f74b880d1 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 9 Sep 2019 17:32:25 +0100 Subject: [PATCH 3/4] Refactor _PyObject_GC_IS_COLLECTING --- Include/cpython/objimpl.h | 3 +-- Modules/gcmodule.c | 14 +++++++++++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/Include/cpython/objimpl.h b/Include/cpython/objimpl.h index 0d62adaaa99ea3..7d60bbab4ab4da 100644 --- a/Include/cpython/objimpl.h +++ b/Include/cpython/objimpl.h @@ -97,8 +97,7 @@ typedef struct { #define _PyGC_SET_FINALIZED(o) \ _PyGCHead_SET_FINALIZED(_Py_AS_GC(o)) -#define _PyObject_GC_IS_COLLECTING(op) \ - ((((PyGC_Head*)op)->_gc_prev & _PyGC_PREV_MASK_COLLECTING) != 0) +PyAPI_FUNC(int) _PyObject_GC_IS_COLLECTING(PyObject *op); PyAPI_FUNC(PyObject *) _PyObject_GC_Malloc(size_t size); PyAPI_FUNC(PyObject *) _PyObject_GC_Calloc(size_t size); diff --git a/Modules/gcmodule.c b/Modules/gcmodule.c index 1f3181470977bc..d904af712c56c1 100644 --- a/Modules/gcmodule.c +++ b/Modules/gcmodule.c @@ -69,10 +69,22 @@ module gc /* Get the object given the GC head */ #define FROM_GC(g) ((PyObject *)(((PyGC_Head *)g)+1)) +#define _PyGCHead_IS_COLLECTING(o) \ + (((o)->_gc_prev & PREV_MASK_COLLECTING) != 0) + +int _PyObject_GC_IS_COLLECTING(PyObject *op){ + if (PyObject_IS_GC(op)) { + PyGC_Head *gc = AS_GC(op); + return _PyGCHead_IS_COLLECTING(gc); + } else { + return 0; + } +} + static inline int gc_is_collecting(PyGC_Head *g) { - return _PyObject_GC_IS_COLLECTING(g); + return _PyGCHead_IS_COLLECTING(g); } static inline void From 2fb1c1315a3fe4d8c734528040f22e71b173ae77 Mon Sep 17 00:00:00 2001 From: Pablo Galindo Date: Mon, 16 Sep 2019 21:21:56 +0100 Subject: [PATCH 4/4] Adress feedback and add clarifications --- Objects/weakrefobject.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/Objects/weakrefobject.c b/Objects/weakrefobject.c index 24a597e09d7136..c18eb66e921e2c 100644 --- a/Objects/weakrefobject.c +++ b/Objects/weakrefobject.c @@ -880,9 +880,10 @@ handle_callback(PyWeakReference *ref, PyObject *callback) * reference may try to invoke a callback object that is being * cleaned (tp_clear) by the garbage collector and it may be in an * inconsistent state. As the garbage collector explicitly does - * not invoke callbacks that are part of the same cycle isolate as - * the weak reference (pretending that the weak reference was - * destroyed first), we should act in the same way here. + * not invoke callbacks that are part of the same cycle isolate (check + * PEP 442 for references about this terminology) as the weak reference + * (pretending that the weak reference was * destroyed first), we + * should act in the same way here. * * When running the garbage collector pass over a generation, is * possible to end in this function if the tp_clear of a function @@ -894,9 +895,12 @@ handle_callback(PyWeakReference *ref, PyObject *callback) if (PyObject_IS_GC(callback) && _PyObject_GC_IS_COLLECTING(callback)) { PyErr_WarnEx(PyExc_RuntimeWarning, "A weak reference" - " was triying to execute a callback to a function that is being cleared by" - " the garbage collector.\n Some C extension class in the dependence" - " chain is probably not implementing correctly the garbage collector support.", 1); + " was trying to execute a callback to a function that is being cleared by" + " the garbage collector.\n A C extension class in the dependence" + " chain is probably not implementing the garbage collector support" + " correctly.", 1); + /* Return to avoid a potential crash when calling the callback that is in + * an invalid state */ return; }