Skip to content

gh-123504: Fix reference leak in initialization of _tkinter #123505

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.

Sign up for GitHub

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 9 commits into from
Sep 3, 2024
Merged

gh-123504: Fix reference leak in initialization of _tkinter #123505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cddd021
Fix reference leak in initialization of _tkinter
ZeroIntensity Aug 30, 2024
a9eb482
Add NEWS entry.
ZeroIntensity Aug 30, 2024
36c65af
Switch to `PyModule_AddObjectRef`
ZeroIntensity Aug 30, 2024
cc291e4
Switch to a module clear function.
ZeroIntensity Aug 30, 2024
df9265f
Add back missing newline.
ZeroIntensity Aug 30, 2024
4f1f4e4
Fix formatting.
ZeroIntensity Aug 30, 2024
a77abba
Switch to `Py_CLEAR`
ZeroIntensity Aug 30, 2024
f35f2a0
Update Misc/NEWS.d/next/Library/2024-08-30-09-01-35.gh-issue-123504.l…
ZeroIntensity Sep 3, 2024
fc34562
Use dot notation for module initializer.
ZeroIntensity Sep 3, 2024
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
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2024-08-30-09-01-35.gh-issue-123504.lJ9_BB.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed reference leak in the finalization of :mod:`tkinter`.
27 changes: 19 additions & 8 deletions Modules/_tkinter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3389,17 +3389,28 @@ DisableEventHook(void)
#endif
}

static int
module_clear(PyObject *mod)
{
Py_CLEAR(Tkinter_TclError);
Py_CLEAR(Tkapp_Type);
Py_CLEAR(Tktt_Type);
Py_CLEAR(PyTclObject_Type);
return 0;
}

static void
module_free(void *mod)
{
module_clear((PyObject *)mod);
}

static struct PyModuleDef _tkintermodule = {
PyModuleDef_HEAD_INIT,
"_tkinter",
NULL,
-1,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You removed .m_size = -1, but it's ok, m_size=0 also means "no module state".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a difference between 0 and -1 related to subinterpreters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the documentation:

      Setting ``m_size`` to ``-1`` means that the module does not support
      sub-interpreters, because it has global state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, I forgot about this difference :-(

moduleMethods,
NULL,
NULL,
NULL,
NULL
.m_name = "_tkinter",
.m_methods = moduleMethods,
.m_clear = module_clear,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you define "clear", you should always define "travel".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to emit a warning or even raise an error at runtime?

For example in Objects/typeobject.c, I added an assertion in _PyType_CheckConsistency():

    if (type->tp_flags & Py_TPFLAGS_HAVE_GC) {
        // bpo-44263: tp_traverse is required if Py_TPFLAGS_HAVE_GC is set.
        // Note: tp_clear is optional.
        CHECK(type->tp_traverse != NULL);
    }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good idea. For both types and modules.

.m_free = module_free
};

PyMODINIT_FUNC
Expand Down