Skip to content

avoid mutable references in critical_section_mutex implementations#6050

Merged
ngoldbaum merged 1 commit into
PyO3:mainfrom
davidhewitt:mutex-refs
May 16, 2026
Merged

avoid mutable references in critical_section_mutex implementations#6050
ngoldbaum merged 1 commit into
PyO3:mainfrom
davidhewitt:mutex-refs

Conversation

@davidhewitt
Copy link
Copy Markdown
Member

There is a slight technicality in with_critical_section_mutex2 that if the same PyMutex is passed as both arguments, the &mut references to the raw ffi mutex internally are UB due to mutable aliasing.

Fortunately we can just work with raw pointers here as this is an FFI call. I'm not sure wether the compiler actually will produce exploitable UB in this particular position due to the references being immediately coerced to pointers for the FFI call, but it seems wise to tidy this up.

cc @ngoldbaum

(Credit to Codex security scanning for this discovery.)

@davidhewitt davidhewitt added the CI-skip-changelog Skip checking changelog entry label May 16, 2026
@@ -243,9 +243,9 @@ where
let mut guard = CS2Guard(unsafe { core::mem::zeroed() });
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

While we're here, I think we might be able to replace these core::mem::zeroed() with MaybeUninit of some form, to avoid needing to zero-intialize the structures at each critical section entry.

Looks like the C headers don't zero-initialize:

https://github.com/python/cpython/blob/a7ed0c9e1dee1397e80e2e0d90d110155da2bc30/Include/critical_section.h#L74-L90

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

(Possibly better to defer this to a separate PR given correctness / performance implications are not related to the exact diff here.)

@davidhewitt davidhewitt requested a review from ngoldbaum May 16, 2026 13:17
@davidhewitt davidhewitt mentioned this pull request May 16, 2026
@ngoldbaum
Copy link
Copy Markdown
Contributor

Thanks! There is another followup (pointed out to me by Claude when I asked it to review this), which is that the critical section API allows possible UB by passing the same mutex twice and calling get_mut and then get (so there is a simultaneous mutable and immutable reference). We should probably tighten the API to prevent that.

It's already unsafe of course but we should also prevent footguns or at least document them in the safety contract :)

@ngoldbaum ngoldbaum enabled auto-merge May 16, 2026 13:37
@ngoldbaum ngoldbaum added this pull request to the merge queue May 16, 2026
Merged via the queue into PyO3:main with commit 072d544 May 16, 2026
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI-skip-changelog Skip checking changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants