Skip to content

bpo-31095: fix potential crash during GC. #2974

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 24, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 20 additions & 9 deletions Doc/extending/newtypes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -728,8 +728,9 @@ functions. With :c:func:`Py_VISIT`, :c:func:`Noddy_traverse` can be simplified:
uniformity across these boring implementations.

We also need to provide a method for clearing any subobjects that can
participate in cycles. We implement the method and reimplement the deallocator
to use it::
participate in cycles.

::

static int
Noddy_clear(Noddy *self)
Expand All @@ -747,13 +748,6 @@ to use it::
return 0;
}

static void
Noddy_dealloc(Noddy* self)
{
Noddy_clear(self);
Py_TYPE(self)->tp_free((PyObject*)self);
}

Notice the use of a temporary variable in :c:func:`Noddy_clear`. We use the
temporary variable so that we can set each member to *NULL* before decrementing
its reference count. We do this because, as was discussed earlier, if the
Expand All @@ -776,6 +770,23 @@ be simplified::
return 0;
}

Note that :c:func:`Noddy_dealloc` may call arbitrary functions through
``__del__`` method or weakref callback. It means circular GC can be
triggered inside the function. Since GC assumes reference count is not zero,
we need to untrack the object from GC by calling :c:func:`PyObject_GC_UnTrack`
before clearing members. Here is reimplemented deallocator which uses
:c:func:`PyObject_GC_UnTrack` and :c:func:`Noddy_clear`.

::

static void
Noddy_dealloc(Noddy* self)
{
PyObject_GC_UnTrack(self);
Noddy_clear(self);
Py_TYPE(self)->tp_free((PyObject*)self);
}

Finally, we add the :const:`Py_TPFLAGS_HAVE_GC` flag to the class flags::

Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HAVE_GC, /* tp_flags */
Expand Down
1 change: 1 addition & 0 deletions Doc/includes/noddy4.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ Noddy_clear(Noddy *self)
static void
Noddy_dealloc(Noddy* self)
{
PyObject_GC_UnTrack(self);
Noddy_clear(self);
Py_TYPE(self)->tp_free((PyObject*)self);
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix potential crash during GC caused by ``tp_dealloc`` which doesn't call
``PyObject_GC_UnTrack()``.
4 changes: 4 additions & 0 deletions Modules/_collectionsmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -1717,6 +1717,8 @@ dequeiter_traverse(dequeiterobject *dio, visitproc visit, void *arg)
static void
dequeiter_dealloc(dequeiterobject *dio)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(dio);
Py_XDECREF(dio->deque);
PyObject_GC_Del(dio);
}
Expand Down Expand Up @@ -2097,6 +2099,8 @@ static PyMemberDef defdict_members[] = {
static void
defdict_dealloc(defdictobject *dd)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(dd);
Py_CLEAR(dd->default_factory);
PyDict_Type.tp_dealloc((PyObject *)dd);
}
Expand Down
4 changes: 3 additions & 1 deletion Modules/_elementtree.c
Original file line number Diff line number Diff line change
Expand Up @@ -627,6 +627,7 @@ element_gc_clear(ElementObject *self)
static void
element_dealloc(ElementObject* self)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(self);
Py_TRASHCAN_SAFE_BEGIN(self)

Expand Down Expand Up @@ -2076,14 +2077,15 @@ elementiter_dealloc(ElementIterObject *it)
{
Py_ssize_t i = it->parent_stack_used;
it->parent_stack_used = 0;
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(it);
while (i--)
Py_XDECREF(it->parent_stack[i].parent);
PyMem_Free(it->parent_stack);

Py_XDECREF(it->sought_tag);
Py_XDECREF(it->root_element);

PyObject_GC_UnTrack(it);
PyObject_GC_Del(it);
}

Expand Down
7 changes: 6 additions & 1 deletion Modules/_functoolsmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ partial_new(PyTypeObject *type, PyObject *args, PyObject *kw)
static void
partial_dealloc(partialobject *pto)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(pto);
if (pto->weakreflist != NULL)
PyObject_ClearWeakRefs((PyObject *) pto);
Expand Down Expand Up @@ -1073,7 +1074,11 @@ lru_cache_clear_list(lru_list_elem *link)
static void
lru_cache_dealloc(lru_cache_object *obj)
{
lru_list_elem *list = lru_cache_unlink_list(obj);
lru_list_elem *list;
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(obj);

list = lru_cache_unlink_list(obj);
Py_XDECREF(obj->maxsize_O);
Py_XDECREF(obj->func);
Py_XDECREF(obj->cache);
Expand Down
2 changes: 2 additions & 0 deletions Modules/_io/bytesio.c
Original file line number Diff line number Diff line change
Expand Up @@ -1084,6 +1084,8 @@ bytesiobuf_traverse(bytesiobuf *self, visitproc visit, void *arg)
static void
bytesiobuf_dealloc(bytesiobuf *self)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(self);
Py_CLEAR(self->source);
Py_TYPE(self)->tp_free(self);
}
Expand Down
6 changes: 4 additions & 2 deletions Modules/_json.c
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,8 @@ py_encode_basestring(PyObject* self UNUSED, PyObject *pystr)
static void
scanner_dealloc(PyObject *self)
{
/* Deallocate scanner object */
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(self);
scanner_clear(self);
Py_TYPE(self)->tp_free(self);
}
Expand Down Expand Up @@ -1778,7 +1779,8 @@ encoder_listencode_list(PyEncoderObject *s, _PyAccu *acc,
static void
encoder_dealloc(PyObject *self)
{
/* Deallocate Encoder */
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(self);
encoder_clear(self);
Py_TYPE(self)->tp_free(self);
}
Expand Down
3 changes: 3 additions & 0 deletions Modules/_ssl.c
Original file line number Diff line number Diff line change
Expand Up @@ -2778,6 +2778,8 @@ context_clear(PySSLContext *self)
static void
context_dealloc(PySSLContext *self)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(self);
context_clear(self);
SSL_CTX_free(self->ctx);
#ifdef OPENSSL_NPN_NEGOTIATED
Expand Down Expand Up @@ -4292,6 +4294,7 @@ static PyTypeObject PySSLMemoryBIO_Type = {
static void
PySSLSession_dealloc(PySSLSession *self)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(self);
Py_XDECREF(self->ctx);
if (self->session != NULL) {
Expand Down
2 changes: 2 additions & 0 deletions Modules/_struct.c
Original file line number Diff line number Diff line change
Expand Up @@ -1589,6 +1589,8 @@ typedef struct {
static void
unpackiter_dealloc(unpackiterobject *self)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(self);
Py_XDECREF(self->so);
PyBuffer_Release(&self->buf);
PyObject_GC_Del(self);
Expand Down
6 changes: 6 additions & 0 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -1982,6 +1982,8 @@ dict_dealloc(PyDictObject *mp)
PyObject **values = mp->ma_values;
PyDictKeysObject *keys = mp->ma_keys;
Py_ssize_t i, n;

/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(mp);
Py_TRASHCAN_SAFE_BEGIN(mp)
if (values != NULL) {
Expand Down Expand Up @@ -3407,6 +3409,8 @@ dictiter_new(PyDictObject *dict, PyTypeObject *itertype)
static void
dictiter_dealloc(dictiterobject *di)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
_PyObject_GC_UNTRACK(di);
Py_XDECREF(di->di_dict);
Py_XDECREF(di->di_result);
PyObject_GC_Del(di);
Expand Down Expand Up @@ -3766,6 +3770,8 @@ dictiter_reduce(dictiterobject *di)
static void
dictview_dealloc(_PyDictViewObject *dv)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
_PyObject_GC_UNTRACK(dv);
Py_XDECREF(dv->dv_dict);
PyObject_GC_Del(dv);
}
Expand Down
3 changes: 3 additions & 0 deletions Objects/setobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,7 @@ set_dealloc(PySetObject *so)
setentry *entry;
Py_ssize_t used = so->used;

/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(so);
Py_TRASHCAN_SAFE_BEGIN(so)
if (so->weakreflist != NULL)
Expand Down Expand Up @@ -809,6 +810,8 @@ typedef struct {
static void
setiter_dealloc(setiterobject *si)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
_PyObject_GC_UNTRACK(si);
Py_XDECREF(si->si_set);
PyObject_GC_Del(si);
}
Expand Down
2 changes: 2 additions & 0 deletions Parser/asdl_c.py
Original file line number Diff line number Diff line change
Expand Up @@ -633,6 +633,8 @@ def visitModule(self, mod):
static void
ast_dealloc(AST_object *self)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(self);
Py_CLEAR(self->dict);
Py_TYPE(self)->tp_free(self);
}
Expand Down
2 changes: 2 additions & 0 deletions Python/Python-ast.c
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,8 @@ typedef struct {
static void
ast_dealloc(AST_object *self)
{
/* bpo-31095: UnTrack is needed before calling any callbacks */
PyObject_GC_UnTrack(self);
Py_CLEAR(self->dict);
Py_TYPE(self)->tp_free(self);
}
Expand Down