Fix issue #1898 - Permit::release() must be called when Mutex::lock() is aborted even if it is in the Waiting state

[bikeshedder]

It is possible to leave a mutex in a locked state even if no future exists holding the lock.

• create Mutex
• lock Mutex
• try to lock mutex in another future that is aborted (e.g. via timeout)
• unlock Mutex
• locking the mutex again fails

I think this should be considered a critical issue as it can be exploited as shown in issue #1898 when using hyper and aborting requests.

[bikeshedder]

Permit::release must be called regardless if it's in the state Acquired or Waiting. If a Waiting Permit is not released its waiter will stay in the queue blocking any future Permit to be acquired.

The original code did the following:

• release semaphore if it is aquired
• panic via unreachable! if the semaphore is idle or waiting unless the code is already panicking

Since a Permit must always be released unless it is in the Idle state I wonder if that unreachable! is even needed. If needed the following code should be added at the beginning of MutexGuard::drop:

if self.permit.state == semaphore::PermitState::Idle && !::std::thread::panicking() {
    unreachable!("Permit was idle when MutexGuard was dropped")
}

[hawkw]

This looks good to me, but it would be good to hear from @carllerche as well. I commented on some minor nits.

[hawkw]

The use of the interval here is a little un-obvious, it could be good if there was a comment explaining what it's doing here?

[bikeshedder]

I thought that the comment Try to lock mutex in a future that is aborted prematurely was telling enough. tbh. That test case is not really needed. I just added it when debugging the code and trying to reproduce the locking behaviour. This test just makes sure Aquired locks are returned when their future is aborted.

Is there a better way to let a future delay its execution than creating an interval and waiting twice? tick().await is called twice as the first call of it returns instantly.

[hawkw]

Is there a better way to let a future delay its execution than creating an interval and waiting twice? tick().await is called twice as the first call of it returns instantly.

You could possibly just use

task::yield_now().await;

if all you want is to yield to the scheduler so that another task will be polled? But, what you're doing now does make sense, I just thought it would be helpful if there was a comment explaining what it was for.

[carllerche]

I'm going to move forward merging & shipping. We can tweak the test in a follow up PR.

[hawkw]

Nit: It would be good to reference the GitHub issue that these tests reproduce.

[carllerche]

Thanks for debugging & fixing 👍