From 6c8db8a5c6ae5942d587e57704945ae74662f948 Mon Sep 17 00:00:00 2001 From: benjaminjohnson2204 Date: Mon, 15 Jan 2024 13:59:39 -0800 Subject: [PATCH 1/8] Fix use-after-free in atexit.unregister() --- Lib/test/_test_atexit.py | 34 ++++++++++++++++++++++++++++++++++ Misc/ACKS | 1 + Modules/atexitmodule.c | 11 ++++++++++- 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/Lib/test/_test_atexit.py b/Lib/test/_test_atexit.py index f618c1fcbca52b..92980f9a52d038 100644 --- a/Lib/test/_test_atexit.py +++ b/Lib/test/_test_atexit.py @@ -135,6 +135,40 @@ def func(): finally: atexit.unregister(func) + def test_eq_unregister_clear(self): + # Issue #112127: callback's __eq__ may call unregister or _clear + global cnt + cnt = 0 + class Func: + def __init__(self, action, eq_ret_val): + self.action = action + self.eq_ret_val = eq_ret_val + + def __call__(self): + return + + def __eq__(self, o): + global cnt + cnt += 1 + if cnt == 1: + self.action(o) + return self.eq_ret_val(o) + + for action, eq_ret_val in ( + (lambda o: atexit.unregister(self), lambda o: NotImplemented), + (lambda o: atexit.unregister(self), lambda o: True), + (lambda o: atexit.unregister(o), lambda o: NotImplemented), + (lambda o: atexit.unregister(o), lambda o: True), + (lambda o: atexit._clear(), lambda o: NotImplemented), + (lambda o: atexit._clear(), lambda o: True), + ): + cnt = 0 + f1 = Func(action, eq_ret_val) + f2 = Func(action, eq_ret_val) + atexit.register(f1) + atexit.register(f2) + atexit._run_exitfuncs() + if __name__ == "__main__": unittest.main() diff --git a/Misc/ACKS b/Misc/ACKS index 466023f390a421..beb817c177fcce 100644 --- a/Misc/ACKS +++ b/Misc/ACKS @@ -881,6 +881,7 @@ Jim Jewett Pedro Diaz Jimenez Orjan Johansen Fredrik Johansson +Benjamin Johnson Gregory K. Johnson Kent Johnson Michael Johnson diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c index b6f1bcbca67916..9c40a10a49d542 100644 --- a/Modules/atexitmodule.c +++ b/Modules/atexitmodule.c @@ -278,11 +278,20 @@ atexit_unregister(PyObject *module, PyObject *func) continue; } + /* + * Increment refcounts of func and cb->func because equality check may + * unregister one or both + */ + PyObject *cb_func = Py_NewRef(cb->func); + PyObject *cur_func = Py_NewRef(func); int eq = PyObject_RichCompareBool(cb->func, func, Py_EQ); + Py_DECREF(cb_func); + Py_DECREF(cur_func); if (eq < 0) { return NULL; } - if (eq) { + // Equality comparison might have already deleted this callback + if (eq && state->callbacks[i] != NULL) { atexit_delete_cb(state, i); } } From 075c49f47ffcecdde7e8badf0ab2bdc4e12f06f1 Mon Sep 17 00:00:00 2001 From: benjaminjohnson2204 Date: Tue, 13 Feb 2024 01:07:07 -0800 Subject: [PATCH 2/8] Address code review comments in atexit tests --- Lib/test/_test_atexit.py | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/Lib/test/_test_atexit.py b/Lib/test/_test_atexit.py index 92980f9a52d038..872dbc1a8ab41d 100644 --- a/Lib/test/_test_atexit.py +++ b/Lib/test/_test_atexit.py @@ -137,7 +137,6 @@ def func(): def test_eq_unregister_clear(self): # Issue #112127: callback's __eq__ may call unregister or _clear - global cnt cnt = 0 class Func: def __init__(self, action, eq_ret_val): @@ -148,26 +147,20 @@ def __call__(self): return def __eq__(self, o): - global cnt cnt += 1 if cnt == 1: self.action(o) return self.eq_ret_val(o) - for action, eq_ret_val in ( - (lambda o: atexit.unregister(self), lambda o: NotImplemented), - (lambda o: atexit.unregister(self), lambda o: True), - (lambda o: atexit.unregister(o), lambda o: NotImplemented), - (lambda o: atexit.unregister(o), lambda o: True), - (lambda o: atexit._clear(), lambda o: NotImplemented), - (lambda o: atexit._clear(), lambda o: True), - ): - cnt = 0 - f1 = Func(action, eq_ret_val) - f2 = Func(action, eq_ret_val) - atexit.register(f1) - atexit.register(f2) - atexit._run_exitfuncs() + for action in lambda o: atexit.unregister(self), lambda o: atexit.unregister(o), lambda o: atexit._clear(): + for eq_ret_val in NotImplemented, True: + with self.subTest(action=action, eq_ret_val=eq_ret_val): + cnt = 0 + f1 = Func(action, eq_ret_val) + f2 = Func(action, eq_ret_val) + atexit.register(f1) + atexit.register(f2) + atexit._run_exitfuncs() if __name__ == "__main__": From 5b022fce6b7083433eb6c51aaeb5140df0d91ad4 Mon Sep 17 00:00:00 2001 From: benjaminjohnson2204 Date: Tue, 13 Feb 2024 08:39:05 -0800 Subject: [PATCH 3/8] Fix linter issue --- Lib/test/_test_atexit.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Lib/test/_test_atexit.py b/Lib/test/_test_atexit.py index 872dbc1a8ab41d..74a8ccec500e95 100644 --- a/Lib/test/_test_atexit.py +++ b/Lib/test/_test_atexit.py @@ -152,8 +152,9 @@ def __eq__(self, o): self.action(o) return self.eq_ret_val(o) - for action in lambda o: atexit.unregister(self), lambda o: atexit.unregister(o), lambda o: atexit._clear(): - for eq_ret_val in NotImplemented, True: + for action in lambda o: atexit.unregister(self), \ + lambda o: atexit.unregister(o), lambda o: atexit._clear(): + for eq_ret_val in NotImplemented, True: with self.subTest(action=action, eq_ret_val=eq_ret_val): cnt = 0 f1 = Func(action, eq_ret_val) From 90568246691435ac49cab85ecd0e153a7d53a19b Mon Sep 17 00:00:00 2001 From: benjaminjohnson2204 Date: Wed, 17 Jul 2024 19:56:03 -0700 Subject: [PATCH 4/8] Address PR comments --- Lib/test/_test_atexit.py | 8 ++++++-- Modules/atexitmodule.c | 2 -- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Lib/test/_test_atexit.py b/Lib/test/_test_atexit.py index 74a8ccec500e95..cd29c5a96176c6 100644 --- a/Lib/test/_test_atexit.py +++ b/Lib/test/_test_atexit.py @@ -147,13 +147,17 @@ def __call__(self): return def __eq__(self, o): + nonlocal cnt cnt += 1 if cnt == 1: self.action(o) return self.eq_ret_val(o) - for action in lambda o: atexit.unregister(self), \ - lambda o: atexit.unregister(o), lambda o: atexit._clear(): + for action in ( + lambda o: atexit.unregister(self), + lambda o: atexit.unregister(o), + lambda o: atexit._clear() + ): for eq_ret_val in NotImplemented, True: with self.subTest(action=action, eq_ret_val=eq_ret_val): cnt = 0 diff --git a/Modules/atexitmodule.c b/Modules/atexitmodule.c index b6737f8fad8c58..71e4ca704a2a7f 100644 --- a/Modules/atexitmodule.c +++ b/Modules/atexitmodule.c @@ -288,10 +288,8 @@ atexit_unregister(PyObject *module, PyObject *func) * unregister one or both */ PyObject *cb_func = Py_NewRef(cb->func); - PyObject *cur_func = Py_NewRef(func); int eq = PyObject_RichCompareBool(cb->func, func, Py_EQ); Py_DECREF(cb_func); - Py_DECREF(cur_func); if (eq < 0) { return NULL; } From 0775ca8f2a02675ede75cc9882815063e82aa15c Mon Sep 17 00:00:00 2001 From: benjaminjohnson2204 Date: Wed, 17 Jul 2024 19:58:46 -0700 Subject: [PATCH 5/8] Remove trailing whitespace --- Lib/test/_test_atexit.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/_test_atexit.py b/Lib/test/_test_atexit.py index cd29c5a96176c6..04ef0b19345b53 100644 --- a/Lib/test/_test_atexit.py +++ b/Lib/test/_test_atexit.py @@ -155,7 +155,7 @@ def __eq__(self, o): for action in ( lambda o: atexit.unregister(self), - lambda o: atexit.unregister(o), + lambda o: atexit.unregister(o), lambda o: atexit._clear() ): for eq_ret_val in NotImplemented, True: From 143fd96c426dce956db1a962819944e3f78874b1 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 17 Dec 2025 14:37:25 +0200 Subject: [PATCH 6/8] Fix tests. --- Lib/test/_test_atexit.py | 39 ++++++++++----------------------------- 1 file changed, 10 insertions(+), 29 deletions(-) diff --git a/Lib/test/_test_atexit.py b/Lib/test/_test_atexit.py index 04ef0b19345b53..490b0686a0c179 100644 --- a/Lib/test/_test_atexit.py +++ b/Lib/test/_test_atexit.py @@ -137,35 +137,16 @@ def func(): def test_eq_unregister_clear(self): # Issue #112127: callback's __eq__ may call unregister or _clear - cnt = 0 - class Func: - def __init__(self, action, eq_ret_val): - self.action = action - self.eq_ret_val = eq_ret_val - - def __call__(self): - return - - def __eq__(self, o): - nonlocal cnt - cnt += 1 - if cnt == 1: - self.action(o) - return self.eq_ret_val(o) - - for action in ( - lambda o: atexit.unregister(self), - lambda o: atexit.unregister(o), - lambda o: atexit._clear() - ): - for eq_ret_val in NotImplemented, True: - with self.subTest(action=action, eq_ret_val=eq_ret_val): - cnt = 0 - f1 = Func(action, eq_ret_val) - f2 = Func(action, eq_ret_val) - atexit.register(f1) - atexit.register(f2) - atexit._run_exitfuncs() + class Evil: + def __eq__(self, other): + action(other) + return NotImplemented + + for action in atexit.unregister, lambda o: atexit._clear(): + with self.subTest(action=action): + atexit.register(lambda: None) + atexit.unregister(Evil()) + atexit._clear() if __name__ == "__main__": From 99e35c99cba0907dffeafad95ffcd285120451a4 Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 17 Dec 2025 14:41:26 +0200 Subject: [PATCH 7/8] Add a NEWS entry. --- .../next/Library/2025-12-17-14-41-09.gh-issue-112127.13OHQk.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 Misc/NEWS.d/next/Library/2025-12-17-14-41-09.gh-issue-112127.13OHQk.rst diff --git a/Misc/NEWS.d/next/Library/2025-12-17-14-41-09.gh-issue-112127.13OHQk.rst b/Misc/NEWS.d/next/Library/2025-12-17-14-41-09.gh-issue-112127.13OHQk.rst new file mode 100644 index 00000000000000..12276e2c9dc384 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-12-17-14-41-09.gh-issue-112127.13OHQk.rst @@ -0,0 +1,2 @@ +Fix possible use-after-free in :func:`atexit.unregister()` when the callback +is unregistered during comparison. From 7ec00974165c23a913578899c1d8193990e5d33a Mon Sep 17 00:00:00 2001 From: Serhiy Storchaka Date: Wed, 17 Dec 2025 14:44:28 +0200 Subject: [PATCH 8/8] Fix use-after-free issue in atexit.unregister --- .../next/Library/2025-12-17-14-41-09.gh-issue-112127.13OHQk.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2025-12-17-14-41-09.gh-issue-112127.13OHQk.rst b/Misc/NEWS.d/next/Library/2025-12-17-14-41-09.gh-issue-112127.13OHQk.rst index 12276e2c9dc384..c983683ebd5589 100644 --- a/Misc/NEWS.d/next/Library/2025-12-17-14-41-09.gh-issue-112127.13OHQk.rst +++ b/Misc/NEWS.d/next/Library/2025-12-17-14-41-09.gh-issue-112127.13OHQk.rst @@ -1,2 +1,2 @@ -Fix possible use-after-free in :func:`atexit.unregister()` when the callback +Fix possible use-after-free in :func:`atexit.unregister` when the callback is unregistered during comparison.