-
-
Notifications
You must be signed in to change notification settings - Fork 33.5k
gh-98896: Fix parsing issue in resource_tracker to allow shared memory names containing colons #138473
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
gh-98896: Fix parsing issue in resource_tracker to allow shared memory names containing colons #138473
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
ac9555d
gh-98896: Fix parsing of registered or unregistered resources
rani-pinchuk f3c8138
Add test for shared memory names that contain colons
rani-pinchuk d973cf0
Fix the test
rani-pinchuk e5f9549
📜🤖 Added by blurb_it.
blurb-it[bot] 9ac85d8
Address also newlines in shared_memory names by encoding and decoding…
rani-pinchuk 84813da
Sending the message as JSON and encode the name using base64 to keep …
rani-pinchuk ea6416e
Fixes according to review comments
rani-pinchuk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -7335,3 +7335,46 @@ def test_forkpty(self): | |||||||
| res = assert_python_failure("-c", code, PYTHONWARNINGS='error') | ||||||||
| self.assertIn(b'DeprecationWarning', res.err) | ||||||||
| self.assertIn(b'is multi-threaded, use of forkpty() may lead to deadlocks in the child', res.err) | ||||||||
|
|
||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This should have been a double line gap. I'll fix it in the backports. |
||||||||
| @unittest.skipUnless(HAS_SHMEM, "requires multiprocessing.shared_memory") | ||||||||
| class TestSharedMemoryNames(unittest.TestCase): | ||||||||
| def test_that_shared_memory_name_with_colons_has_no_resource_tracker_errors(self): | ||||||||
| # Test script that creates and cleans up shared memory with colon in name | ||||||||
| test_script = textwrap.dedent(""" | ||||||||
| import sys | ||||||||
| from multiprocessing import shared_memory | ||||||||
| import time | ||||||||
|
|
||||||||
| # Test various patterns of colons in names | ||||||||
| test_names = [ | ||||||||
| "a:b", | ||||||||
| "a:b:c", | ||||||||
| "test:name:with:many:colons", | ||||||||
| ":starts:with:colon", | ||||||||
| "ends:with:colon:", | ||||||||
| "::double::colons::", | ||||||||
| "name\\nwithnewline", | ||||||||
| "name-with-trailing-newline\\n", | ||||||||
| "\\nname-starts-with-newline", | ||||||||
| "colons:and\\nnewlines:mix", | ||||||||
| "multi\\nline\\nname", | ||||||||
| ] | ||||||||
|
|
||||||||
| for name in test_names: | ||||||||
| try: | ||||||||
| shm = shared_memory.SharedMemory(create=True, size=100, name=name) | ||||||||
| shm.buf[:5] = b'hello' # Write something to the shared memory | ||||||||
| shm.close() | ||||||||
| shm.unlink() | ||||||||
|
|
||||||||
| except Exception as e: | ||||||||
| print(f"Error with name '{name}': {e}", file=sys.stderr) | ||||||||
| sys.exit(1) | ||||||||
|
|
||||||||
| print("SUCCESS") | ||||||||
| """) | ||||||||
|
|
||||||||
| rc, out, err = assert_python_ok("-c", test_script) | ||||||||
| self.assertIn(b"SUCCESS", out) | ||||||||
| self.assertNotIn(b"traceback", err.lower(), err) | ||||||||
| self.assertNotIn(b"resource_tracker.py", err, err) | ||||||||
2 changes: 2 additions & 0 deletions
2
Misc/NEWS.d/next/Library/2025-09-03-20-18-39.gh-issue-98896.tjez89.rst
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| Fix a failure in multiprocessing resource_tracker when SharedMemory names contain colons. | ||
| Patch by Rani Pinchuk. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decoding is OK with strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comment. I like your suggestion to consider using JSON to encode the message.
Also, in the original implementation there was a check that the message is not longer than 512 bytes, since writes shorter than
PIPE_BUF(512 bytes on POSIX) are guaranteed to be atomic. But there was no check on the name length.Now I check that the name is at most 255 bytes long, which is the value of
NAME_MAXon Linux (including the leading slash that POSIX requires in shared memory and semaphore names).I still encode the name using Base64, because
json.dumps(..., ensure_ascii=True)would otherwise expand each non-ASCII byte into a 6-character escape like\uDC80. Using Base64 ensures that a 255-byte name becomes at most 340 bytes long, so the total JSON message always remains well below 512 bytes.As a result, the previous runtime check for the message length is now replaced by an
assert.What do you think?