From f16a4479de6272fce665c4b63adaea1e794cc9a4 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Wed, 20 Sep 2023 22:46:32 +0200 Subject: [PATCH] gh-109593: Fix reentrancy issue in multiprocessing resource_tracker --- Lib/multiprocessing/resource_tracker.py | 6 ++++- Lib/test/lock_tests.py | 36 +++++++++++++++++++++++++ Lib/test/test_importlib/test_locks.py | 2 ++ Lib/test/test_threading.py | 3 +++ Lib/threading.py | 7 +++++ Modules/_threadmodule.c | 14 ++++++++++ 6 files changed, 67 insertions(+), 1 deletion(-) diff --git a/Lib/multiprocessing/resource_tracker.py b/Lib/multiprocessing/resource_tracker.py index 3783c1ffc6e4a9..69e71be41ecb0a 100644 --- a/Lib/multiprocessing/resource_tracker.py +++ b/Lib/multiprocessing/resource_tracker.py @@ -54,12 +54,14 @@ class ResourceTracker(object): def __init__(self): - self._lock = threading.Lock() + self._lock = threading.RLock() self._fd = None self._pid = None def _stop(self): with self._lock: + if self._lock._recursion_count() > 1: + return if self._fd is None: # not running return @@ -81,6 +83,8 @@ def ensure_running(self): This can be run from any process. Usually a child process will use the resource created by its parent.''' with self._lock: + if self._lock._recursion_count() > 1: + return if self._fd is not None: # resource tracker was launched before, is it still running? if self._check_alive(): diff --git a/Lib/test/lock_tests.py b/Lib/test/lock_tests.py index a4f52cb20ad301..238e607d20a2b2 100644 --- a/Lib/test/lock_tests.py +++ b/Lib/test/lock_tests.py @@ -330,6 +330,42 @@ def test_release_save_unacquired(self): lock.release() self.assertRaises(RuntimeError, lock._release_save) + def test_recursion_count(self): + lock = self.locktype() + self.assertEqual(0, lock._recursion_count()) + lock.acquire() + self.assertEqual(1, lock._recursion_count()) + lock.acquire() + lock.acquire() + self.assertEqual(3, lock._recursion_count()) + lock.release() + self.assertEqual(2, lock._recursion_count()) + lock.release() + lock.release() + self.assertEqual(0, lock._recursion_count()) + + phase = [] + + def f(): + lock.acquire() + phase.append(None) + while len(phase) == 1: + _wait() + lock.release() + phase.append(None) + + with threading_helper.wait_threads_exit(): + start_new_thread(f, ()) + while len(phase) == 0: + _wait() + self.assertEqual(len(phase), 1) + self.assertEqual(0, lock._recursion_count()) + phase.append(None) + while len(phase) == 2: + _wait() + self.assertEqual(len(phase), 3) + self.assertEqual(0, lock._recursion_count()) + def test_different_thread(self): # Cannot release from a different thread lock = self.locktype() diff --git a/Lib/test/test_importlib/test_locks.py b/Lib/test/test_importlib/test_locks.py index ba9cf51c261d52..7091c36aaaf761 100644 --- a/Lib/test/test_importlib/test_locks.py +++ b/Lib/test/test_importlib/test_locks.py @@ -29,6 +29,8 @@ class ModuleLockAsRLockTests: test_timeout = None # _release_save() unsupported test_release_save_unacquired = None + # _recursion_count() unsupported + test_recursion_count = None # lock status in repr unsupported test_repr = None test_locked_repr = None diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 6465a446565844..ad52bdb25a092e 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -1779,6 +1779,9 @@ class ConditionAsRLockTests(lock_tests.RLockTests): # Condition uses an RLock by default and exports its API. locktype = staticmethod(threading.Condition) + def test_recursion_count(self): + self.skipTest("Condition does not expose _recursion_count()") + class ConditionTests(lock_tests.ConditionTests): condtype = staticmethod(threading.Condition) diff --git a/Lib/threading.py b/Lib/threading.py index f6bbdb0d0c80ee..31cefd2143a8c4 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -245,6 +245,13 @@ def _release_save(self): def _is_owned(self): return self._owner == get_ident() + # Internal method used for reentrancy checks + + def _recursion_count(self): + if self._owner != get_ident(): + return 0 + return self._count + _PyRLock = _RLock diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 7692bacccc4909..800b5fc6fbe06e 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -490,6 +490,18 @@ PyDoc_STRVAR(rlock_release_save_doc, \n\ For internal use by `threading.Condition`."); +static PyObject * +rlock_recursion_count(rlockobject *self, PyObject *Py_UNUSED(ignored)) +{ + unsigned long tid = PyThread_get_thread_ident(); + return PyLong_FromUnsignedLong( + self->rlock_owner == tid ? self->rlock_count : 0UL); +} + +PyDoc_STRVAR(rlock_recursion_count_doc, +"_recursion_count() -> int\n\ +\n\ +For internal use by reentrancy checks."); static PyObject * rlock_is_owned(rlockobject *self, PyObject *Py_UNUSED(ignored)) @@ -565,6 +577,8 @@ static PyMethodDef rlock_methods[] = { METH_VARARGS, rlock_acquire_restore_doc}, {"_release_save", (PyCFunction)rlock_release_save, METH_NOARGS, rlock_release_save_doc}, + {"_recursion_count", (PyCFunction)rlock_recursion_count, + METH_NOARGS, rlock_recursion_count_doc}, {"__enter__", _PyCFunction_CAST(rlock_acquire), METH_VARARGS | METH_KEYWORDS, rlock_acquire_doc}, {"__exit__", (PyCFunction)rlock_release,