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

io: fix unsound pin projection in read_buf and write_buf #2612

Merged
merged 1 commit into from
Jun 12, 2020
Merged

io: fix unsound pin projection in read_buf and write_buf #2612

merged 1 commit into from
Jun 12, 2020

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Jun 12, 2020

Motivation

// safety: no data is moved from self
unsafe {
let me = self.get_unchecked_mut();
Pin::new_unchecked(&mut *me.writer).poll_write_buf(cx, &mut me.buf)
}

This is a Pin<&mut Type> to Pin<Field> projection and is unsound if W is not Unpin (you can move W after WriteBuf dropped):
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c4740885e4bdb8169e6e5e335fca72d6

The correct projection is Pin<&mut Type> to Pin<&mut Field>. In WriteBuf, it is Pin<&mut WriteBuf<'_, W, B>> to Pin<&mut &mut W>, and it needs to add W: Unpin bounds to convert Pin<&mut &mut W> to Pin<&mut W>.

Solution

Add Self: Unpin bounds to AsyncReadExt::read_buf and AsyncWriteExt::write_buf.
This changes the public API, but I don't think there is any other way to fix this.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Yay Pin unsafety is fun.

@taiki-e taiki-e merged commit 1769f65 into tokio-rs:master Jun 12, 2020
@taiki-e taiki-e deleted the fix-pin branch June 12, 2020 05:28
@Darksonn Darksonn added the I-unsound 💥 A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label Jun 12, 2020
@Darksonn
Copy link
Contributor

Add Self: Unpin bounds to AsyncReadExt::read_buf and AsyncWriteExt::write_buf.
This changes the public API, but I don't think there is any other way to fix this.

Breaking changes are allowed for bugs. In principle it would be enough to add the bound on the Future impl, but I don't think that would be helpful.

hawkw added a commit that referenced this pull request Jul 20, 2020
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621,
  #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567,
  #2520, #2521, #2572, #2493)
- rt: allow calls to `block_on` inside calls to `block_in_place` that are
  themselves inside `block_on` (#2645)
- net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597)
- io: improve stack usage by allocating large buffers on directly on the heap
  (#2634)
- io: fix unsound pin projection in `AsyncReadExt::read_buf` and
  `AsyncWriteExt::write_buf` (#2612)
- io: fix unnecessary zeroing for `AsyncRead` implementors (#2525)
- io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654)

- coop: returning `Poll::Pending` no longer decrements the task budget (#2549)

- io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods
  (#1915)
- io: fix panic in `AsyncReadExt::read_line` (#2541)
- task: add [`tracing`] instrumentation to spawned tasks (#2655)
- sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors)
  (#2615)
- net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604)
- fs: add `OpenOptionsExt` for `OpenOptions` (#2515)
- fs: add `DirBuilder` (#2524)

[`tracing`]: https://crates.io/crates/tracing

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jul 22, 2020
# 0.2.22 (July 2!, 2020)

### Fixes
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621,
  #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567,
  #2520, #2521, #2493)
- rt: allow calls to `block_on` inside calls to `block_in_place` that are
  themselves inside `block_on` (#2645)
- net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597)
- io: improve stack usage by allocating large buffers on directly on the heap
  (#2634)
- io: fix unsound pin projection in `AsyncReadExt::read_buf` and
  `AsyncWriteExt::write_buf` (#2612)
- io: fix unnecessary zeroing for `AsyncRead` implementors (#2525)
- io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654)
- io: fix panic in `AsyncReadExt::read_line` (#2541)

### Changes
- coop: returning `Poll::Pending` no longer decrements the task budget (#2549)

### Added
- io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods
  (#1915)
- task: add [`tracing`] instrumentation to spawned tasks (#2655)
- sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors)
  (#2615)
- net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604)
- fs: add `OpenOptionsExt` for `OpenOptions` (#2515)
- fs: add `DirBuilder` (#2524)

[`tracing`]: https://crates.io/crates/tracing

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate I-unsound 💥 A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness M-io Module: tokio/io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants