Skip to content

Commit

Permalink
Fix issues with AsyncBufRead::read_line and AsyncBufReadExt::lines (
Browse files Browse the repository at this point in the history
#2884)

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. (#2862)

Fixes #2862
  • Loading branch information
hkratz authored Sep 18, 2024
1 parent b92f4c5 commit 99ebe58
Show file tree
Hide file tree
Showing 5 changed files with 102 additions and 18 deletions.
1 change: 1 addition & 0 deletions futures-util/src/io/lines.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ impl<R: AsyncBufRead> Stream for Lines<R> {
fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
let this = self.project();
let n = ready!(read_line_internal(this.reader, cx, this.buf, this.bytes, this.read))?;
*this.read = 0;
if n == 0 && this.buf.is_empty() {
return Poll::Ready(None);
}
Expand Down
47 changes: 32 additions & 15 deletions futures-util/src/io/read_line.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,14 @@ pub struct ReadLine<'a, R: ?Sized> {
buf: &'a mut String,
bytes: Vec<u8>,
read: usize,
finished: bool,
}

impl<R: ?Sized + Unpin> Unpin for ReadLine<'_, R> {}

impl<'a, R: AsyncBufRead + ?Sized + Unpin> ReadLine<'a, R> {
pub(super) fn new(reader: &'a mut R, buf: &'a mut String) -> Self {
Self { reader, bytes: mem::take(buf).into_bytes(), buf, read: 0 }
Self { reader, bytes: mem::take(buf).into_bytes(), buf, read: 0, finished: false }
}
}

Expand All @@ -35,26 +36,42 @@ pub(super) fn read_line_internal<R: AsyncBufRead + ?Sized>(
bytes: &mut Vec<u8>,
read: &mut usize,
) -> Poll<io::Result<usize>> {
let ret = ready!(read_until_internal(reader, cx, b'\n', bytes, read));
if str::from_utf8(bytes).is_err() {
bytes.clear();
Poll::Ready(ret.and_then(|_| {
Err(io::Error::new(io::ErrorKind::InvalidData, "stream did not contain valid UTF-8"))
}))
} else {
debug_assert!(buf.is_empty());
debug_assert_eq!(*read, 0);
// Safety: `bytes` is a valid UTF-8 because `str::from_utf8` returned `Ok`.
mem::swap(unsafe { buf.as_mut_vec() }, bytes);
Poll::Ready(ret)
let mut ret = ready!(read_until_internal(reader, cx, b'\n', bytes, read));
if str::from_utf8(&bytes[bytes.len() - *read..bytes.len()]).is_err() {
bytes.truncate(bytes.len() - *read);
if ret.is_ok() {
ret = Err(io::Error::new(
io::ErrorKind::InvalidData,
"stream did not contain valid UTF-8",
));
}
}
*read = 0;
// Safety: `bytes` is valid UTF-8 because it was taken from a String
// and the newly read bytes are either valid UTF-8 or have been removed.
mem::swap(unsafe { buf.as_mut_vec() }, bytes);
Poll::Ready(ret)
}

impl<R: AsyncBufRead + ?Sized + Unpin> Future for ReadLine<'_, R> {
type Output = io::Result<usize>;

fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
let Self { reader, buf, bytes, read } = &mut *self;
read_line_internal(Pin::new(reader), cx, buf, bytes, read)
let Self { reader, buf, bytes, read, finished: _ } = &mut *self;
let ret = ready!(read_line_internal(Pin::new(reader), cx, buf, bytes, read));
self.finished = true;
Poll::Ready(ret)
}
}

impl<R: ?Sized> Drop for ReadLine<'_, R> {
fn drop(&mut self) {
// restore old string contents
if !self.finished {
self.bytes.truncate(self.bytes.len() - self.read);
// Safety: `bytes` is valid UTF-8 because it was taken from a String
// and the newly read bytes have been removed.
mem::swap(unsafe { self.buf.as_mut_vec() }, &mut self.bytes);
}
}
}
3 changes: 1 addition & 2 deletions futures-util/src/io/read_until.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use futures_core::ready;
use futures_core::task::{Context, Poll};
use futures_io::AsyncBufRead;
use std::io;
use std::mem;
use std::pin::Pin;
use std::vec::Vec;

Expand Down Expand Up @@ -46,7 +45,7 @@ pub(super) fn read_until_internal<R: AsyncBufRead + ?Sized>(
reader.as_mut().consume(used);
*read += used;
if done || used == 0 {
return Poll::Ready(Ok(mem::replace(read, 0)));
return Poll::Ready(Ok(*read));
}
}
}
Expand Down
26 changes: 25 additions & 1 deletion futures/tests/io_lines.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use futures::executor::block_on;
use futures::future::{Future, FutureExt};
use futures::io::{AsyncBufReadExt, Cursor};
use futures::io::{AsyncBufReadExt, AsyncRead, Cursor};
use futures::stream::{self, StreamExt, TryStreamExt};
use futures::task::Poll;
use futures_test::io::AsyncReadTestExt;
Expand All @@ -27,6 +27,24 @@ macro_rules! run_next {
};
}

struct IOErrorRead(bool);

impl AsyncRead for IOErrorRead {
fn poll_read(
mut self: std::pin::Pin<&mut Self>,
_cx: &mut std::task::Context<'_>,
b: &mut [u8],
) -> Poll<std::io::Result<usize>> {
if self.0 {
Poll::Ready(Err(std::io::ErrorKind::InvalidInput.into()))
} else {
self.0 = true;
b[..16].fill(b'x');
Ok(16).into()
}
}
}

#[test]
fn lines() {
let buf = Cursor::new(&b"12\r"[..]);
Expand Down Expand Up @@ -58,3 +76,9 @@ fn maybe_pending() {
assert_eq!(run_next!(s), "".to_string());
assert!(run(s.next()).is_none());
}

#[test]
fn issue2862() {
let mut lines = futures::io::BufReader::new(IOErrorRead(false)).lines();
assert!(block_on(lines.next()).unwrap().is_err())
}
43 changes: 43 additions & 0 deletions futures/tests/io_read_line.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use futures::future::{Future, FutureExt};
use futures::io::{AsyncBufReadExt, Cursor};
use futures::stream::{self, StreamExt, TryStreamExt};
use futures::task::Poll;
use futures::AsyncRead;
use futures_test::io::AsyncReadTestExt;
use futures_test::task::noop_context;

Expand All @@ -15,6 +16,24 @@ fn run<F: Future + Unpin>(mut f: F) -> F::Output {
}
}

struct IOErrorRead(bool);

impl AsyncRead for IOErrorRead {
fn poll_read(
mut self: std::pin::Pin<&mut Self>,
_cx: &mut std::task::Context<'_>,
b: &mut [u8],
) -> Poll<std::io::Result<usize>> {
if self.0 {
Poll::Ready(Err(std::io::ErrorKind::InvalidInput.into()))
} else {
self.0 = true;
b[..16].fill(b'x');
Ok(16).into()
}
}
}

#[test]
fn read_line() {
let mut buf = Cursor::new(b"12");
Expand All @@ -34,6 +53,30 @@ fn read_line() {
assert_eq!(v, "");
}

#[test]
fn read_line_drop() {
// string contents should be preserved if the future is dropped
let mut buf = Cursor::new(b"12\n\n");
let mut v = String::from("abc");
drop(buf.read_line(&mut v));
assert_eq!(v, "abc");
}

#[test]
fn read_line_io_error() {
let mut r = futures::io::BufReader::new(IOErrorRead(false));
let _ = block_on(r.read_line(&mut String::new()));
}

#[test]
fn read_line_utf8_error() {
let mut buf = Cursor::new(b"12\xFF\n\n");
let mut v = String::from("abc");
let res = block_on(buf.read_line(&mut v));
assert_eq!(res.unwrap_err().kind(), std::io::ErrorKind::InvalidData);
assert_eq!(v, "abc");
}

#[test]
fn maybe_pending() {
let mut buf = b"12".interleave_pending();
Expand Down

0 comments on commit 99ebe58

Please sign in to comment.