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

Fix issues with AsyncBufRead::read_line and AsyncBufReadExt::lines #2884

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

hkratz
Copy link
Contributor

@hkratz hkratz commented Sep 14, 2024

Fixes the following issues in AsyncBufRead::read_line:

  • When the future is dropped the previous string contents are not restored so the string is empty.
  • If invalid UTF-8 is encountered the previous string contents are not restored.
  • If an IO error occurs after read_until_internal already read a couple of bytes a debug assertion fails.
  • Performance: The whole string to which read contents are appended is checked for UTF-8 validity instead of just the added bytes.

Fixes the following issues in AsyncBufReadExt:lines

Fixes #2862

@hkratz hkratz changed the title Fix issues with AsyncBufRead::read_line and AsyncBufReadExt::lines` Fix issues with AsyncBufRead::read_line and AsyncBufReadExt::lines Sep 14, 2024
@taiki-e taiki-e added A-io Area: futures::io 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Sep 14, 2024
Fixes the following issues in `AsyncBufRead::read_line`:
* When the future is dropped the previous string contents are not restored so the string is empty.
* If invalid UTF-8 is encountered the previous string contents are not restored.
* If an IO error occurs after `read_until_internal` already read a couple of bytes a debug assertion fails.
* Performance: The whole string to which read contents are appended is check for UTF-8 validity instead of just the added bytes.

* Fixes the following issues in `AsyncBufRead::read_line`
* If an IO error occurs after `read_until_internal` already read a couple of bytes a debug assertion fails. (rust-lang#2862)

Fixes rust-lang#2862.
@hkratz
Copy link
Contributor Author

hkratz commented Sep 14, 2024

The miri error is caused by memchr + MIRIFLAGS=-Zmiri-symbolic-alignment-check with longer byte slices.

It happens even with this test on master:

#[test]
fn read_until_eof_256() {
    let mut r = Cursor::new(vec![b'x'; 256]);
    let _ = block_on(r.read_until(b'\n', &mut Vec::new()));
}

Or when running this through miri with MIRIFLAGS=-Zmiri-symbolic-alignment-check:

fn main() {
    let v=vec![b'x'; 256];
    memchr::memchr(b'a', &v);
}

Apparently we have not run into it before because our tests have short slices. Not sure what we want to do about it either. Ignore read_until-related tests for Miri or use a shorter length for IOErrorRead here to placate miri?

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.

Thanks!

@taiki-e taiki-e merged commit 99ebe58 into rust-lang:master Sep 18, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. A-io Area: futures::io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lines::poll_next panics if the reader returns an error after returning data
2 participants