You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Handle std::bad_alloc in make_deallocation_stream / set_deallocation_stream - #3134
I have been working on adding clang-tidy to pre-commit for cuda-python. There is exactly one (potentially) real bug it found (the rest is just helping to make the code clearer to prevent /future/ bugs). So with that said, this is a theoretical bug, but something that has been observed in practice. We can decide this isn't worth the effort and just suppress the clang-tidy observation about it instead.
My agent's summary is below:
Fix: std::bad_alloc aborts the process in make_deallocation_stream / set_deallocation_stream
Branch: handle-memory-exception (based on upstream/main at e957c5c).
Package: cuda_core. Nothing here is committed.
Summary
make_deallocation_stream() and set_deallocation_stream() in the C++ handle
layer (cuda_core/cuda/core/_cpp/rt/) were declared noexcept, but both can
allocate host memory. When that allocation fails, std::bad_alloc escapes a noexcept function and the runtime calls std::terminate, which aborts the
whole Python process (SIGABRT). It should raise MemoryError.
The fix removes noexcept from both functions, declares the Cython binding of set_deallocation_stream as except+ (so std::bad_alloc becomes MemoryError), and handles the one caller that needs cleanup before the error
propagates. It is a robustness fix: it only matters when the host is out of
memory, but the failure mode (an unrecoverable abort) is exactly what the
project's own guidelines forbid.
How it was found
clang-tidy's bugprone-unhandled-exception-at-new flagged the bare new
in make_deallocation_stream:
stream.cpp: warning: missing exception handler for allocation failure at 'new'
[bugprone-unhandled-exception-at-new]
The warning alone does not say why it matters, and it points at only one of the
two throwing sites. The rest of the analysis below was done by reading the code.
Why it is a bug
cuda_core/CLAUDE.md ("Failure handling", "Never terminate the process") says:
An implicit abort (an exception escaping a noexcept function or a
deleter, including std::bad_alloc from an allocation inside noexcept
code) is a bug (#1489, #2417), not a policy choice: noexcept helpers must
not allocate, or must catch what they call.
make_deallocation_stream(h, out) noexcept (stream.cpp) binds a deallocation
stream to a stream handle. Real streams are copied unchanged, but a
default-stream token (nullptr, CU_STREAM_LEGACY, CU_STREAM_PER_THREAD)
with no embedded context takes this path:
get_current_context() is called. It is notnoexcept and allocates:
it goes through create_context_handle_ref, which builds a std::shared_ptr (a control-block allocation).
new StreamBox{...} and the std::shared_ptr that owns it allocate.
Either allocation can throw std::bad_alloc. Because the enclosing function is noexcept, the exception cannot propagate and std::terminate runs.
set_deallocation_stream(h, h_stream) noexcept (memory.cpp) calls it, so it
inherited the problem. It is the C++ side of the public Buffer.set_deallocation_stream(), and it is also called from Buffer.from_handle() whenever the memory resource is device accessible.
The other callers of make_deallocation_stream (deviceptr_alloc_from_pool, deviceptr_alloc_async, deviceptr_create_mapped_graphics, deviceptr_import_ipc) are not noexcept, and their Cython declarations are
already except+, so they were already safe once the callee may throw.
The fix
C++ (cuda_core/cuda/core/_cpp/rt/):
stream.cpp, internal.hpp: make_deallocation_stream is no longer noexcept. The comment documents that it may throw std::bad_alloc.
memory.cpp, api.hpp: set_deallocation_stream is no longer noexcept.
The header comment now says it may throw std::bad_alloc, matching how create_vmm_range documents the same thing.
Cython:
_rt.pyx, _rt.pxd: set_deallocation_stream is declared except+ nogil instead of noexcept nogil, like its sibling deviceptr_import_ipc. Cython converts std::bad_alloc into MemoryError.
_memory/_buffer.pyx:
_apply_deallocation_stream (used by Buffer.set_deallocation_stream)
is declared except *, so the MemoryError propagates with no change.
Buffer._init (used by Buffer.from_handle) already resets self._h_ptr
before raising on a failed status, because the DevicePtrHandle destructor
calls _mr_dealloc_callback, whose except Exception handler can clear an
in-flight error indicator. The new MemoryError path does the same reset,
inside an except MemoryError: handler and then re-raises, so the same
hazard cannot swallow the error.
Release note added to cuda_core/docs/source/release/1.3.0-notes.rst.
Alternatives considered
Catch std::bad_alloc in C++ and return CUDA_ERROR_OUT_OF_MEMORY. This
would leave the Cython signature untouched, but it reports host exhaustion as
a device error, and it hides the real exception type. MemoryError is the
accurate Python exception and is what the sibling functions already raise.
Keep noexcept and wrap the body in try/catch. This still needs some
channel to report the failure, so it ends up as one of the two options above.
Testing
New file cuda_core/tests/test_allocation_failure.py. A tiny shared library
(compiled with the system C++ compiler during the test) replaces operator new so that the Nth C++ allocation on the arming thread throws std::bad_alloc. It is preloaded with LD_PRELOAD into a child process, so
an abort shows up as a non-zero exit status instead of killing the test run.
Python allocates through its own allocators, so only C++ allocations are
affected. The tests are Linux-only and skip if no C++ compiler is available.
test_set_deallocation_stream_raises_memory_error_instead_of_terminating:
fails the first C++ allocation inside Buffer.set_deallocation_stream with a
default-stream token and asserts a clean MemoryError.
test_buffer_from_handle_survives_allocation_failure: fails each of the
first eight allocations on the way through Buffer.from_handle and asserts the
child exits normally and at least once saw a MemoryError.
Results (GPU run on this machine):
Build
test_allocation_failure.py
Without the fix
both fail: child exits with signal 6 and terminate called after throwing an instance of 'std::bad_alloc'
With the fix
both pass
I confirmed the "without" row by reverting only the source changes, rebuilding,
and running the new tests.
Wider run with the fix: pytest cuda_core/tests --ignore=cuda_core/tests/cython
gave 4081 passed, 201 skipped, 6 xfailed, 1 failed. The one failure, test_program_cache.py::test_filestream_write_over_cap_skips_vanished_files, is
unrelated and flaky: three consecutive runs of that single test gave pass, fail,
pass. The repo's pre-commit hooks pass on the changed files.
Limits and follow-ups
Only these two functions were fixed. A scan of every noexcept function body
under _cpp/rt/ for new, make_shared, containers and std::string found
no other direct allocation. Transitive cases (a noexcept function calling a
throwing function) are not caught by any stock clang-tidy check, so there may
be others. The fault-injection shim used here would find them if run across
the whole suite (fail the Nth allocation for many N).
Cases where a driver resource is created and a later host allocation
throws (a possible leak under memory exhaustion) were not audited. This fix
does not change them.
The tests depend on a Linux-style preload; other platforms skip them.
Buffer operations that bind a default stream now raise MemoryError if the host runs out of memory, instead of terminating the process.
If stream setup fails with a memory error during buffer initialization, the newly created device-pointer handle is reset before the error is raised.
The deallocation-stream functions can now propagate allocation exceptions through C++ and Cython. Buffer._init resets its device-pointer handle and re-raises MemoryError. The release notes describe the resulting error behavior.
Changes: Default-stream allocation errors
Layer / File(s)
Summary
C++ exception propagation and pointer cleanup cuda_core/cuda/core/_cpp/rt/api.hpp, cuda_core/cuda/core/_cpp/rt/internal.hpp, cuda_core/cuda/core/_cpp/rt/stream.cpp, cuda_core/cuda/core/_cpp/rt/memory.cpp
The deallocation-stream functions no longer declare noexcept. Binding a default-stream token may throw std::bad_alloc.
Python error propagation and validation cuda_core/cuda/core/_rt.pxd, cuda_core/cuda/core/_rt.pyx, cuda_core/cuda/core/_memory/_buffer.pyx, cuda_core/docs/source/release/1.3.0-notes.rst
Cython translates C++ exceptions. Buffer._init resets its device-pointer handle when MemoryError is raised. The release notes describe this outcome.
Change: Bug fix
Merge Risk: Moderate · up to 10f93
On host-memory exhaustion, CUDA resources can remain allocated or mapped after the new exception propagation. Add rollback handling before merging to prevent resource leaks and later availability problems.
- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
### CodeRabbit Verify
Run CodeRabbit Verify to test the changes in this pull request.
suggestion: Assert a failure inside default-stream binding.
The first Buffer.from_handle attempt can raise MemoryError while creating its pointer handle. That result alone satisfies this assertion, even if no attempt reaches the new cleanup in Buffer._init. Identify the allocation that fails during set_deallocation_stream, or instrument the shim to confirm that call was reached, and assert that outcome separately.
As per path instructions, check that “public behavior changes have corresponding tests.”
Reviewing files that changed from the base of the PR and between e957c5c and 52ef259.
📒 Files selected for processing: 9
cuda_core/cuda/core/_cpp/rt/api.hpp
cuda_core/cuda/core/_cpp/rt/internal.hpp
cuda_core/cuda/core/_cpp/rt/memory.cpp
cuda_core/cuda/core/_cpp/rt/stream.cpp
cuda_core/cuda/core/_memory/_buffer.pyx
cuda_core/cuda/core/_rt.pxd
cuda_core/cuda/core/_rt.pyx
cuda_core/docs/source/release/1.3.0-notes.rst
cuda_core/tests/test_allocation_failure.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
suggestion: Extend the allocation-failure harness to cover make_deviceptr_box.
test_allocation_failure.py does not call deviceptr_alloc_from_pool, deviceptr_alloc_async, or deviceptr_import_ipc. Add one focused scenario that forces new DevicePtrBox to throw after the CUDA allocation, then asserts MemoryError, a zero child exit status, and resource cleanup. This covers the shared cleanup branch without testing every allocation count.
Reviewing files that changed from the base of the PR and between 52ef259 and 8fb56cb.
📒 Files selected for processing: 1
cuda_core/cuda/core/_cpp/rt/memory.cpp
🚧 Files skipped from review as they are similar to previous changes: 1
cuda_core/cuda/core/_cpp/rt/memory.cpp
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
important: Unmap the graphics resource when handle creation throws.
cuGraphicsMapResources succeeds before make_deallocation_stream can throw std::bad_alloc. The unmap-owning handle is not constructed, and _mapped_buffer remains unset. GraphicsResource.close() therefore cannot unmap the resource. A later map() call can fail because the resource is still mapped.
Reviewing files that changed from the base of the PR and between 8fb56cb and c07e400.
📒 Files selected for processing: 1
cuda_core/tests/test_allocation_failure.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Please drop this helper and restore the four call sites (deviceptr_alloc_from_pool, deviceptr_alloc_async, and both branches of deviceptr_import_ipc) to their form on main.
No owning creator in _cpp/rt/ rolls back the driver resource when new Box throws after the driver call. create_stream_handle leaks the stream, and deviceptr_alloc_from_pool on main leaks the allocation. The one try/catch in a creation path, in graph_prepare_attachment, protects a host object, not a driver resource. The accepted outcome for a host allocation failure is a MemoryError and a leaked driver resource. The same call was made in #3028 (thread). A rollback here would be the first of more than twenty, one per owning creator.
The two new tests do not go through these call sites, so they are unaffected.
The reason will be displayed to describe this comment to others. Learn more.
Optional. My preference is to drop this file. It compiles a shim at test time, runs on Linux only, and guards one regression: a bare new inside a noexcept function. The clang-tidy check from your pre-commit work, bugprone-unhandled-exception-at-new, catches that case at lint time.
The reason will be displayed to describe this comment to others. Learn more.
Follow-up Comments
important: Roll back the graphics mapping when stream binding raises.
347-373: Major · Stability & Availability · Quick win
important: Roll back the graphics mapping when stream binding raises.
GraphicsResource.map maps the resource before deviceptr_create_mapped_graphics binds the deallocation stream. If make_deallocation_stream raises std::bad_alloc, the DevicePtrBox owner does not exist, so cuGraphicsUnmapResources is not called and the resource remains mapped.
Catch the helper failure, unmap with cy_stream, and attach any rollback failure while preserving the original exception. This is a fifth site in addition to the four allocation/import paths.
Reviewing files that changed from the base of the PR and between c07e400 and 10f93cd.
Files selected for processing: 7
cuda_core/cuda/core/_cpp/rt/api.hpp
cuda_core/cuda/core/_cpp/rt/internal.hpp
cuda_core/cuda/core/_cpp/rt/memory.cpp
cuda_core/cuda/core/_cpp/rt/stream.cpp
cuda_core/cuda/core/_rt.pxd
cuda_core/cuda/core/_rt.pyx
cuda_core/docs/source/release/1.3.0-notes.rst
Files skipped from review as they are similar to previous changes: 1
cuda_core/docs/source/release/1.3.0-notes.rst
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
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
bugSomething isn't workingcuda.coreEverything related to the cuda.core moduleP1Medium priority - Should do
2 participants
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Andy-Jost
left a comment
I have been working on adding
clang-tidytopre-commitforcuda-python. There is exactly one (potentially) real bug it found (the rest is just helping to make the code clearer to prevent /future/ bugs). So with that said, this is a theoretical bug, but something that has been observed in practice. We can decide this isn't worth the effort and just suppress theclang-tidyobservation about it instead.My agent's summary is below:
Fix:
std::bad_allocaborts the process inmake_deallocation_stream/set_deallocation_streamBranch:
handle-memory-exception(based onupstream/mainat e957c5c).Package:
cuda_core. Nothing here is committed.Summary
make_deallocation_stream()andset_deallocation_stream()in the C++ handlelayer (
cuda_core/cuda/core/_cpp/rt/) were declarednoexcept, but both canallocate host memory. When that allocation fails,
std::bad_allocescapes anoexceptfunction and the runtime callsstd::terminate, which aborts thewhole Python process (SIGABRT). It should raise
MemoryError.The fix removes
noexceptfrom both functions, declares the Cython binding ofset_deallocation_streamasexcept+(sostd::bad_allocbecomesMemoryError), and handles the one caller that needs cleanup before the errorpropagates. It is a robustness fix: it only matters when the host is out of
memory, but the failure mode (an unrecoverable abort) is exactly what the
project's own guidelines forbid.
How it was found
clang-tidy'sbugprone-unhandled-exception-at-newflagged the barenewin
make_deallocation_stream:The warning alone does not say why it matters, and it points at only one of the
two throwing sites. The rest of the analysis below was done by reading the code.
Why it is a bug
cuda_core/CLAUDE.md("Failure handling", "Never terminate the process") says:make_deallocation_stream(h, out) noexcept(stream.cpp) binds a deallocationstream to a stream handle. Real streams are copied unchanged, but a
default-stream token (
nullptr,CU_STREAM_LEGACY,CU_STREAM_PER_THREAD)with no embedded context takes this path:
get_current_context()is called. It is notnoexceptand allocates:it goes through
create_context_handle_ref, which builds astd::shared_ptr(a control-block allocation).new StreamBox{...}and thestd::shared_ptrthat owns it allocate.Either allocation can throw
std::bad_alloc. Because the enclosing function isnoexcept, the exception cannot propagate andstd::terminateruns.set_deallocation_stream(h, h_stream) noexcept(memory.cpp) calls it, so itinherited the problem. It is the C++ side of the public
Buffer.set_deallocation_stream(), and it is also called fromBuffer.from_handle()whenever the memory resource is device accessible.The other callers of
make_deallocation_stream(deviceptr_alloc_from_pool,deviceptr_alloc_async,deviceptr_create_mapped_graphics,deviceptr_import_ipc) are notnoexcept, and their Cython declarations arealready
except+, so they were already safe once the callee may throw.The fix
C++ (
cuda_core/cuda/core/_cpp/rt/):stream.cpp,internal.hpp:make_deallocation_streamis no longernoexcept. The comment documents that it may throwstd::bad_alloc.memory.cpp,api.hpp:set_deallocation_streamis no longernoexcept.The header comment now says it may throw
std::bad_alloc, matching howcreate_vmm_rangedocuments the same thing.Cython:
_rt.pyx,_rt.pxd:set_deallocation_streamis declaredexcept+ nogilinstead ofnoexcept nogil, like its siblingdeviceptr_import_ipc. Cython convertsstd::bad_allocintoMemoryError._memory/_buffer.pyx:_apply_deallocation_stream(used byBuffer.set_deallocation_stream)is declared
except *, so theMemoryErrorpropagates with no change.Buffer._init(used byBuffer.from_handle) already resetsself._h_ptrbefore raising on a failed status, because the
DevicePtrHandledestructorcalls
_mr_dealloc_callback, whoseexcept Exceptionhandler can clear anin-flight error indicator. The new
MemoryErrorpath does the same reset,inside an
except MemoryError:handler and then re-raises, so the samehazard cannot swallow the error.
Release note added to
cuda_core/docs/source/release/1.3.0-notes.rst.Alternatives considered
std::bad_allocin C++ and returnCUDA_ERROR_OUT_OF_MEMORY. Thiswould leave the Cython signature untouched, but it reports host exhaustion as
a device error, and it hides the real exception type.
MemoryErroris theaccurate Python exception and is what the sibling functions already raise.
noexceptand wrap the body intry/catch. This still needs somechannel to report the failure, so it ends up as one of the two options above.
Testing
New file
cuda_core/tests/test_allocation_failure.py. A tiny shared library(compiled with the system C++ compiler during the test) replaces
operator newso that the Nth C++ allocation on the arming thread throwsstd::bad_alloc. It is preloaded withLD_PRELOADinto a child process, soan abort shows up as a non-zero exit status instead of killing the test run.
Python allocates through its own allocators, so only C++ allocations are
affected. The tests are Linux-only and skip if no C++ compiler is available.
test_set_deallocation_stream_raises_memory_error_instead_of_terminating:fails the first C++ allocation inside
Buffer.set_deallocation_streamwith adefault-stream token and asserts a clean
MemoryError.test_buffer_from_handle_survives_allocation_failure: fails each of thefirst eight allocations on the way through
Buffer.from_handleand asserts thechild exits normally and at least once saw a
MemoryError.Results (GPU run on this machine):
test_allocation_failure.pyterminate called after throwing an instance of 'std::bad_alloc'I confirmed the "without" row by reverting only the source changes, rebuilding,
and running the new tests.
Wider run with the fix:
pytest cuda_core/tests --ignore=cuda_core/tests/cythongave 4081 passed, 201 skipped, 6 xfailed, 1 failed. The one failure,
test_program_cache.py::test_filestream_write_over_cap_skips_vanished_files, isunrelated and flaky: three consecutive runs of that single test gave pass, fail,
pass. The repo's pre-commit hooks pass on the changed files.
Limits and follow-ups
noexceptfunction bodyunder
_cpp/rt/fornew,make_shared, containers andstd::stringfoundno other direct allocation. Transitive cases (a
noexceptfunction calling athrowing function) are not caught by any stock clang-tidy check, so there may
be others. The fault-injection shim used here would find them if run across
the whole suite (fail the Nth allocation for many N).
throws (a possible leak under memory exhaustion) were not audited. This fix
does not change them.