Repository navigation
fix(core): reject mismatched UUID in register() even under -O - #2704
Conversation
…#2697) DeviceMemoryResource.register() and PinnedMemoryResource.register() validated the supplied UUID against the resource's own with a bare `assert`, which CPython removes under `-O`. A mismatched UUID was then accepted, registered as a foreign registry key, and rewritten into the resource's own UUID, silently corrupting the IPC registry. Replace the assertion with an explicit ValueError so the check is enforced regardless of the interpreter's assertion mode, and convert the internal register() result check in MP_from_allocation_handle to an explicit RuntimeError as well. Signed-off-by: waterWang <waterWang@users.noreply.github.com>
github-actions
Bot
added
the
cuda.core
Andy-Jost
left a comment
Andy-Jost
left a comment
There was a problem hiding this comment.
Thank you, and sorry for the wait. The change is right. Three items before merge:
test_register_rejects_mismatched_uuidruns for every kind in theipc_memory_resourcefixture, which now includesVirtualMemoryResource(#3028, merged after this PR was opened). That class has noregister(), so the test errors there. Add the skip the neighbouring test uses:if not _is_pool(mr): pytest.skip("allocation handles and the registry are pool-only").- The internal-invariant message: the convention in
cuda_core/CLAUDE.mdisRuntimeError("internal cuda.core error, please report: ..."), so the two kinds of error read the same way everywhere. - A release-note entry in
cuda_core/docs/source/release/1.3.0-notes.rst, section "Fixes and enhancements", in the style of its neighbours, and aRaisessection on the tworegister()docstrings (ValueErrorfor a mismatched UUID;RuntimeErrorwhen the resource is not IPC-enabled).
The branch is 281 commits behind main; a merge of main is enough, no rebase needed. If you prefer, I can push the merge and these three items to your branch (edits by maintainers are enabled on the PR); say the word. CI has not run yet: outside contributions need a maintainer to approve the workflow runs, and GitHub queues them when the branch is updated. I will approve them as soon as they appear.
|
/ok to test e603e0e |
|
…(cuDSS 1.0 library names) Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
I merged |
|
/ok to test 4eecdbc |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Registration checks and coverage cuda_core/cuda/core/_memory/_ipc.pyx, cuda_core/tests/memory_ipc/test_errors.py, cuda_core/cuda/core/_memory/_device_memory_resource.pyi, cuda_core/cuda/core/_memory/_device_memory_resource.pyx, cuda_core/cuda/core/_memory/_pinned_memory_resource.pyi, cuda_core/cuda/core/_memory/_pinned_memory_resource.pyx, cuda_core/docs/source/release/1.3.0-notes.rst |
MP_register raises ValueError when a resource has a different non-None UUID. MP_from_allocation_handle raises RuntimeError if registration returns a different resource. The test checks rejection, UUID preservation, and registration under the resource’s own UUID. The API documentation and release notes describe the exception conditions. |
Assessment against linked issues
| Objective | Addressed | Explanation |
|---|---|---|
Reject registration under a mismatched UUID, including when Python runs with -O [2697] |
✅ |
Possibly related issues
- cuda.core: replace bare assert statements in library code with explicit errors #3202: The PR replaces the two assertions in
_memory/_ipc.pyx; the issue tracks other assertions separately.
Suggested reviewers: andy-jost
Change: Bug fix · Severity of issue fixed: Medium
Merge Risk: 
73639
-
Registering a resource under another resource’s existing UUID can still succeed instead of raising ValueError. Fix that path before merging.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments: 1
cuda_core/tests/memory_ipc/test_errors.py: 1
115-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Assert the exception type without matching message text.
MP_registerraisesValueErrorfor the UUID mismatch. The message wording is not part of the public contract, and no competingValueErrorexists on this exercised pool-resource path. Removematch="already has UUID".
ℹ️ Review info
⚙️ Run configuration
- Source mode: Auto
- Repository: NVIDIA/cuda-python
- Branch: main
- File: .coderabbit.yaml
- Commit: d5f37e0
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Advanced
- Run ID:
a53483fb-52b3-4e12-ae12-48ab90140384
📒 Files selected for processing: 2
cuda_core/cuda/core/_memory/_ipc.pyxcuda_core/tests/memory_ipc/test_errors.py
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| if not self.is_ipc_enabled: | ||
| raise RuntimeError("Memory resource is not IPC-enabled") | ||
| assert self.uuid is None or self.uuid == uuid | ||
| if self.uuid is not None and self.uuid != uuid: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
important: Check the resource UUID before the registry lookup. If a UUID already belongs to another resource, MP_register returns that resource at Lines 477–480. The new check never runs, so mr.register(other_uuid) accepts a UUID that differs from mr.uuid. Move the mismatch check before the existing-entry return.
|
|
||
|
|
||
| @pytest.mark.human_authored | ||
| def test_register_rejects_mismatched_uuid(ipc_memory_resource): |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
-
🏁 Script executed:
sed -n '1,140p' cuda_core/tests/memory_ipc/test_errors.py sed -n '425,475p' cuda_core/tests/conftest.py rg -n 'ipc_memory_resource|supports.*vmm|VirtualMemoryResource' cuda_core/tests/memory_ipc/conftest.py cuda_core/tests/conftest.py
Repository: NVIDIA/cuda-python
Length of output: 8888
🏁 Script executed:
sed -n '470,510p' cuda_core/tests/conftest.py sed -n '140,230p' cuda_core/cuda/core/_memory/_virtual_memory_resource.pyi nl -ba cuda_core/tests/memory_ipc/test_errors.py | sed -n '85,125p'
Repository: NVIDIA/cuda-python
Length of output: 7957
important: Restrict this test to pool resources.
When VMM IPC is supported, ipc_memory_resource also yields VirtualMemoryResource. That resource has no uuid or register API, so the test fails at mr.uuid before it checks UUID rejection. Limit this test to the device and pinned fixture parameters.
--- "a/cuda_core/tests/memory_ipc/test_errors.py"
+++ "b/cuda_core/tests/memory_ipc/test_errors.py"
@@ -94,8 +94,9 @@
DeviceMemoryResource.from_registry(key)
+@pytest.mark.parametrize("ipc_memory_resource", ["device", "pinned"], indirect=True)
@pytest.mark.human_authored
def test_register_rejects_mismatched_uuid(ipc_memory_resource):
"""register() rejects a UUID that does not match the resource's own.
The check is an explicit ValueError (not a bare ``assert``) so it remains
- test_register_rejects_mismatched_uuid skips non-pool resources: VirtualMemoryResource joined the ipc_memory_resource fixture and has no register(). - The internal-invariant error uses the "internal cuda.core error, please report" wording. - register() on DeviceMemoryResource and PinnedMemoryResource documents its RuntimeError and ValueError, and the 1.3.0 release notes record the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: NVIDIA/cuda-python
- Branch: main
- File: .coderabbit.yaml
- Commit: 8124abe
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Advanced
- Run ID:
a08a978a-5587-4189-a371-002287fb7bee
-
cuda_core/cuda/core/_memory/_device_memory_resource.pyicuda_core/cuda/core/_memory/_device_memory_resource.pyxcuda_core/cuda/core/_memory/_ipc.pyxcuda_core/cuda/core/_memory/_pinned_memory_resource.pyicuda_core/cuda/core/_memory/_pinned_memory_resource.pyxcuda_core/docs/source/release/1.3.0-notes.rstcuda_core/tests/memory_ipc/test_errors.py
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
-
|
/ok to test 620afec |
…register() A rejected registration that cleared the UUID to None would have passed the old check (uuid != other). CodeRabbit's suggestion. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/ok to test 736393d |
Andy-Jost
left a comment
Andy-Jost
left a comment
There was a problem hiding this comment.
Thanks, @waterWang. The review items and the CodeRabbit note are addressed, and CI is green. Approving.
Description
closes #2697
DeviceMemoryResource.register()andPinnedMemoryResource.register()validated the supplied UUID against the resource's own with a bare
assertinMP_register(cuda_core/cuda/core/_memory/_ipc.pyx).Since CPython strips assertions when run with
-O, the check silentlydisappears in optimized interpreters: the resource is registered under
a foreign key and its own UUID is rewritten to the attacker-
controlled value (
mr.uuidreports the new value from then on).Changes
_ipc.pyxMP_register: replaceassert self.uuid is None or self.uuid == uuidwith an explicitValueErrorwhen the suppliedUUID does not match the resource's own.
_ipc.pyxMP_from_allocation_handle: convert the internalassert registered is selfto an explicitRuntimeError, matchingthe maintainer direction to remove bare asserts from the library
(they are behavioral no-ops under
-O).cuda_core/tests/memory_ipc/test_errors.py: addtest_register_rejects_mismatched_uuidcovering the rejection andverifying the resource's UUID is not rewritten on failure.
Checklist