Skip to content

Commit 2123fdc

Browse files
committed
Make the signal handler table safe thread safe without a lock
Utilise atomic_exchange and `QSBR` to ensure that accessing an old handler does not result in a potential UAF.
1 parent 7b12a41 commit 2123fdc

3 files changed

Lines changed: 56 additions & 15 deletions

File tree

‎Lib/test/test_signal.py‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import threading
1313
import time
1414
import unittest
15+
import weakref
1516
from test import support
1617
from test.support import (
1718
force_not_colorized, is_apple, is_apple_mobile, os_helper, threading_helper
@@ -1441,6 +1442,42 @@ def handler(a, b):
14411442
signal.raise_signal(signal.SIGINT)
14421443
self.assertTrue(is_ok)
14431444

1445+
def test_handler_replacing_itself_stays_alive(self):
1446+
events = []
1447+
def handler(signum, frame):
1448+
events.append("enter")
1449+
signal.signal(signum, signal.SIG_DFL)
1450+
support.gc_collect()
1451+
events.append("exit")
1452+
handler = functools.partial(handler)
1453+
wr = weakref.ref(handler, lambda ref: events.append("freed"))
1454+
old_signal = signal.signal(signal.SIGINT, handler)
1455+
self.addCleanup(signal.signal, signal.SIGINT, old_signal)
1456+
del handler
1457+
1458+
signal.raise_signal(signal.SIGINT)
1459+
support.gc_collect()
1460+
self.assertEqual(events, ["enter", "exit", "freed"])
1461+
1462+
def test_handler_replacing_itself_and_raising_is_freed(self):
1463+
events = []
1464+
def handler(signum, frame):
1465+
events.append("enter")
1466+
signal.signal(signum, signal.SIG_DFL)
1467+
support.gc_collect()
1468+
events.append("exit")
1469+
raise ZeroDivisionError
1470+
handler = functools.partial(handler)
1471+
wr = weakref.ref(handler, lambda ref: events.append("freed"))
1472+
old_signal = signal.signal(signal.SIGINT, handler)
1473+
self.addCleanup(signal.signal, signal.SIGINT, old_signal)
1474+
del handler
1475+
1476+
with self.assertRaises(ZeroDivisionError):
1477+
signal.raise_signal(signal.SIGINT)
1478+
support.gc_collect()
1479+
self.assertEqual(events, ["enter", "exit", "freed"])
1480+
14441481
def test__thread_interrupt_main(self):
14451482
# See https://github.com/python/cpython/issues/102397
14461483
code = """if 1:
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fix a possible crash in :func:`signal.getsignal` on a free-threaded build when
2+
another thread replaces the signal handler at the same time. A Python signal
3+
handler that replaces itself with :func:`signal.signal` now stays alive until
4+
it returns.

‎Modules/signalmodule.c‎

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include "pycore_fileutils.h" // _Py_BEGIN_SUPPRESS_IPH
1111
#include "pycore_interpframe.h" // _PyThreadState_GetFrame()
1212
#include "pycore_moduleobject.h" // _PyModule_GetState()
13+
#include "pycore_object.h" // _PyObject_XDecRefDelayed()
1314
#include "pycore_pyerrors.h" // _PyErr_SetString()
1415
#include "pycore_pystate.h" // _PyThreadState_GET()
1516
#include "pycore_signal.h" // _Py_RestoreSignals()
@@ -135,12 +136,14 @@ get_handler(int i)
135136
return (PyObject *)_Py_atomic_load_ptr(&Handlers[i].func);
136137
}
137138

138-
Py_LOCAL_INLINE(void)
139-
set_handler(int i, PyObject* func)
139+
Py_LOCAL_INLINE(PyObject *)
140+
swap_handler(int i, PyObject *func)
140141
{
141-
/* Store func with atomic operation to ensure
142-
that PyErr_SetInterrupt is async-signal-safe. */
143-
_Py_atomic_store_ptr(&Handlers[i].func, func);
142+
PyObject *old = _Py_atomic_exchange_ptr(&Handlers[i].func,
143+
Py_XNewRef(func));
144+
PyObject *result = Py_XNewRef(old);
145+
_PyObject_XDecRefDelayed(old);
146+
return result;
144147
}
145148

146149

@@ -532,9 +535,7 @@ signal_signal_impl(PyObject *module, int signalnum, PyObject *handler)
532535
return NULL;
533536
}
534537

535-
old_handler = get_handler(signalnum);
536-
set_handler(signalnum, Py_NewRef(handler));
537-
538+
old_handler = swap_handler(signalnum, handler);
538539
if (old_handler != NULL) {
539540
return old_handler;
540541
}
@@ -1595,9 +1596,7 @@ signal_get_set_handlers(signal_state_t *state, PyObject *mod_dict)
15951596
}
15961597
// If signal_module_exec() is called more than one, we must
15971598
// clear the strong reference to the previous function.
1598-
PyObject* old_func = get_handler(signum);
1599-
set_handler(signum, Py_NewRef(func));
1600-
Py_XDECREF(old_func);
1599+
Py_XDECREF(swap_handler(signum, func));
16011600
}
16021601

16031602
// Install Python SIGINT handler which raises KeyboardInterrupt
@@ -1609,8 +1608,8 @@ signal_get_set_handlers(signal_state_t *state, PyObject *mod_dict)
16091608
return -1;
16101609
}
16111610

1612-
set_handler(SIGINT, int_handler);
1613-
Py_DECREF(sigint_func);
1611+
Py_XDECREF(swap_handler(SIGINT, int_handler));
1612+
Py_DECREF(int_handler);
16141613
PyOS_setsig(SIGINT, signal_handler);
16151614
}
16161615
return 0;
@@ -1744,9 +1743,8 @@ _PySignal_Fini(void)
17441743

17451744
// Restore default signals and clear handlers
17461745
for (int signum = 1; signum < Py_NSIG; signum++) {
1747-
PyObject *func = get_handler(signum);
17481746
_Py_atomic_store_int_relaxed(&Handlers[signum].tripped, 0);
1749-
set_handler(signum, NULL);
1747+
PyObject *func = swap_handler(signum, NULL);
17501748
if (func != NULL
17511749
&& func != Py_None
17521750
&& !compare_handler(func, state->default_handler)
@@ -1864,6 +1862,7 @@ _PyErr_CheckSignalsTstate(PyThreadState *tstate)
18641862
"calling signal handler");
18651863
continue;
18661864
}
1865+
Py_INCREF(func);
18671866
PyObject *arglist = NULL;
18681867
if (frame == NULL) {
18691868
arglist = Py_BuildValue("(iO)", i, Py_None);
@@ -1882,6 +1881,7 @@ _PyErr_CheckSignalsTstate(PyThreadState *tstate)
18821881
else {
18831882
result = NULL;
18841883
}
1884+
Py_DECREF(func);
18851885
if (!result) {
18861886
/* On error, re-schedule a call to _PyErr_CheckSignalsTstate() */
18871887
_Py_atomic_store_int(&is_tripped, 1);

0 commit comments

Comments
 (0)