Skip to content

Safety hazard: thread::park() doesn't guarantee being unwind-free but sync::Once implicitly relies on it being unwind-free #89571

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

Closed
Kixunil opened this issue Oct 5, 2021 · 1 comment
Labels
A-atomic Area: Atomics, barriers, and sync primitives I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@Kixunil
Copy link
Contributor

Kixunil commented Oct 5, 2021

I noticed Once contains this code:

        // We have enqueued ourselves, now lets wait.
        // It is important not to return before being signaled, otherwise we
        // would drop our `Waiter` node and leave a hole in the linked list
        // (and a dangling reference). Guard against spurious wakeups by
        // reparking ourselves until we are signaled.
        while !node.signaled.load(Ordering::Acquire) {
            // If the managing thread happens to signal and unpark us before we
            // can park ourselves, the result could be this thread never gets
            // unparked. Luckily `park` comes with the guarantee that if it got
            // an `unpark` just before on an unparked thread it does not park.
            thread::park();
        }

As the comment correctly explains the while loop must not end prematurely. However if thread::park() unwinds it would break the loop.

Does thread::park() panic though?
I found at least two instances: 1 2 where it explicitly could and I didn't review all implementations, nor internals of Mutex and CondVar. I understand those are supposed to be unlikely but I guess they were intended to reduce unsafety but amusingly they increased it for Once.

I think it'd be safer to either:

  • Guarantee that park can't unwind, change those panics to aborts and document it, ideally make this a public guarantee as well. (I noticed this issue because I'm writing something with structure similar to that piece in Once)
  • Explicitly document that this is not guaranteed with a warning about this footgun and change Once to abort if park panics.
@Kixunil Kixunil changed the title Safety hazard: thread::park() doesn't guarantee being unwind-free but sync::Once implicitly relies on it being panic-free Safety hazard: thread::park() doesn't guarantee being unwind-free but sync::Once implicitly relies on it being unwind-free Oct 5, 2021
@jyn514 jyn514 added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue. A-atomic Area: Atomics, barriers, and sync primitives labels Apr 10, 2023
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Apr 10, 2023
@m-ou-se
Copy link
Member

m-ou-se commented Apr 12, 2023

This seems to have been resolved in #102412

@jyn514 jyn514 closed this as completed Apr 12, 2023
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-atomic Area: Atomics, barriers, and sync primitives I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants