Harden program-cache permissions, binary-path resolution, and coredump helper lifetime#2399
Open
rparolin wants to merge 10 commits into
Open
Harden program-cache permissions, binary-path resolution, and coredump helper lifetime#2399rparolin wants to merge 10 commits into
rparolin wants to merge 10 commits into
Conversation
…er lifetime - cuda.core: create the on-disk program cache tree owner-only (0o700) and re-assert restrictive perms on POSIX, so cached device code cannot be read or planted by other local users regardless of the inherited umask. - cuda.pathfinder: absolutize the resolved binary-utility path to honor the documented absolute-path contract; surface AddDllDirectory failures on Windows with GetLastError instead of silently swallowing them. - cuda.bindings: retain the caller's bytes in _HelperCUcoredumpSettings so the borrowed pointer cannot outlive its backing buffer, and free the getter's 1 KiB buffer in __dealloc__; clarify get_buffer_pointer's lifetime contract. Adds regression tests for the cache permissions, path absolutization, and coredump helper lifetime. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rparolin
added
bug
Contributor
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
rparolin
marked this pull request as ready for review
July 21, 2026 20:41
Shorten the explanatory comments to a line or two each and drop the internal issue-number references (which don't resolve in this repo). No code changes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
It didn't actually guard the two lifetime fixes: the driver copies the path during the set call (so the NVIDIA#379 latent UAF can't trigger), and a missing free leaks silently (so NVIDIA#381 wouldn't fail the test either). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The test pre-creates a 0o777 cache dir to verify the loader tightens it to 0o700; the permissive mask is the point of the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The abspath change makes find_nvidia_binary_utility return a drive-qualified path on Windows (C:\... ), so the three search-order tests must compare against os.path.abspath(expected). No-op on Linux; fixes the Windows CI failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ting The c_int/c_byte param feeders silently narrowed an out-of-range Python int (e.g. 2**32+5 -> 5). Range-check in the feeder and raise OverflowError; declare feed() as 'except? -1' so Cython propagates the exception instead of swallowing it. Addresses Glasswing V9.1 (leofang chose the raise option over matching ctypes' silent wrap). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per Leo's review: on 3.13+ PyLong_AsInt converts and range-checks in one call (raising OverflowError itself), so gate the manual INT_MIN/INT_MAX check to pre-3.13 only. c_byte keeps the manual check (no 8-bit CPython equivalent). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…flow Per Leo's follow-up: use a single code path on all supported Pythons instead of version-gating PyLong_AsInt. PyLong_AsLongAndOverflow flags out-of-long values via its overflow out-param (no exception set), then we bounds-check the 32-bit (c_int) / 8-bit (c_byte) target range and raise OverflowError. Drops the PY_VERSION_HEX gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…#2402) The out-of-range c_int/c_byte -> OverflowError change is a distinct behavior change; moved to a standalone PR so it can be reviewed separately from this hardening batch. No functional change to the remaining hardening work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
leofang
reviewed
Jul 22, 2026
| use. The buffer export is released before this returns, so resizing a mutable | ||
| buffer (e.g. ``bytearray``) during a ``nogil`` call is a use-after-free. | ||
| Prefer immutable ``bytes``. | ||
| """ |
Member
There was a problem hiding this comment.
This code is part of cybind (which @mdboom is improving). Let's not touch it.
leofang
reviewed
Jul 22, 2026
Comment on lines
+400
to
+409
| # Cache holds compiled device code loaded via cuLibraryLoadData, so keep | ||
| # it owner-only (0o700) so other local users can't read or replace it. | ||
| # mkdir's mode is umask-masked and exist_ok keeps an existing dir's | ||
| # perms, so re-chmod on POSIX. | ||
| self._root.mkdir(parents=True, exist_ok=True, mode=0o700) | ||
| self._entries.mkdir(exist_ok=True, mode=0o700) | ||
| self._tmp.mkdir(exist_ok=True, mode=0o700) | ||
| if os.name != "nt": | ||
| for d in (self._root, self._entries, self._tmp): | ||
| os.chmod(d, 0o700) |
Member
There was a problem hiding this comment.
I am not sure this is acceptable. One can imagine in a compute cluster a group of users want to share the same kernel cache. If the directory already exists, we should use it as-is without changing the permission.
Member
There was a problem hiding this comment.
self._tmp can certainly be made owner-only.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Small safety fixes across three areas, each with a test. Mostly hardening; one intentional behavior change is called out below.
What changed
cuda.core
0o700) so other users on the machine can't read or tamper with it.cuda.pathfinder
find_nvidia_binary_utilitynow always returns a full absolute path, as the docs promise (it could return a relative one before).AddDllDirectoryfails, we now warn instead of failing silently.cuda.bindings
get_buffer_pointer's docs: don't resize the buffer while its pointer is in use.c_int/c_bytekernel argument now raisesOverflowErrorinstead of being silently truncated (e.g.2**32+5used to become5). This is stricter thanctypes' own silent wrap — flagged for reviewer sign-off.Tests
Added tests for the folder permissions, the absolute path, the coredump helper, and the out-of-range kernel argument. All pass locally (RTX 5880).
One thing to confirm
The folder-permission fix uses
chmod. If the cache folder is owned by someone else (a shared cache), construction will now raise an error instead of silently continuing. That's intentional — shared caches aren't safe — but please confirm it's okay. Easy to soften if you'd rather it tolerate that case.Reviewers: @leofang (core/bindings), @rwgk (pathfinder).