Repository: rust-lang/futures-rs (rust)

Issue:
`Lines::poll_next` panics if the reader returns an error after returning data
0.3.30:

`Lines::poll_next` propagates the error from reader before it restores the invariant that `buf` is empty. This results in panics on IO failures.

<https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=488083df8fff1bae3dd22cabbb405451>

```rust
use futures::AsyncRead;
use futures::StreamExt;
use futures::AsyncBufReadExt;
use futures::executor::block_on;
use std::task::Poll;

struct BadReader(bool);

impl AsyncRead for BadReader {
    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 {
            return Poll::Ready(Err(std::io::ErrorKind::InvalidInput.into()));
        } else {
            self.0 = true;
            b.fill(b'x');
            Ok(b.len()).into()
        }
    }
}

fn main() {
    let mut lines = futures::io::BufReader::new(BadReader(false)).lines();
    while let Some(_) = block_on(lines.next()) {}
}
```

Output:

```
  Compiling playground v0.0.1 (/playground)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.66s
     Running `target/debug/playground`
thread 'main' panicked at /playground/.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-util-0.3.30/src/io/read_line.rs:44:9:
assertion `left == right` failed
  left: 8192
 right: 0
stack backtrace:
   0: rust_begin_unwind
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_fmt
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panicking.rs:72:14
   2: core::panicking::assert_failed_inner
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panicking.rs:343:17
   3: core::panicking::assert_failed
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/panicking.rs:298:5
   4: futures_util::io::read_line::read_line_internal
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-util-0.3.30/src/io/read_line.rs:44:9
   5: <futures_util::io::lines::Lines<R> as futures_core::stream::Stream>::poll_next
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-util-0.3.30/src/io/lines.rs:35:24
   6: futures_util::stream::stream::StreamExt::poll_next_unpin
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-util-0.3.30/src/stream/stream/mod.rs:1638:9
   7: <futures_util::stream::stream::next::Next<St> as core::future::future::Future>::poll
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-util-0.3.30/src/stream/stream/next.rs:32:9
   8: futures_executor::local_pool::block_on::{{closure}}
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-executor-0.3.30/src/local_pool.rs:317:23
   9: futures_executor::local_pool::run_executor::{{closure}}
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-executor-0.3.30/src/local_pool.rs:90:37
  10: std::thread::local::LocalKey<T>::try_with
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/thread/local.rs:284:16
  11: std::thread::local::LocalKey<T>::with
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/std/src/thread/local.rs:260:9
  12: futures_executor::local_pool::run_executor
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-executor-0.3.30/src/local_pool.rs:86:5
  13: futures_executor::local_pool::block_on
             at ./.cargo/registry/src/index.crates.io-6f17d22bba15001f/futures-executor-0.3.30/src/local_pool.rs:317:5
  14: playground::main
             at ./src/main.rs:23:25
  15: core::ops::function::FnOnce::call_once
             at /rustc/9b00956e56009bab2aa15d7bff10916599e3d6d6/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
```

Pull request:
Fix issues with `AsyncBufRead::read_line` and `AsyncBufReadExt::lines`
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`
* If an IO error occurs after `read_until_internal` already read a couple of bytes a debug assertion fails. (Fixes #2862)

Fixes #2862

Patch:
diff --git a/futures-task/src/waker.rs b/futures-task/src/waker.rs
index b1410a19..747457a5 100644
--- a/futures-task/src/waker.rs
+++ b/futures-task/src/waker.rs
@@ -36,6 +36,7 @@ unsafe fn increase_refcount<T: ArcWake + 'static>(data: *const ()) {
 }
 
 // used by `waker_ref`
+#[inline(always)]
 unsafe fn clone_arc_raw<T: ArcWake + 'static>(data: *const ()) -> RawWaker {
     unsafe { increase_refcount::<T>(data) }
     RawWaker::new(data, waker_vtable::<T>())
diff --git a/futures-util/src/io/lines.rs b/futures-util/src/io/lines.rs
index 8c4d17c5..0a1abf4b 100644
--- a/futures-util/src/io/lines.rs
+++ b/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);
         }
diff --git a/futures-util/src/io/read_line.rs b/futures-util/src/io/read_line.rs
index b4483223..43942add 100644
--- a/futures-util/src/io/read_line.rs
+++ b/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);
+        }
     }
 }
diff --git a/futures-util/src/io/read_until.rs b/futures-util/src/io/read_until.rs
index d6121d6f..adc359db 100644
--- a/futures-util/src/io/read_until.rs
+++ b/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));
         }
     }
 }


Test patch:
diff --git a/futures/tests/io_lines.rs b/futures/tests/io_lines.rs
index 5ce01a69..62afef32 100644
--- a/futures/tests/io_lines.rs
+++ b/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())
+}
diff --git a/futures/tests/io_read_line.rs b/futures/tests/io_read_line.rs
index 88a87792..c7559c59 100644
--- a/futures/tests/io_read_line.rs
+++ b/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();
diff --git a/futures/tests/task_arc_wake.rs b/futures/tests/task_arc_wake.rs
index 36c7c67b..aedc15bc 100644
--- a/futures/tests/task_arc_wake.rs
+++ b/futures/tests/task_arc_wake.rs
@@ -44,18 +44,17 @@ fn create_from_arc() {
     assert_eq!(1, Arc::strong_count(&some_w));
 }
 
-// TODO: rustc regression: https://github.com/rust-lang/rust/issues/121600
-// #[test]
-// fn ref_wake_same() {
-//     let some_w = Arc::new(CountingWaker::new());
-//
-//     let w1: Waker = task::waker(some_w.clone());
-//     let w2 = task::waker_ref(&some_w);
-//     let w3 = w2.clone();
-//
-//     assert!(w1.will_wake(&w2));
-//     assert!(w2.will_wake(&w3));
-// }
+#[test]
+fn ref_wake_same() {
+    let some_w = Arc::new(CountingWaker::new());
+
+    let w1: Waker = task::waker(some_w.clone());
+    let w2 = task::waker_ref(&some_w);
+    let w3 = w2.clone();
+
+    assert!(w1.will_wake(&w2));
+    assert!(w2.will_wake(&w3));
+}
 
 #[test]
 fn proper_refcount_on_wake_panic() {