From 418907767d9e14fc7cd8146a95d363000fb2219a Mon Sep 17 00:00:00 2001 From: Trevor Elliott Date: Fri, 19 Apr 2024 15:09:52 -0700 Subject: [PATCH] Restrict the preview1 implementation of fd_read to one iovec (#8415) * We can only have one mutable borrow at a time after #8277 Co-authored-by: Nick Fitzgerald * Add a test like pread/pwrite, but for read/write --------- Co-authored-by: Nick Fitzgerald --- .../src/bin/preview1_file_pread_pwrite.rs | 2 +- .../src/bin/preview1_file_read_write.rs | 209 ++++++++++++++++++ crates/wasi-common/src/snapshots/preview_1.rs | 12 +- crates/wasi-common/tests/all/async_.rs | 4 + crates/wasi-common/tests/all/sync.rs | 4 + crates/wasi/tests/all/async_.rs | 6 + crates/wasi/tests/all/preview1.rs | 4 + crates/wasi/tests/all/sync.rs | 4 + 8 files changed, 240 insertions(+), 5 deletions(-) create mode 100644 crates/test-programs/src/bin/preview1_file_read_write.rs diff --git a/crates/test-programs/src/bin/preview1_file_pread_pwrite.rs b/crates/test-programs/src/bin/preview1_file_pread_pwrite.rs index ff1630f43f91..77061e72104f 100644 --- a/crates/test-programs/src/bin/preview1_file_pread_pwrite.rs +++ b/crates/test-programs/src/bin/preview1_file_pread_pwrite.rs @@ -165,7 +165,7 @@ unsafe fn test_file_pwrite_and_file_pos(dir_fd: wasi::Fd) { buf: buf.as_ptr(), buf_len: buf.len(), }; - let n = wasi::fd_pwrite(file_fd, &mut [ciovec], 50).expect("writing bytes at offset 2"); + let n = wasi::fd_pwrite(file_fd, &mut [ciovec], 50).expect("writing bytes at offset 50"); assert_eq!(n, 1); assert_eq!(wasi::fd_tell(file_fd).unwrap(), 0); diff --git a/crates/test-programs/src/bin/preview1_file_read_write.rs b/crates/test-programs/src/bin/preview1_file_read_write.rs new file mode 100644 index 000000000000..f32825a279e1 --- /dev/null +++ b/crates/test-programs/src/bin/preview1_file_read_write.rs @@ -0,0 +1,209 @@ +use std::{env, process}; +use test_programs::preview1::open_scratch_directory; + +unsafe fn test_file_read_write(dir_fd: wasi::Fd) { + // Create a file in the scratch directory. + let file_fd = wasi::path_open( + dir_fd, + 0, + "file", + wasi::OFLAGS_CREAT, + wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE, + 0, + 0, + ) + .expect("opening a file"); + assert!( + file_fd > libc::STDERR_FILENO as wasi::Fd, + "file descriptor range check", + ); + + let contents = &[0u8, 1, 2, 3]; + let ciovec = wasi::Ciovec { + buf: contents.as_ptr() as *const _, + buf_len: contents.len(), + }; + let mut nwritten = wasi::fd_write(file_fd, &mut [ciovec]).expect("writing bytes at offset 0"); + assert_eq!(nwritten, 4, "nwritten bytes check"); + + let contents = &mut [0u8; 4]; + let iovec = wasi::Iovec { + buf: contents.as_mut_ptr() as *mut _, + buf_len: contents.len(), + }; + wasi::fd_seek(file_fd, 0, wasi::WHENCE_SET).expect("seeking to offset 0"); + let mut nread = wasi::fd_read(file_fd, &[iovec]).expect("reading bytes at offset 0"); + assert_eq!(nread, 4, "nread bytes check"); + assert_eq!(contents, &[0u8, 1, 2, 3], "written bytes equal read bytes"); + + // Write all the data through multiple iovecs. + // + // Note that this needs to be done with a loop, because some + // platforms do not support writing multiple iovecs at once. + // See https://github.com/rust-lang/rust/issues/74825. + let contents = &[0u8, 1, 2, 3]; + let mut offset = 0usize; + wasi::fd_seek(file_fd, 0, wasi::WHENCE_SET).expect("seeking to offset 0"); + loop { + let mut ciovecs: Vec = Vec::new(); + let mut remaining = contents.len() - offset; + if remaining > 2 { + ciovecs.push(wasi::Ciovec { + buf: contents[offset..].as_ptr() as *const _, + buf_len: 2, + }); + remaining -= 2; + } + ciovecs.push(wasi::Ciovec { + buf: contents[contents.len() - remaining..].as_ptr() as *const _, + buf_len: remaining, + }); + + nwritten = wasi::fd_write(file_fd, ciovecs.as_slice()).expect("writing bytes at offset 0"); + + offset += nwritten; + if offset == contents.len() { + break; + } + } + assert_eq!(offset, 4, "nread bytes check"); + + // Read all the data through multiple iovecs. + // + // Note that this needs to be done with a loop, because some + // platforms do not support reading multiple iovecs at once. + // See https://github.com/rust-lang/rust/issues/74825. + let contents = &mut [0u8; 4]; + let mut offset = 0usize; + wasi::fd_seek(file_fd, 0, wasi::WHENCE_SET).expect("seeking to offset 0"); + loop { + let buffer = &mut [0u8; 4]; + let iovecs = &[ + wasi::Iovec { + buf: buffer.as_mut_ptr() as *mut _, + buf_len: 2, + }, + wasi::Iovec { + buf: buffer[2..].as_mut_ptr() as *mut _, + buf_len: 2, + }, + ]; + nread = wasi::fd_read(file_fd, iovecs).expect("reading bytes at offset 0"); + if nread == 0 { + break; + } + contents[offset..offset + nread].copy_from_slice(&buffer[0..nread]); + offset += nread; + } + assert_eq!(offset, 4, "nread bytes check"); + assert_eq!(contents, &[0u8, 1, 2, 3], "file cursor was overwritten"); + + let contents = &mut [0u8; 4]; + let iovec = wasi::Iovec { + buf: contents.as_mut_ptr() as *mut _, + buf_len: contents.len(), + }; + wasi::fd_seek(file_fd, 2, wasi::WHENCE_SET).expect("seeking to offset 2"); + nread = wasi::fd_read(file_fd, &[iovec]).expect("reading bytes at offset 2"); + assert_eq!(nread, 2, "nread bytes check"); + assert_eq!(contents, &[2u8, 3, 0, 0], "file cursor was overwritten"); + + let contents = &[1u8, 0]; + let ciovec = wasi::Ciovec { + buf: contents.as_ptr() as *const _, + buf_len: contents.len(), + }; + wasi::fd_seek(file_fd, 2, wasi::WHENCE_SET).expect("seeking to offset 2"); + nwritten = wasi::fd_write(file_fd, &mut [ciovec]).expect("writing bytes at offset 2"); + assert_eq!(nwritten, 2, "nwritten bytes check"); + + let contents = &mut [0u8; 4]; + let iovec = wasi::Iovec { + buf: contents.as_mut_ptr() as *mut _, + buf_len: contents.len(), + }; + wasi::fd_seek(file_fd, 0, wasi::WHENCE_SET).expect("seeking to offset 0"); + nread = wasi::fd_read(file_fd, &[iovec]).expect("reading bytes at offset 0"); + assert_eq!(nread, 4, "nread bytes check"); + assert_eq!(contents, &[0u8, 1, 1, 0], "file cursor was overwritten"); + + wasi::fd_close(file_fd).expect("closing a file"); + wasi::path_unlink_file(dir_fd, "file").expect("removing a file"); +} + +unsafe fn test_file_write_and_file_pos(dir_fd: wasi::Fd) { + let path = "file2"; + let file_fd = wasi::path_open( + dir_fd, + 0, + path, + wasi::OFLAGS_CREAT, + wasi::RIGHTS_FD_READ | wasi::RIGHTS_FD_WRITE, + 0, + 0, + ) + .expect("opening a file"); + assert!( + file_fd > libc::STDERR_FILENO as wasi::Fd, + "file descriptor range check", + ); + + // Perform a 0-sized pwrite at an offset beyond the end of the file. Unix + // semantics should pop out where nothing is actually written and the size + // of the file isn't modified. + assert_eq!(wasi::fd_tell(file_fd).unwrap(), 0); + let ciovec = wasi::Ciovec { + buf: [].as_ptr(), + buf_len: 0, + }; + wasi::fd_seek(file_fd, 2, wasi::WHENCE_SET).expect("seeking to offset 2"); + let n = wasi::fd_write(file_fd, &mut [ciovec]).expect("writing bytes at offset 2"); + assert_eq!(n, 0); + + assert_eq!(wasi::fd_tell(file_fd).unwrap(), 2); + let stat = wasi::fd_filestat_get(file_fd).unwrap(); + assert_eq!(stat.size, 0); + + // Now write a single byte and make sure it actually works + let buf = [0]; + let ciovec = wasi::Ciovec { + buf: buf.as_ptr(), + buf_len: buf.len(), + }; + wasi::fd_seek(file_fd, 50, wasi::WHENCE_SET).expect("seeking to offset 50"); + let n = wasi::fd_write(file_fd, &mut [ciovec]).expect("writing bytes at offset 50"); + assert_eq!(n, 1); + + assert_eq!(wasi::fd_tell(file_fd).unwrap(), 51); + let stat = wasi::fd_filestat_get(file_fd).unwrap(); + assert_eq!(stat.size, 51); + + wasi::fd_close(file_fd).expect("closing a file"); + wasi::path_unlink_file(dir_fd, path).expect("removing a file"); +} + +fn main() { + let mut args = env::args(); + let prog = args.next().unwrap(); + let arg = if let Some(arg) = args.next() { + arg + } else { + eprintln!("usage: {} ", prog); + process::exit(1); + }; + + // Open scratch directory + let dir_fd = match open_scratch_directory(&arg) { + Ok(dir_fd) => dir_fd, + Err(err) => { + eprintln!("{}", err); + process::exit(1) + } + }; + + // Run the tests. + unsafe { + test_file_read_write(dir_fd); + test_file_write_and_file_pos(dir_fd); + } +} diff --git a/crates/wasi-common/src/snapshots/preview_1.rs b/crates/wasi-common/src/snapshots/preview_1.rs index b73d3a5ec87f..a58312651ae4 100644 --- a/crates/wasi-common/src/snapshots/preview_1.rs +++ b/crates/wasi-common/src/snapshots/preview_1.rs @@ -340,13 +340,17 @@ impl wasi_snapshot_preview1::WasiSnapshotPreview1 for WasiCtx { return Ok(0); } } else { - // Convert all of the unsafe guest slices to safe ones--this uses - // Wiggle's internal borrow checker to ensure no overlaps. We assume - // here that, because the memory is not shared, there are no other - // threads to access it while it is written to. + // Convert the first unsafe guest slice into a safe one--Wiggle + // can only track mutable borrows for an entire region, and converting + // all guest pointers to slices would cause a runtime borrow-checking + // error. As read is allowed to return less than the requested amount, + // it's valid (though not as efficient) for us to only perform the + // read of the first buffer. let mut guest_slices: Vec> = iovs .into_iter() + .filter(|iov| iov.len() > 0) .map(|iov| Ok(iov.as_slice_mut()?.unwrap())) + .take(1) .collect::>()?; // Read directly into the Wasm memory. diff --git a/crates/wasi-common/tests/all/async_.rs b/crates/wasi-common/tests/all/async_.rs index 4bcf8e828dd8..66d04b990183 100644 --- a/crates/wasi-common/tests/all/async_.rs +++ b/crates/wasi-common/tests/all/async_.rs @@ -133,6 +133,10 @@ async fn preview1_file_pread_pwrite() { run(PREVIEW1_FILE_PREAD_PWRITE, true).await.unwrap() } #[test_log::test(tokio::test(flavor = "multi_thread"))] +async fn preview1_file_read_write() { + run(PREVIEW1_FILE_READ_WRITE, true).await.unwrap() +} +#[test_log::test(tokio::test(flavor = "multi_thread"))] async fn preview1_file_seek_tell() { run(PREVIEW1_FILE_SEEK_TELL, true).await.unwrap() } diff --git a/crates/wasi-common/tests/all/sync.rs b/crates/wasi-common/tests/all/sync.rs index 4af52d33a1cb..c29ca81052f2 100644 --- a/crates/wasi-common/tests/all/sync.rs +++ b/crates/wasi-common/tests/all/sync.rs @@ -125,6 +125,10 @@ fn preview1_file_pread_pwrite() { run(PREVIEW1_FILE_PREAD_PWRITE, true).unwrap() } #[test_log::test] +fn preview1_file_read_write() { + run(PREVIEW1_FILE_READ_WRITE, true).unwrap() +} +#[test_log::test] fn preview1_file_seek_tell() { run(PREVIEW1_FILE_SEEK_TELL, true).unwrap() } diff --git a/crates/wasi/tests/all/async_.rs b/crates/wasi/tests/all/async_.rs index b6efb44d360a..4ff988ab8e77 100644 --- a/crates/wasi/tests/all/async_.rs +++ b/crates/wasi/tests/all/async_.rs @@ -99,6 +99,12 @@ async fn preview1_file_pread_pwrite() { .unwrap() } #[test_log::test(tokio::test(flavor = "multi_thread"))] +async fn preview1_file_read_write() { + run(PREVIEW1_FILE_READ_WRITE_COMPONENT, false) + .await + .unwrap() +} +#[test_log::test(tokio::test(flavor = "multi_thread"))] async fn preview1_file_seek_tell() { run(PREVIEW1_FILE_SEEK_TELL_COMPONENT, false).await.unwrap() } diff --git a/crates/wasi/tests/all/preview1.rs b/crates/wasi/tests/all/preview1.rs index b5f67c23ed21..00a3ab0c7177 100644 --- a/crates/wasi/tests/all/preview1.rs +++ b/crates/wasi/tests/all/preview1.rs @@ -86,6 +86,10 @@ async fn preview1_file_pread_pwrite() { run(PREVIEW1_FILE_PREAD_PWRITE, false).await.unwrap() } #[test_log::test(tokio::test(flavor = "multi_thread"))] +async fn preview1_file_read_write() { + run(PREVIEW1_FILE_READ_WRITE, false).await.unwrap() +} +#[test_log::test(tokio::test(flavor = "multi_thread"))] async fn preview1_file_seek_tell() { run(PREVIEW1_FILE_SEEK_TELL, false).await.unwrap() } diff --git a/crates/wasi/tests/all/sync.rs b/crates/wasi/tests/all/sync.rs index ab89ff77f955..5dcf0faef23c 100644 --- a/crates/wasi/tests/all/sync.rs +++ b/crates/wasi/tests/all/sync.rs @@ -93,6 +93,10 @@ fn preview1_file_pread_pwrite() { run(PREVIEW1_FILE_PREAD_PWRITE_COMPONENT, false).unwrap() } #[test_log::test] +fn preview1_file_read_write() { + run(PREVIEW1_FILE_READ_WRITE_COMPONENT, false).unwrap() +} +#[test_log::test] fn preview1_file_seek_tell() { run(PREVIEW1_FILE_SEEK_TELL_COMPONENT, false).unwrap() }