Skip to content

with_critical_section_mutex improvements #6051

Description

@ngoldbaum

Over in #6050 we fixed an issue with possible UB using with_critical_section_mutex2. However, there are additional issues:

[The] 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).

I think we can fix this by changing the API of EnteredCriticalSection to be a new EnteredCriticalSectionUnsafe struct:

#[cfg(all(Py_3_14, not(Py_LIMITED_API)))]
pub struct EnteredCriticalSectionUnsafe<'a, T>(&'a UnsafeCell<T>);

#[cfg(all(Py_3_14, not(Py_LIMITED_API)))]
impl<T> EnteredCriticalSectionUnsafe<'_, T> {
    pub unsafe fn get(&self) -> *const UnsafeCell {...}
}

That way if anyone needs unsafe aliased access to the data, they have to explicitly do it via the UnsafeCell API.

I think this is only necessary for the _mutex variants and we shouldn't need to change the API of with_critical_section or with_critical_section2, since those don't allow locking arbitrary data.

Credit to @epilys for suggesting this to me on IRC.

Also as pointed out on the PR review by @davidhewitt, we can also probably use MaybeUninit instead of std::mem::zeroed() when initializaing CsGuard and Cs2Gaurd internally:

let mut guard = CSGuard(unsafe { core::mem::zeroed() });

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions