Remove uses of Pin::new_unchecked in h1 Dispatcher #1374
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This removes the last uses of unsafe `Pin` functions in actix-web. This PR adds a `Pin<Box<_>>` wrapper to `DispatcherState::Upgrade`, `State::ExpectCall`, and `State::ServiceCall`. The previous uses of the futures `State::ExpectCall` and `State::ServiceCall` were Undefined Behavior - a future was obtained from `self.expect.call` or `self.service.call`, pinned on the stack, and then immediately returned from `handle_request`. The only alternative to using `Box::pin` would be to refactor `handle_request` to write the futures directly into their final location, or avoid polling them before they are returned. The previous use of `DispatcherState::Upgrade` doesn't seem to be unsound. However, having data pinned inside an enum that we `std::mem::replace` would require some careful `unsafe` code to ensure that we never call `std::mem::replace` when the active variant contains pinned data. By using `Box::pin`, we any possibility of future refactoring accidentally introducing undefined behavior.
|
The test failures seem unrelated. |
|
Yeah, will be fixed by #1372. |
JohnTitor
approved these changes
Feb 25, 2020
|
LGTM! |
|
Thanks! |
Aaron1011
added a commit
to Aaron1011/actix-web
that referenced
this pull request
Mar 4, 2020
This ended up getting reverted by actix#1367, which re-introduced an unsound use of `Pin::new_unchecked` See my original PR actix#1374 for the reasoning behind this change.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes the last uses of unsafe
Pinfunctions in actix-web.This PR adds a
Pin<Box<_>>wrapper toDispatcherState::Upgrade,State::ExpectCall, andState::ServiceCall.The previous uses of the futures
State::ExpectCallandState::ServiceCallwere Undefined Behavior - a future was obtained from
self.expect.callor
self.service.call, pinned on the stack, and then immediatelyreturned from
handle_request. The only alternative to usingBox::pinwould be to refactor
handle_requestto write the futures directly intotheir final location, or avoid polling them before they are returned.
The previous use of
DispatcherState::Upgradedoesn't seem to beunsound. However, having data pinned inside an enum that we
std::mem::replacewould require some carefulunsafecode to ensurethat we never call
std::mem::replacewhen the active variant containspinned data. By using
Box::pin, we any possibility of futurerefactoring accidentally introducing undefined behavior.