Skip to content

Fix an invalid memory access in call to cuMemPoolGetAttribute #1272

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.

Sign up for GitHub

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

Closed
Andy-Jost wants to merge 1 commit into from
Closed

Fix an invalid memory access in call to cuMemPoolGetAttribute #1272

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cuda_core/cuda/core/experimental/_memory/_device_memory_resource.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
from __future__ import annotations

from libc.limits cimport ULLONG_MAX
from libc.stdint cimport uintptr_t
from libc.stdint cimport uintptr_t, uint64_t
from libc.string cimport memset

from cuda.bindings cimport cydriver
Expand Down Expand Up @@ -116,10 +116,10 @@ cdef DMRA_mempool_attribute(property_type: type):
return decorator


cdef int DMRA_getattribute(
cdef uint64_t DMRA_getattribute(
cydriver.CUmemoryPool pool_handle, cydriver.CUmemPool_attribute attr_enum
):
cdef int value
cdef uint64_t value = 0
Copy link
Collaborator

@rwgk rwgk Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ok, I believe it'll work.

But at a minimum there should be a comment/warning somewhere, explaining that

  • we know we're implicitly casting int* to cuuint64_t* for some attributes, and

  • we decided to not worry about endianness here because the ints we know about today are only converted to bool.

Better IMO would be to just do it right, so that any changes to the enums in the future will lead to obvious failures that are most likely easily fixed.

For this one case it may not matter much, but in aggregate it'll makes a huge difference in long-term reliability and maintainability of cuda-python as a whole.

It's really straightforward to achieve, although it's significantly more code:

This would give us complete safety. If there are changes to the API in the future, it will fail loudly and we'll know immediately that we need to review and adjust.

Copy link
Member

@leofang leofang Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of safety, I left a comment above (#1274) probably while you were leaving the review here 🙂

with nogil:
HANDLE_RETURN(cydriver.cuMemPoolGetAttribute(pool_handle, attr_enum, <void *> &value))
return value
Expand Down
Loading