Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutex: Only track a single locked flag #3665

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bugadani
Copy link
Contributor

No description provided.


fn lock(&self, waker: &core::task::Waker) -> bool {
if self.locked.replace(true) {
unsafe { (&mut *self.waker.get()).register(waker) };
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is sound. register() can drop or wake wakers, which will call waker methods through the vtable which is basically running arbitrary code. This code could call back into the same Mutex, causing two &muts to the same WakerRegistration to exist at the same time, which is UB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we add a !Sync version of WakerRegistration, that would only need &self to register/wake?

Copy link
Member

Choose a reason for hiding this comment

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

it might be possible if it uses Cell<Option<Waker>>, but it'd mean you'd have to constantly take out and put back the waker so it might be less efficient, not sure.

Copy link
Contributor Author

@bugadani bugadani Dec 22, 2024

Choose a reason for hiding this comment

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

We can look into it through a pointer unsafely I think.

Copy link
Member

@Dirbaio Dirbaio Dec 22, 2024

Choose a reason for hiding this comment

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

I don't think so. for example to wake the waker you'd need a &Waker pointing to the contents. The wake can run arbitrary code which can reentrantly call into the same WakerRegistration and replace the waker (with either &mut or ptr::write), which would be UB because you'd be mutating the data while a &Waker pointing to that location exists.

The only way is to really ensure you never get any & or &mut pointing to the contents, which is what Cell does basically.

Copy link
Contributor Author

@bugadani bugadani Dec 22, 2024

Choose a reason for hiding this comment

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

Uh, I might be dense here to not understand, but given the following impl, how can a reference exist while we're mutating the cell?

fn register(&self, w: &Waker) {
        match self.inner_as_ref() { // may need to be unsafe but that's beside the question
            Some(ref w2) if (w2.will_wake(w)) => return,
            _ => {}
        }

        // clone the new waker and store it
        if let Some(old_waker) = self.waker.replace(Some(w.clone())) {
            old_waker.wake() // this may call reentrantly into `register`
        }
    }

@bugadani bugadani marked this pull request as draft December 22, 2024 23:09
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