From 8472204d13e42b3e8e6342dec8aff4d5f929feba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20du=20Garreau?= Date: Wed, 22 May 2024 11:37:02 +0200 Subject: [PATCH 1/5] Fix `io::Take::read_buf` --- std/src/io/mod.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/std/src/io/mod.rs b/std/src/io/mod.rs index 0b57d01f27346..98e7b40bdc6ec 100644 --- a/std/src/io/mod.rs +++ b/std/src/io/mod.rs @@ -2941,7 +2941,7 @@ impl Read for Take { } let mut cursor = sliced_buf.unfilled(); - self.inner.read_buf(cursor.reborrow())?; + let result = self.inner.read_buf(cursor.reborrow()); let new_init = cursor.init_ref().len(); let filled = sliced_buf.len(); @@ -2956,13 +2956,14 @@ impl Read for Take { } self.limit -= filled as u64; + + result } else { let written = buf.written(); - self.inner.read_buf(buf.reborrow())?; + let result = self.inner.read_buf(buf.reborrow()); self.limit -= (buf.written() - written) as u64; + result } - - Ok(()) } } From 57f279d91b7deea8958b7718cfc720974bd2a11f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20du=20Garreau?= Date: Wed, 22 May 2024 11:41:19 +0200 Subject: [PATCH 2/5] Fix `io::BufReader` uses of `read_buf` --- std/src/io/buffered/bufreader.rs | 2 +- std/src/io/buffered/bufreader/buffer.rs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/std/src/io/buffered/bufreader.rs b/std/src/io/buffered/bufreader.rs index e51dde994de45..438df70505b20 100644 --- a/std/src/io/buffered/bufreader.rs +++ b/std/src/io/buffered/bufreader.rs @@ -349,7 +349,7 @@ impl Read for BufReader { let prev = cursor.written(); let mut rem = self.fill_buf()?; - rem.read_buf(cursor.reborrow())?; + rem.read_buf(cursor.reborrow())?; // actually never fails self.consume(cursor.written() - prev); //slice impl of read_buf known to never unfill buf diff --git a/std/src/io/buffered/bufreader/buffer.rs b/std/src/io/buffered/bufreader/buffer.rs index 1bf84d8bef312..68b024bb98aac 100644 --- a/std/src/io/buffered/bufreader/buffer.rs +++ b/std/src/io/buffered/bufreader/buffer.rs @@ -133,11 +133,13 @@ impl Buffer { buf.set_init(self.initialized); } - reader.read_buf(buf.unfilled())?; + let result = reader.read_buf(buf.unfilled()); self.pos = 0; self.filled = buf.len(); self.initialized = buf.init_len(); + + result?; } Ok(self.buffer()) } From f34267540cee8c8b3609313fe71de46071882eb8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20du=20Garreau?= Date: Wed, 22 May 2024 11:47:58 +0200 Subject: [PATCH 3/5] Fix `io::default_read_to_end` uses of `read_buf` --- std/src/io/mod.rs | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/std/src/io/mod.rs b/std/src/io/mod.rs index 98e7b40bdc6ec..954062a489c78 100644 --- a/std/src/io/mod.rs +++ b/std/src/io/mod.rs @@ -474,18 +474,28 @@ pub(crate) fn default_read_to_end( } let mut cursor = read_buf.unfilled(); - loop { + let result = loop { match r.read_buf(cursor.reborrow()) { - Ok(()) => break, Err(e) if e.is_interrupted() => continue, - Err(e) => return Err(e), + // Do not stop now in case of error: we might have received both data + // and an error + res => break res, } - } + }; let unfilled_but_initialized = cursor.init_ref().len(); let bytes_read = cursor.written(); let was_fully_initialized = read_buf.init_len() == buf_len; + // SAFETY: BorrowedBuf's invariants mean this much memory is initialized. + unsafe { + let new_len = bytes_read + buf.len(); + buf.set_len(new_len); + } + + // Now that all data is pushed to the vector, we can fail without data loss + result?; + if bytes_read == 0 { return Ok(buf.len() - start_len); } @@ -499,12 +509,6 @@ pub(crate) fn default_read_to_end( // store how much was initialized but not filled initialized = unfilled_but_initialized; - // SAFETY: BorrowedBuf's invariants mean this much memory is initialized. - unsafe { - let new_len = bytes_read + buf.len(); - buf.set_len(new_len); - } - // Use heuristics to determine the max read size if no initial size hint was provided if size_hint.is_none() { // The reader is returning short reads but it doesn't call ensure_init(). From 4a11249c0f70d699530d808f6d107643a5cc2dfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20du=20Garreau?= Date: Mon, 23 Sep 2024 17:38:18 +0200 Subject: [PATCH 4/5] Add tests --- std/src/io/tests.rs | 63 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/std/src/io/tests.rs b/std/src/io/tests.rs index f551dcd401efc..56b71c47dc73c 100644 --- a/std/src/io/tests.rs +++ b/std/src/io/tests.rs @@ -735,6 +735,69 @@ fn read_buf_full_read() { assert_eq!(BufReader::new(FullRead).fill_buf().unwrap().len(), DEFAULT_BUF_SIZE); } +struct DataAndErrorReader(&'static [u8]); + +impl Read for DataAndErrorReader { + fn read(&mut self, _buf: &mut [u8]) -> io::Result { + panic!("We want tests to use `read_buf`") + } + + fn read_buf(&mut self, buf: io::BorrowedCursor<'_>) -> io::Result<()> { + self.0.read_buf(buf).unwrap(); + Err(io::Error::other("error")) + } +} + +#[test] +fn read_buf_data_and_error_take() { + let mut buf = [0; 64]; + let mut buf = io::BorrowedBuf::from(buf.as_mut_slice()); + + let mut r = DataAndErrorReader(&[4, 5, 6]).take(1); + assert!(r.read_buf(buf.unfilled()).is_err()); + assert_eq!(buf.filled(), &[4]); + + assert!(r.read_buf(buf.unfilled()).is_ok()); + assert_eq!(buf.filled(), &[4]); + assert_eq!(r.get_ref().0, &[5, 6]); +} + +#[test] +fn read_buf_data_and_error_buf() { + let mut r = BufReader::new(DataAndErrorReader(&[4, 5, 6])); + + assert!(r.fill_buf().is_err()); + assert_eq!(r.fill_buf().unwrap(), &[4, 5, 6]); +} + +#[test] +fn read_buf_data_and_error_read_to_end() { + let mut r = DataAndErrorReader(&[4, 5, 6]); + + let mut v = Vec::with_capacity(200); + assert!(r.read_to_end(&mut v).is_err()); + + assert_eq!(v, &[4, 5, 6]); +} + +#[test] +fn read_to_end_error() { + struct ErrorReader; + + impl Read for ErrorReader { + fn read(&mut self, _buf: &mut [u8]) -> io::Result { + Err(io::Error::other("error")) + } + } + + let mut r = [4, 5, 6].chain(ErrorReader); + + let mut v = Vec::with_capacity(200); + assert!(r.read_to_end(&mut v).is_err()); + + assert_eq!(v, &[4, 5, 6]); +} + #[test] // Miri does not support signalling OOM #[cfg_attr(miri, ignore)] From 57557c9629f47efb9d3b6aba7cb75ac3688192d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20du=20Garreau?= Date: Mon, 23 Sep 2024 17:40:04 +0200 Subject: [PATCH 5/5] Add a comment to `Read::read_buf` --- std/src/io/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/std/src/io/mod.rs b/std/src/io/mod.rs index 954062a489c78..dd6458c38c6e9 100644 --- a/std/src/io/mod.rs +++ b/std/src/io/mod.rs @@ -978,6 +978,8 @@ pub trait Read { /// with uninitialized buffers. The new data will be appended to any existing contents of `buf`. /// /// The default implementation delegates to `read`. + /// + /// This method makes it possible to return both data and an error but it is advised against. #[unstable(feature = "read_buf", issue = "78485")] fn read_buf(&mut self, buf: BorrowedCursor<'_>) -> Result<()> { default_read_buf(|b| self.read(b), buf)