Skip to content

Commit

Permalink
[3.10] gh-94207: Fix struct module leak (GH-94239) (GH-94266)
Browse files Browse the repository at this point in the history
* gh-94207: Fix struct module leak (GH-94239)

Make _struct.Struct a GC type

This fixes a memory leak in the _struct module, where as soon
as a Struct object is stored in the cache, there's a cycle from
the _struct module to the cache to Struct objects to the Struct
type back to the module. If _struct.Struct is not gc-tracked, that
cycle is never collected.

This PR makes _struct.Struct GC-tracked, and adds a regression test.
(cherry picked from commit 6b86534)

Co-authored-by: Mark Dickinson <dickinsm@gmail.com>
  • Loading branch information
miss-islington and mdickinson authored Jun 25, 2022
1 parent 1494382 commit 86e49a5
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 2 deletions.
18 changes: 18 additions & 0 deletions Lib/test/test_struct.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
from collections import abc
import array
import gc
import math
import operator
import unittest
import struct
import sys
import weakref

from test import support
from test.support import import_helper
from test.support.script_helper import assert_python_ok

ISBIGENDIAN = sys.byteorder == "big"
Expand Down Expand Up @@ -671,6 +674,21 @@ def __del__(self):
self.assertIn(b"Exception ignored in:", stderr)
self.assertIn(b"C.__del__", stderr)

def test__struct_reference_cycle_cleaned_up(self):
# Regression test for python/cpython#94207.

# When we create a new struct module, trigger use of its cache,
# and then delete it ...
_struct_module = import_helper.import_fresh_module("_struct")
module_ref = weakref.ref(_struct_module)
_struct_module.calcsize("b")
del _struct_module

# Then the module should have been garbage collected.
gc.collect()
self.assertIsNone(
module_ref(), "_struct module was not garbage collected")

def test_issue35714(self):
# Embedded null characters should not be allowed in format strings.
for s in '\0', '2\0i', b'\0':
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Made :class:`_struct.Struct` GC-tracked in order to fix a reference leak in
the :mod:`_struct` module.
22 changes: 20 additions & 2 deletions Modules/_struct.c
Original file line number Diff line number Diff line change
Expand Up @@ -1495,10 +1495,26 @@ Struct___init___impl(PyStructObject *self, PyObject *format)
return ret;
}

static int
s_clear(PyStructObject *s)
{
Py_CLEAR(s->s_format);
return 0;
}

static int
s_traverse(PyStructObject *s, visitproc visit, void *arg)
{
Py_VISIT(Py_TYPE(s));
Py_VISIT(s->s_format);
return 0;
}

static void
s_dealloc(PyStructObject *s)
{
PyTypeObject *tp = Py_TYPE(s);
PyObject_GC_UnTrack(s);
if (s->weakreflist != NULL)
PyObject_ClearWeakRefs((PyObject *)s);
if (s->s_codes != NULL) {
Expand Down Expand Up @@ -2079,21 +2095,23 @@ static PyType_Slot PyStructType_slots[] = {
{Py_tp_getattro, PyObject_GenericGetAttr},
{Py_tp_setattro, PyObject_GenericSetAttr},
{Py_tp_doc, (void*)s__doc__},
{Py_tp_traverse, s_traverse},
{Py_tp_clear, s_clear},
{Py_tp_methods, s_methods},
{Py_tp_members, s_members},
{Py_tp_getset, s_getsetlist},
{Py_tp_init, Struct___init__},
{Py_tp_alloc, PyType_GenericAlloc},
{Py_tp_new, s_new},
{Py_tp_free, PyObject_Del},
{Py_tp_free, PyObject_GC_Del},
{0, 0},
};

static PyType_Spec PyStructType_spec = {
"_struct.Struct",
sizeof(PyStructObject),
0,
Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC | Py_TPFLAGS_BASETYPE,
PyStructType_slots
};

Expand Down

0 comments on commit 86e49a5

Please sign in to comment.