Fix issues with AsyncBufRead::read_line and AsyncBufReadExt::lines (#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

Author: hkratz

futures-util/src/io/lines.rs

@@ -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);
         }

futures-util/src/io/read_line.rs

@@ -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 }
     }
 }
 
@@ -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);
+        }
     }
 }

futures-util/src/io/read_until.rs

@@ -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;
 
@@ -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));
         }
     }
 }

futures/tests/io_lines.rs

@@ -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;
@@ -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"[..]);
@@ -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())
+}

futures/tests/io_read_line.rs

@@ -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;
 
@@ -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");
@@ -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();