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.

Sign up for GitHub

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

Jump to bottom

io: fix panic in read_line #2541

Merged
merged 2 commits into from
May 24, 2020
Merged

io: fix panic in read_line #2541

merged 2 commits into from
May 24, 2020

Conversation

Darksonn
Copy link
Contributor

The changes in this PR are inspired by #2384. Note that the documentation specifies different behaviour on error for these methods than in the linked PR, namely that extra data should not be discarded on error.

Fixes: #2532

@Darksonn
io: fix panic in read_line
f893e2e 
The changes in this PR are inspired by tokio-rs#2384.

Fixes: tokio-rs#2532
@Darksonn Darksonn added C-maintenance Category: PRs that clean code up or issues documenting cleanup. A-tokio Area: The main tokio crate M-io Module: tokio/io labels May 16, 2020
@Darksonn Darksonn self-assigned this May 16, 2020
@Darksonn
Copy link
Contributor Author

Darksonn commented May 21, 2020 •
edited
Loading

I decided to rewrite read_to_end and read_to_string too.

The new implementation changes the behaviour such that set_len is called after poll_read. The motivation behind this change is that it makes it much more obvious that a rouge panic wont give the caller access to a vector containing exposed uninitialized memory. The new implementation also makes sure to not zero memory twice.

It does mean we have to be a bit careful when the vector is reallocated, but I think it is a win over the other approach. It is the order that set_len suggests in its examples, and Vec does also say that:

There is one case which we will not break, however: using unsafe code to write to the excess capacity, and then increasing the length to match, is always valid.

Additionally it makes the various implementations more consistent with each other regarding naming of variables, and whether we store how many bytes we have read, or how many were in the container originally.

Fixes: #2544

@Darksonn Darksonn mentioned this pull request May 21, 2020
Closed
@Darksonn Darksonn requested a review from taiki-e May 21, 2020 21:31
taiki-e
taiki-e reviewed May 24, 2020
View reviewed changes
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the changes on read_to_end/read_to_string yet, but the changes to read_line/read_until looks good to me.

@taiki-e
Copy link
Member

taiki-e commented May 24, 2020

@Darksonn Could you move changes on read_to_end/read_to_string to a separate PR?

  • These seem to be quite different kinds of bugs.
  • There is a lot of unsafe code and it seems that it needs to be reviewed more carefully. (I want reviews from others)

@Darksonn
Copy link
Contributor Author

Alright. I am sharing some code between them, so if you are generally happy with the changes in read_line/read_until, I will first remove them here, and create the PR once this has been merged.

@taiki-e
Copy link
Member

taiki-e commented May 24, 2020

@Darksonn Thanks, I'll approve this PR once the changes on read_to_end/read_to_string have been removed from this PR.

@Darksonn
Copy link
Contributor Author

Turns out I had already separated it in commits, so I could merely point it at an earlier commit.

@Darksonn
Copy link
Contributor Author

Oh hey github actions are running on it! Exciting.

@Darksonn Darksonn merged commit 954f2b7 into tokio-rs:master May 24, 2020
@Darksonn Darksonn deleted the read-line-panic branch May 25, 2020 08:42
jensim pushed a commit to jensim/tokio that referenced this pull request Jun 7, 2020
@Darksonn @jensim
io: fix panic in read_line (tokio-rs#2541)
671bad8 
Fixes: tokio-rs#2532
@gparker42 gparker42 mentioned this pull request Jun 8, 2020
Closed
hawkw added a commit that referenced this pull request Jul 20, 2020
@hawkw
chore: prepare to release 0.2.22
972c717 
- 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 hawkw mentioned this pull request Jul 20, 2020
Merged
hawkw added a commit that referenced this pull request Jul 22, 2020
@hawkw
chore: prepare to release 0.2.22 (#2672)
21f7260 
# 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
Reviewers

@taiki-e taiki-e taiki-e approved these changes

Assignees

@Darksonn Darksonn

Labels
A-tokio Area: The main tokio crate C-maintenance Category: PRs that clean code up or issues documenting cleanup. M-io Module: tokio/io
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Debug assertion triggered in read_line_internal when using BufReader with TcpStream.
2 participants
@Darksonn @taiki-e