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.
* cuda.core: replace library asserts with explicit errors
python -O strips assert statements, and Cython drops them too, so the
checks they carried vanished in optimized runs. Values that come from a
caller, a DLPack or CUDA Array Interface producer, or an NVML payload
now raise ValueError or TypeError naming the value. Internal invariants
raise RuntimeError("internal cuda.core error, please report: ...").
The unmatched-capsule branch in view_as_dlpack was an assert False, but
a producer reaches it by returning anything other than an unconsumed
DLPack capsule, so it raises ValueError rather than the internal error.
Enforce ruff S101 for cuda_core/cuda/core only. Enabling it for
everything outside tests would flag about 180 asserts in pathfinder,
the samples and the shared test helpers.
The two asserts in _memory/_ipc.pyx are handled by #2704.
* cuda.core: replace the unsqueeze_extents assert, add release note
unsqueeze_extents in _layout.pyx still checked an internal invariant
with assert, which python -O strips. It now raises the "internal
cuda.core error, please report" RuntimeError.
Add a 1.3.0 release note that lists the entry points whose failed
checks now raise ValueError or TypeError instead of AssertionError.
The managed memory allocate test now closes the resource as well as
the graph builder.
* cuda_bindings_12: keep S101 ignored for the CUDA 12 sources
cuda_bindings_12/pyproject.toml has its own [tool.ruff] section, so
ruff lints that tree with it instead of the root ruff.toml, and the
root per-file-ignores never reach it. The Bandit job passes
--select S with only the S codes from the root ignore list, so
moving S101 out of that list exposed 911 asserts in
cuda_bindings_12. Ignore S101 for the whole tree in its own config.
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