From 0a42a540c603846aa22f29f378a61a64c9d4383e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20du=20Garreau?= Date: Wed, 7 Feb 2024 16:21:16 +0100 Subject: [PATCH] Make `io::BorrowedCursor::advance` safe This also keeps the old `advance` method under `advance_unchecked` name. This makes pattern like `std::io::default_read_buf` safe to write. --- library/core/src/io/borrowed_buf.rs | 22 +++++++++++++++++++++- library/core/tests/io/borrowed_buf.rs | 16 ++++------------ library/std/src/io/mod.rs | 12 ++---------- library/std/src/io/tests.rs | 2 +- library/std/src/io/util.rs | 2 +- library/std/src/sys/pal/hermit/net.rs | 2 +- library/std/src/sys/pal/solid/fs.rs | 2 +- library/std/src/sys/pal/solid/net.rs | 2 +- library/std/src/sys/pal/unix/fd.rs | 2 +- library/std/src/sys/pal/unix/net.rs | 2 +- library/std/src/sys/pal/wasi/fd.rs | 2 +- library/std/src/sys/pal/windows/handle.rs | 2 +- library/std/src/sys/pal/windows/net.rs | 2 +- library/std/src/sys/pal/windows/pipe.rs | 2 +- 14 files changed, 38 insertions(+), 34 deletions(-) diff --git a/library/core/src/io/borrowed_buf.rs b/library/core/src/io/borrowed_buf.rs index fe25cac280fe3..161bb4b6a1fc7 100644 --- a/library/core/src/io/borrowed_buf.rs +++ b/library/core/src/io/borrowed_buf.rs @@ -233,6 +233,26 @@ impl<'a> BorrowedCursor<'a> { &mut self.buf.buf[self.buf.filled..] } + /// Advance the cursor by asserting that `n` bytes have been filled. + /// + /// After advancing, the `n` bytes are no longer accessible via the cursor and can only be + /// accessed via the underlying buffer. I.e., the buffer's filled portion grows by `n` elements + /// and its unfilled portion (and the capacity of this cursor) shrinks by `n` elements. + /// + /// If less than `n` bytes initialized (by the cursor's point of view), `set_init` should be + /// called first. + /// + /// # Panics + /// + /// Panics if there are less than `n` bytes initialized. + #[inline] + pub fn advance(&mut self, n: usize) -> &mut Self { + assert!(self.buf.init >= self.buf.filled + n); + + self.buf.filled += n; + self + } + /// Advance the cursor by asserting that `n` bytes have been filled. /// /// After advancing, the `n` bytes are no longer accessible via the cursor and can only be @@ -244,7 +264,7 @@ impl<'a> BorrowedCursor<'a> { /// The caller must ensure that the first `n` bytes of the cursor have been properly /// initialised. #[inline] - pub unsafe fn advance(&mut self, n: usize) -> &mut Self { + pub unsafe fn advance_unchecked(&mut self, n: usize) -> &mut Self { self.buf.filled += n; self.buf.init = cmp::max(self.buf.init, self.buf.filled); self diff --git a/library/core/tests/io/borrowed_buf.rs b/library/core/tests/io/borrowed_buf.rs index 69511e49acdbc..a5dd4e525777a 100644 --- a/library/core/tests/io/borrowed_buf.rs +++ b/library/core/tests/io/borrowed_buf.rs @@ -40,9 +40,7 @@ fn advance_filled() { let buf: &mut [_] = &mut [0; 16]; let mut rbuf: BorrowedBuf<'_> = buf.into(); - unsafe { - rbuf.unfilled().advance(1); - } + rbuf.unfilled().advance(1); assert_eq!(rbuf.filled().len(), 1); assert_eq!(rbuf.unfilled().capacity(), 15); @@ -53,9 +51,7 @@ fn clear() { let buf: &mut [_] = &mut [255; 16]; let mut rbuf: BorrowedBuf<'_> = buf.into(); - unsafe { - rbuf.unfilled().advance(16); - } + rbuf.unfilled().advance(16); assert_eq!(rbuf.filled().len(), 16); assert_eq!(rbuf.unfilled().capacity(), 0); @@ -79,9 +75,7 @@ fn set_init() { assert_eq!(rbuf.init_len(), 8); - unsafe { - rbuf.unfilled().advance(4); - } + rbuf.unfilled().advance(4); unsafe { rbuf.set_init(2); @@ -153,9 +147,7 @@ fn cursor_set_init() { assert_eq!(rbuf.unfilled().uninit_mut().len(), 8); assert_eq!(unsafe { rbuf.unfilled().as_mut() }.len(), 16); - unsafe { - rbuf.unfilled().advance(4); - } + rbuf.unfilled().advance(4); unsafe { rbuf.unfilled().set_init(2); diff --git a/library/std/src/io/mod.rs b/library/std/src/io/mod.rs index a238e74ed95cf..f842a0b6d554e 100644 --- a/library/std/src/io/mod.rs +++ b/library/std/src/io/mod.rs @@ -578,15 +578,7 @@ where F: FnOnce(&mut [u8]) -> Result, { let n = read(cursor.ensure_init().init_mut())?; - assert!( - n <= cursor.capacity(), - "read should not return more bytes than there is capacity for in the read buffer" - ); - unsafe { - // SAFETY: we initialised using `ensure_init` so there is no uninit data to advance to - // and we have checked that the read amount is not over capacity (see #120603) - cursor.advance(n); - } + cursor.advance(n); Ok(()) } @@ -2915,7 +2907,7 @@ impl Read for Take { unsafe { // SAFETY: filled bytes have been filled and therefore initialized - buf.advance(filled); + buf.advance_unchecked(filled); // SAFETY: new_init bytes of buf's unfilled buffer have been initialized buf.set_init(new_init); } diff --git a/library/std/src/io/tests.rs b/library/std/src/io/tests.rs index 33e9d8efed511..fd7e51688cdeb 100644 --- a/library/std/src/io/tests.rs +++ b/library/std/src/io/tests.rs @@ -655,7 +655,7 @@ fn bench_take_read_buf(b: &mut test::Bencher) { // Issue #120603 #[test] -#[should_panic = "read should not return more bytes than there is capacity for in the read buffer"] +#[should_panic] fn read_buf_broken_read() { struct MalformedRead; diff --git a/library/std/src/io/util.rs b/library/std/src/io/util.rs index a04bc4811460b..16eaed15e720c 100644 --- a/library/std/src/io/util.rs +++ b/library/std/src/io/util.rs @@ -198,7 +198,7 @@ impl Read for Repeat { // SAFETY: the entire unfilled portion of buf has been initialized unsafe { - buf.advance(remaining); + buf.advance_unchecked(remaining); } Ok(()) diff --git a/library/std/src/sys/pal/hermit/net.rs b/library/std/src/sys/pal/hermit/net.rs index 3cf63fccf2e71..871a2ccdfa49c 100644 --- a/library/std/src/sys/pal/hermit/net.rs +++ b/library/std/src/sys/pal/hermit/net.rs @@ -156,7 +156,7 @@ impl Socket { ) })?; unsafe { - buf.advance(ret as usize); + buf.advance_unchecked(ret as usize); } Ok(()) } diff --git a/library/std/src/sys/pal/solid/fs.rs b/library/std/src/sys/pal/solid/fs.rs index 6c66b93a3e1a3..a6c1336109ad7 100644 --- a/library/std/src/sys/pal/solid/fs.rs +++ b/library/std/src/sys/pal/solid/fs.rs @@ -388,7 +388,7 @@ impl File { // Safety: `num_bytes_read` bytes were written to the unfilled // portion of the buffer - cursor.advance(num_bytes_read); + cursor.advance_unchecked(num_bytes_read); Ok(()) } diff --git a/library/std/src/sys/pal/solid/net.rs b/library/std/src/sys/pal/solid/net.rs index 1c310648a3db5..6ea874e509e20 100644 --- a/library/std/src/sys/pal/solid/net.rs +++ b/library/std/src/sys/pal/solid/net.rs @@ -209,7 +209,7 @@ impl Socket { netc::recv(self.as_raw_fd(), buf.as_mut().as_mut_ptr().cast(), buf.capacity(), flags) })?; unsafe { - buf.advance(ret as usize); + buf.advance_unchecked(ret as usize); } Ok(()) } diff --git a/library/std/src/sys/pal/unix/fd.rs b/library/std/src/sys/pal/unix/fd.rs index bf1fb3123c4ce..a1c0321876fa8 100644 --- a/library/std/src/sys/pal/unix/fd.rs +++ b/library/std/src/sys/pal/unix/fd.rs @@ -161,7 +161,7 @@ impl FileDesc { // Safety: `ret` bytes were written to the initialized portion of the buffer unsafe { - cursor.advance(ret as usize); + cursor.advance_unchecked(ret as usize); } Ok(()) } diff --git a/library/std/src/sys/pal/unix/net.rs b/library/std/src/sys/pal/unix/net.rs index 8f537de7026f5..1b6a6bb2c5c77 100644 --- a/library/std/src/sys/pal/unix/net.rs +++ b/library/std/src/sys/pal/unix/net.rs @@ -272,7 +272,7 @@ impl Socket { ) })?; unsafe { - buf.advance(ret as usize); + buf.advance_unchecked(ret as usize); } Ok(()) } diff --git a/library/std/src/sys/pal/wasi/fd.rs b/library/std/src/sys/pal/wasi/fd.rs index d7295a799daab..8966e4b80ad37 100644 --- a/library/std/src/sys/pal/wasi/fd.rs +++ b/library/std/src/sys/pal/wasi/fd.rs @@ -60,7 +60,7 @@ impl WasiFd { }]; match wasi::fd_read(self.as_raw_fd() as wasi::Fd, &bufs) { Ok(n) => { - buf.advance(n); + buf.advance_unchecked(n); Ok(()) } Err(e) => Err(err2io(e)), diff --git a/library/std/src/sys/pal/windows/handle.rs b/library/std/src/sys/pal/windows/handle.rs index c4495f81a5a3a..3f85bb0a099a9 100644 --- a/library/std/src/sys/pal/windows/handle.rs +++ b/library/std/src/sys/pal/windows/handle.rs @@ -121,7 +121,7 @@ impl Handle { Ok(read) => { // Safety: `read` bytes were written to the initialized portion of the buffer unsafe { - cursor.advance(read); + cursor.advance_unchecked(read); } Ok(()) } diff --git a/library/std/src/sys/pal/windows/net.rs b/library/std/src/sys/pal/windows/net.rs index c34e01e000aca..e37fbe9ef83e4 100644 --- a/library/std/src/sys/pal/windows/net.rs +++ b/library/std/src/sys/pal/windows/net.rs @@ -234,7 +234,7 @@ impl Socket { } } _ => { - unsafe { buf.advance(result as usize) }; + unsafe { buf.advance_unchecked(result as usize) }; Ok(()) } } diff --git a/library/std/src/sys/pal/windows/pipe.rs b/library/std/src/sys/pal/windows/pipe.rs index 7624e746f5c89..fd10df82d8b47 100644 --- a/library/std/src/sys/pal/windows/pipe.rs +++ b/library/std/src/sys/pal/windows/pipe.rs @@ -273,7 +273,7 @@ impl AnonPipe { Err(e) => Err(e), Ok(n) => { unsafe { - buf.advance(n); + buf.advance_unchecked(n); } Ok(()) }