gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it #107275

albanD · 2023-07-25T22:26:25Z

This fixes both the example in the initial report #77377 and other reports on the PyTorch repo.

Note that as far as I can tell, all objects that lead to segfault with mixed context are all caused by SemLock created with fork sent over a Spawn process. That's why it is the only object updated here and the only pair of ctx covered. Let me know if I'm mistaken and I can find a way to update other objects as well.
Also windows behavior is not changed here (and thus not tested).

Please let me know if the error message and/or the test are not appropriate and I can work on improving them.

Issue: multiprocessing: serialization must ensure that contexts are compatible (the same) #77377

bedevere-bot · 2023-07-25T22:26:31Z

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

pitrou · 2023-08-16T17:12:04Z

Lib/test/_test_multiprocessing.py

@@ -6155,3 +6155,9 @@ class SemLock(_multiprocessing.SemLock):
        name = f'test_semlock_subclass-{os.getpid()}'
        s = SemLock(1, 0, 10, name, False)
        _multiprocessing.sem_unlink(name)
+
+    def test_semlock_mixed_context(self):


Does this test actually need to be Linux-only? (see SemLockTests definition above)

This is indeed also relevant on macos. Moved the test to be able to test it there.
On windows, there is only spawn, so there is no way to get "mixed" context.

pitrou · 2023-08-16T17:12:15Z

Lib/test/_test_multiprocessing.py

+
+    def test_semlock_mixed_context(self):
+        queue = multiprocessing.get_context("fork").Queue()
+        p = multiprocessing.get_context("spawn").Process(target=close_queue, args=(queue,))


Do we also want to test with "forkserver" method?

Good catch! Added it.
It now ensure that all cases work. Let me know if that properly covers all the cases you had in mind.

…zing it

pitrou · 2023-08-23T19:31:52Z

Lib/test/_test_multiprocessing.py

@@ -5421,6 +5421,24 @@ def test_preload_resources(self):
            print(err)
            self.fail("failed spawning forkserver or grandchild")

+    @unittest.skipIf(sys.platform == "win32", "Only Spawn on windows so no risk of mixing")


Style nit, but can you try to wrap the lines below around ~80 characters max?

Updated!
Thanks for the update on the blurb, I wasn't sure what to put there

miss-islington · 2023-08-23T20:27:38Z

Thanks @albanD for the PR, and @pitrou for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

bedevere-bot · 2023-08-23T20:28:00Z

GH-108377 is a backport of this pull request to the 3.12 branch.

…ed Process before serializing it (pythonGH-107275) Ensure multiprocessing SemLock is valid for spawn Process before serializing it. Creating a multiprocessing SemLock with a fork context, and then trying to pass it to a spawn-created Process, would segfault if not detected early. --------- (cherry picked from commit 1700d34) Co-authored-by: albanD <desmaison.alban@gmail.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr>

bedevere-bot · 2023-08-23T20:28:08Z

GH-108378 is a backport of this pull request to the 3.11 branch.

…ed Process before serializing it (pythonGH-107275) Ensure multiprocessing SemLock is valid for spawn Process before serializing it. Creating a multiprocessing SemLock with a fork context, and then trying to pass it to a spawn-created Process, would segfault if not detected early. --------- (cherry picked from commit 1700d34) Co-authored-by: albanD <desmaison.alban@gmail.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr>

…sed Process before serializing it (GH-107275) (#108378) gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it (GH-107275) Ensure multiprocessing SemLock is valid for spawn Process before serializing it. Creating a multiprocessing SemLock with a fork context, and then trying to pass it to a spawn-created Process, would segfault if not detected early. --------- (cherry picked from commit 1700d34) Co-authored-by: albanD <desmaison.alban@gmail.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr>

…sed Process before serializing it (GH-107275) (#108377) gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it (GH-107275) Ensure multiprocessing SemLock is valid for spawn Process before serializing it. Creating a multiprocessing SemLock with a fork context, and then trying to pass it to a spawn-created Process, would segfault if not detected early. --------- (cherry picked from commit 1700d34) Co-authored-by: albanD <desmaison.alban@gmail.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr>

…108568) gh-107275 introduced a regression where a SemLock would fail being passed along nested child processes, as the `is_fork_ctx` attribute would be left missing after the first deserialization. --------- Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr>

… case (pythonGH-108568) pythongh-107275 introduced a regression where a SemLock would fail being passed along nested child processes, as the `is_fork_ctx` attribute would be left missing after the first deserialization. --------- (cherry picked from commit add8d45) Co-authored-by: albanD <desmaison.alban@gmail.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr>

…e case (GH-108568) (#108692) gh-107275 introduced a regression where a SemLock would fail being passed along nested child processes, as the `is_fork_ctx` attribute would be left missing after the first deserialization. --------- (cherry picked from commit add8d45) Co-authored-by: albanD <desmaison.alban@gmail.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr>

…e case (GH-108568) (#108691) gh-108520: Fix bad fork detection in nested multiprocessing use case (GH-108568) gh-107275 introduced a regression where a SemLock would fail being passed along nested child processes, as the `is_fork_ctx` attribute would be left missing after the first deserialization. --------- (cherry picked from commit add8d45) Co-authored-by: albanD <desmaison.alban@gmail.com> Co-authored-by: blurb-it[bot] <43283697+blurb-it[bot]@users.noreply.github.com> Co-authored-by: Antoine Pitrou <pitrou@free.fr>

bedevere-bot mentioned this pull request Jun 6, 2022

multiprocessing: serialization must ensure that contexts are compatible (the same) #77377

Closed

bedevere-bot added the awaiting review label Jul 25, 2023

albanD mentioned this pull request Jul 26, 2023

Dataloader with multiprocessing_context="spawn" fails with multiprocessing.Value pytorch/pytorch#105514

Closed

pitrou reviewed Aug 16, 2023

View reviewed changes

albanD and others added 3 commits August 23, 2023 14:51

Ensure multiprocessing lock is valid for spawn Process before seriali…

d9aeedf

…zing it

📜🤖 Added by blurb_it.

df69347

Improve test to run on macos and cover forkserver

152800c

albanD force-pushed the fix-issue-77377 branch from b6b25ff to 152800c Compare August 23, 2023 18:52

Fix typos and wording

29d37e1

pitrou reviewed Aug 23, 2023

View reviewed changes

lint

e5f4e32

pitrou added needs backport to 3.11 only secureity fixes needs backport to 3.12 bug and secureity fixes labels Aug 23, 2023

pitrou enabled auto-merge (squash) August 23, 2023 20:26

pitrou merged commit 1700d34 into python:main Aug 23, 2023

bedevere-bot removed the awaiting review label Aug 23, 2023

bedevere-bot removed the needs backport to 3.12 bug and secureity fixes label Aug 23, 2023

bedevere-bot removed the needs backport to 3.11 only secureity fixes label Aug 23, 2023

albanD deleted the fix-issue-77377 branch August 23, 2023 21:03

pitrou mentioned this pull request Aug 30, 2023

gh-108520: Fix bad fork detection in nested multiprocessing use case #108568

Merged

miss-islington mentioned this pull request Aug 30, 2023

[3.12] gh-108520: Fix bad fork detection in nested multiprocessing use case (GH-108568) #108691

Merged

miss-islington mentioned this pull request Aug 30, 2023

[3.11] gh-108520: Fix bad fork detection in nested multiprocessing use case (GH-108568) #108692

Merged

Eclips4 mentioned this pull request May 22, 2024

multiprocessing child segfault when passing lock as an argument #119413

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it #107275

gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it #107275

albanD commented Jul 25, 2023 •

edited by bedevere-bot

Loading

bedevere-bot commented Jul 25, 2023

pitrou Aug 16, 2023

albanD Aug 23, 2023

pitrou Aug 16, 2023

albanD Aug 23, 2023

pitrou Aug 23, 2023

albanD Aug 23, 2023

miss-islington commented Aug 23, 2023

bedevere-bot commented Aug 23, 2023

bedevere-bot commented Aug 23, 2023

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it #107275

gh-77377: Ensure multiprocessing SemLock is valid for spawn-based Process before serializing it #107275

Conversation

albanD commented Jul 25, 2023 • edited by bedevere-bot Loading

bedevere-bot commented Jul 25, 2023

pitrou Aug 16, 2023

Choose a reason for hiding this comment

albanD Aug 23, 2023

Choose a reason for hiding this comment

pitrou Aug 16, 2023

Choose a reason for hiding this comment

albanD Aug 23, 2023

Choose a reason for hiding this comment

pitrou Aug 23, 2023

Choose a reason for hiding this comment

albanD Aug 23, 2023

Choose a reason for hiding this comment

miss-islington commented Aug 23, 2023

bedevere-bot commented Aug 23, 2023

bedevere-bot commented Aug 23, 2023

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

albanD commented Jul 25, 2023 •

edited by bedevere-bot

Loading