From 6970bd331c9988910391a6230ed860cf71c5b0a7 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sat, 26 Apr 2025 21:37:49 +1200 Subject: [PATCH 1/5] gh-92408: cleanup during error handling for POSIX shared memory `multiprocessing.shared_memory.SharedMemory` currently tries to unlink a POSIX shared memory object if it fails to `mmap` it after opening/creating it. However, since it only catches `OSError`, it fails to do so when opening an existing zero-length shared memory object, as `mmap.mmap` raises a `ValueError` when given a 0 size. Fix this by catching all exceptions with a bare `except`, so the cleanup is run for all errors. --- Lib/multiprocessing/shared_memory.py | 2 +- Lib/test/_test_multiprocessing.py | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/Lib/multiprocessing/shared_memory.py b/Lib/multiprocessing/shared_memory.py index 99a8ce3320ad4e..cfec862af449d1 100644 --- a/Lib/multiprocessing/shared_memory.py +++ b/Lib/multiprocessing/shared_memory.py @@ -115,7 +115,7 @@ def __init__(self, name=None, create=False, size=0, *, track=True): stats = os.fstat(self._fd) size = stats.st_size self._mmap = mmap.mmap(self._fd, size) - except OSError: + except: self.unlink() raise if self._track: diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 4dc9a31d22f771..518a3fe33ed362 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4851,6 +4851,23 @@ def test_shared_memory_tracking(self): resource_tracker.unregister(mem._name, "shared_memory") mem.close() + @unittest.skipIf(os.name != "posix", "posix-only test w/ empty file") + def test_cleanup_zero_length_shared_memory(self): + import _posixshmem + + name = self._new_shm_name("test_init_cleanup") + mem = _posixshmem.shm_open(name, os.O_RDWR | os.O_CREAT | os.O_EXCL, 0o600) + os.close(mem) + + # First time the mmap.mmap fails with a ValueError, as the shared + # memory is zero-length and hence the mmap fails... + with self.assertRaises(ValueError): + shared_memory.SharedMemory(name, create=False) + + # ...however it should delete the shared memory as part of its cleanup + with self.assertRaises(FileNotFoundError): + _posixshmem.shm_open(name, os.O_RDWR) + # # Test to verify that `Finalize` works. # From 19a65266912ac5126402d565bc94a9c6098bb278 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Thu, 1 May 2025 11:07:12 +1200 Subject: [PATCH 2/5] gh-96026: do not unregister without register in shm POSIX error handling `multiprocessing.shared_memory.SharedMemory` currently tries to unlink a POSIX shared memory object if it fails to mmap it after opening/creating it in the constructor. It does this by calling `cleanup()`, which unregisters the shared memory despite it not having been registered. This will trigger a separate exception in the resource tracker. Fix this by calling the low-level unlink directly. --- Lib/multiprocessing/shared_memory.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/multiprocessing/shared_memory.py b/Lib/multiprocessing/shared_memory.py index cfec862af449d1..fbe531ae507fe7 100644 --- a/Lib/multiprocessing/shared_memory.py +++ b/Lib/multiprocessing/shared_memory.py @@ -116,7 +116,7 @@ def __init__(self, name=None, create=False, size=0, *, track=True): size = stats.st_size self._mmap = mmap.mmap(self._fd, size) except: - self.unlink() + _posixshmem.shm_unlink(self._name) raise if self._track: resource_tracker.register(self._name, "shared_memory") From e33f3419bb98f0e66d3f0f29c0b1edea78fea4be Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sat, 26 Apr 2025 22:26:45 +1200 Subject: [PATCH 3/5] Do not unlink pre-existing POSIX shared memory on error `multiprocessing.shared_memory.SharedMemory` currently attempts to `unlink` a POSIX shared memory object it has opened or created if a subsequent exception is raised in its constructor. This is necessary to avoid it leaking if the shared memory object was created in the constructor, however it seems inappropriate if it is opening a pre-existing shared memory object. Fix this by only unlinking the shared memory object if it was created. --- Lib/multiprocessing/shared_memory.py | 3 ++- Lib/test/_test_multiprocessing.py | 25 ++++++++++++++++++++++--- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/Lib/multiprocessing/shared_memory.py b/Lib/multiprocessing/shared_memory.py index fbe531ae507fe7..b6fef28a08d084 100644 --- a/Lib/multiprocessing/shared_memory.py +++ b/Lib/multiprocessing/shared_memory.py @@ -116,7 +116,8 @@ def __init__(self, name=None, create=False, size=0, *, track=True): size = stats.st_size self._mmap = mmap.mmap(self._fd, size) except: - _posixshmem.shm_unlink(self._name) + if create: + _posixshmem.shm_unlink(self._name) raise if self._track: resource_tracker.register(self._name, "shared_memory") diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index 518a3fe33ed362..df1b680181b32c 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4851,6 +4851,26 @@ def test_shared_memory_tracking(self): resource_tracker.unregister(mem._name, "shared_memory") mem.close() + @unittest.skipIf(os.name != "posix", "windows automatically unlinks") + def test_creating_shm_unlinks_on_error(self): + name = self._new_shm_name("test_creating_shm_unlinks_on_error") + with unittest.mock.patch('mmap.mmap') as mock_mmap: + mock_mmap.side_effect = OSError("filesystems are evil") + with self.assertRaises(OSError): + shared_memory.SharedMemory(name, create=True, size=1) + with self.assertRaises(FileNotFoundError): + import _posixshmem + _posixshmem.shm_unlink(name) + + def test_existing_shm_not_unlinked_on_error(self): + name = self._new_shm_name("test_existing_shm_not_unlinked_on_error") + mem = shared_memory.SharedMemory(name, create=True, size=1) + self.addCleanup(mem.unlink) + with unittest.mock.patch('mmap.mmap') as mock_mmap: + mock_mmap.side_effect = OSError("filesystems are evil") + with self.assertRaises(OSError): + shared_memory.SharedMemory(name, create=False) + @unittest.skipIf(os.name != "posix", "posix-only test w/ empty file") def test_cleanup_zero_length_shared_memory(self): import _posixshmem @@ -4864,9 +4884,8 @@ def test_cleanup_zero_length_shared_memory(self): with self.assertRaises(ValueError): shared_memory.SharedMemory(name, create=False) - # ...however it should delete the shared memory as part of its cleanup - with self.assertRaises(FileNotFoundError): - _posixshmem.shm_open(name, os.O_RDWR) + # ...but it should NOT delete the shared memory as part of its cleanup + _posixshmem.shm_unlink(name) # # Test to verify that `Finalize` works. From 5835415798658beca23a44247b74d5a647c29419 Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Sat, 26 Apr 2025 23:08:46 +1200 Subject: [PATCH 4/5] Allow zero-length shared memory objects These are allowed by POSIX and it seems more consistent/cleaner to support them than forcing the user to treat them as a special case. Windows does not support creating a zero-length mapping, and neither does `mmap` on any platform, so skip those operations and assign a zero-length buffer instead. TODO: The way the POSIX/Windows code is interleaved inside if statements makes for difficult to follow and deeply nested logic. It might be better to refactor this code and keep the platform-specific logic in separate functions or possibly even implementation classes. --- Lib/multiprocessing/shared_memory.py | 23 +++++++++++++--------- Lib/test/_test_multiprocessing.py | 29 ++++++---------------------- 2 files changed, 20 insertions(+), 32 deletions(-) diff --git a/Lib/multiprocessing/shared_memory.py b/Lib/multiprocessing/shared_memory.py index b6fef28a08d084..80b4cf4c9038ef 100644 --- a/Lib/multiprocessing/shared_memory.py +++ b/Lib/multiprocessing/shared_memory.py @@ -74,12 +74,10 @@ class SharedMemory: _track = True def __init__(self, name=None, create=False, size=0, *, track=True): - if not size >= 0: - raise ValueError("'size' must be a positive integer") + if size < 0: + raise ValueError("'size' must be a non-negative integer") if create: self._flags = _O_CREX | os.O_RDWR - if size == 0: - raise ValueError("'size' must be a positive number different from zero") if name is None and not self._flags & os.O_EXCL: raise ValueError("'name' can only be None if create=True") @@ -114,7 +112,8 @@ def __init__(self, name=None, create=False, size=0, *, track=True): os.ftruncate(self._fd, size) stats = os.fstat(self._fd) size = stats.st_size - self._mmap = mmap.mmap(self._fd, size) + if size > 0: + self._mmap = mmap.mmap(self._fd, size) except: if create: _posixshmem.shm_unlink(self._name) @@ -126,7 +125,7 @@ def __init__(self, name=None, create=False, size=0, *, track=True): # Windows Named Shared Memory - if create: + if create and size > 0: while True: temp_name = _make_filename() if name is None else name # Create and reserve shared memory block with this name @@ -156,7 +155,9 @@ def __init__(self, name=None, create=False, size=0, *, track=True): _winapi.CloseHandle(h_map) self._name = temp_name break - + elif create and size == 0: + # TODO: Leave as None? + self._name = _make_filename() if name is None else name else: self._name = name # Dynamically determine the existing named shared memory @@ -180,10 +181,14 @@ def __init__(self, name=None, create=False, size=0, *, track=True): size = _winapi.VirtualQuerySize(p_buf) finally: _winapi.UnmapViewOfFile(p_buf) - self._mmap = mmap.mmap(-1, size, tagname=name) + if size > 0: + self._mmap = mmap.mmap(-1, size, tagname=name) self._size = size - self._buf = memoryview(self._mmap) + if size > 0: + self._buf = memoryview(self._mmap) + else: + self._buf = memoryview(b'') def __del__(self): try: diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index df1b680181b32c..a1665987f6ad60 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4447,14 +4447,6 @@ def test_invalid_shared_memory_creation(self): with self.assertRaises(ValueError): sms_invalid = shared_memory.SharedMemory(create=True, size=-1) - # Test creating a shared memory segment with size 0 - with self.assertRaises(ValueError): - sms_invalid = shared_memory.SharedMemory(create=True, size=0) - - # Test creating a shared memory segment without size argument - with self.assertRaises(ValueError): - sms_invalid = shared_memory.SharedMemory(create=True) - def test_shared_memory_pickle_unpickle(self): for proto in range(pickle.HIGHEST_PROTOCOL + 1): with self.subTest(proto=proto): @@ -4871,21 +4863,12 @@ def test_existing_shm_not_unlinked_on_error(self): with self.assertRaises(OSError): shared_memory.SharedMemory(name, create=False) - @unittest.skipIf(os.name != "posix", "posix-only test w/ empty file") - def test_cleanup_zero_length_shared_memory(self): - import _posixshmem - - name = self._new_shm_name("test_init_cleanup") - mem = _posixshmem.shm_open(name, os.O_RDWR | os.O_CREAT | os.O_EXCL, 0o600) - os.close(mem) - - # First time the mmap.mmap fails with a ValueError, as the shared - # memory is zero-length and hence the mmap fails... - with self.assertRaises(ValueError): - shared_memory.SharedMemory(name, create=False) - - # ...but it should NOT delete the shared memory as part of its cleanup - _posixshmem.shm_unlink(name) + def test_zero_length_shared_memory(self): + name = self._new_shm_name("test_zero_length_shared_memory") + mem = shared_memory.SharedMemory(name, create=True, size=0) + self.addCleanup(mem.unlink) + self.assertEqual(mem.size, 0) + self.assertEqual(len(mem.buf), 0) # # Test to verify that `Finalize` works. From 1fe41a0e2d0112a3131164002db3d82fb73241ff Mon Sep 17 00:00:00 2001 From: Duane Griffin Date: Thu, 1 May 2025 20:22:44 +1200 Subject: [PATCH 5/5] Use shorter names for shared memory objects in tests It appears MacOS has tight limits on name length which the new tests were exceeding. --- Lib/test/_test_multiprocessing.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/_test_multiprocessing.py b/Lib/test/_test_multiprocessing.py index a1665987f6ad60..6514d0f786f6ea 100644 --- a/Lib/test/_test_multiprocessing.py +++ b/Lib/test/_test_multiprocessing.py @@ -4845,7 +4845,7 @@ def test_shared_memory_tracking(self): @unittest.skipIf(os.name != "posix", "windows automatically unlinks") def test_creating_shm_unlinks_on_error(self): - name = self._new_shm_name("test_creating_shm_unlinks_on_error") + name = self._new_shm_name("test_csuoe") with unittest.mock.patch('mmap.mmap') as mock_mmap: mock_mmap.side_effect = OSError("filesystems are evil") with self.assertRaises(OSError): @@ -4855,7 +4855,7 @@ def test_creating_shm_unlinks_on_error(self): _posixshmem.shm_unlink(name) def test_existing_shm_not_unlinked_on_error(self): - name = self._new_shm_name("test_existing_shm_not_unlinked_on_error") + name = self._new_shm_name("test_esnuoe") mem = shared_memory.SharedMemory(name, create=True, size=1) self.addCleanup(mem.unlink) with unittest.mock.patch('mmap.mmap') as mock_mmap: @@ -4864,7 +4864,7 @@ def test_existing_shm_not_unlinked_on_error(self): shared_memory.SharedMemory(name, create=False) def test_zero_length_shared_memory(self): - name = self._new_shm_name("test_zero_length_shared_memory") + name = self._new_shm_name("test_zlsm") mem = shared_memory.SharedMemory(name, create=True, size=0) self.addCleanup(mem.unlink) self.assertEqual(mem.size, 0)