Skip to content

soundness fix: retain PyBuffer while using the &[u8] that comes from it#89

Merged
oconnor663 merged 1 commit intomasterfrom
retain_pybuffer
Oct 14, 2025
Merged

soundness fix: retain PyBuffer while using the &[u8] that comes from it#89
oconnor663 merged 1 commit intomasterfrom
retain_pybuffer

Conversation

@oconnor663
Copy link
Owner

Previously we were getting a slice from a PyBuffer and then letting that PyBuffer drop. That released our "lock" on the buffer, which meant other threads could potentially race to resize it and turn our reads into use-after-free bugs. Refactor things so that the slice is lifetime-bound to the PyBuffer that protects it.

Also, previously I was calling PyBuffer::as_slice to take advantage of the internal contiguity check that it does, but that required disassembling and reassembling the raw slice to cast away ReadOnlyCell. Now I think it's cleaner to do the contiguity check explicitly and then work directly with the raw buffer pointer.

While cleaning up the comments around this, I realized I had previously misunderstood how Python::detach works. I thought it didn't allow certain lifetimes to pass into the closure (similar to std::thread::scope), but in fact it basically just requires Send. Casting away the PyBuffer::as_slice lifetime isn't really relevant; what matters is that &[u8] is Send while &[ReadOnlyCell<T>] isn't. That raised some questions about how all this interacts with freethreaded builds, where we never have a GIL protecting us. I've revised some of the big comments and also opened a discussion thread here: PyO3/pyo3#5508

Previously we were getting a slice from a `PyBuffer` and then letting
that `PyBuffer` drop. That released our "lock" on the buffer, which
meant other threads could potentially race to resize it and turn our
reads into use-after-free bugs. Refactor things so that the slice is
lifetime-bound to the `PyBuffer` that protects it.

Also, previously I was calling `PyBuffer::as_slice` to take advantage of
the internal contiguity check that it does, but that required
disassembling and reassembling the raw slice to cast away
`ReadOnlyCell`. Now I think it's cleaner to do the contiguity check
explicitly and then work directly with the raw buffer pointer.

While cleaning up the comments around this, I realized I had previously
misunderstood how `Python::detach` works. I thought it didn't allow
certain lifetimes to pass into the closure (similar to
`std::thread::scope`), but in fact it basically just requires `Send`.
Casting away the `PyBuffer::as_slice` lifetime isn't really relevant;
what matters is that `&[u8]` is `Send` while `&[ReadOnlyCell<T>]` isn't.
That raised some questions about how all this interacts with
freethreaded builds, where we never have a GIL protecting us. I've
revised some of the big comments and also opened a discussion thread
here: PyO3/pyo3#5508
@oconnor663 oconnor663 requested a review from ddelange October 13, 2025 20:58
@oconnor663 oconnor663 mentioned this pull request Oct 13, 2025
Copy link
Collaborator

@ddelange ddelange left a comment

Choose a reason for hiding this comment

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

I love the deep dive, and the PR description is next level. LGTM 👍

@ddelange
Copy link
Collaborator

this reminds me of ABI3 (limited API). I think we can't use it here because of the low level buffer stuff, but AB3 wheels would be awesome of course because they're forward compatible (future python minor version bumps, or even major version bumps?)

@oconnor663 oconnor663 merged commit 450362d into master Oct 14, 2025
156 checks passed
@oconnor663 oconnor663 deleted the retain_pybuffer branch October 14, 2025 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants