From ea7c1366b5c11e7da7cdd9b1ceb9e5b9c2912351 Mon Sep 17 00:00:00 2001 From: Askar Safin Date: Sat, 29 Jun 2024 04:18:51 +0300 Subject: [PATCH] Fix libc::read shim: make it write to a buffer correct amount of bytes. Add tests for the new behavior. libc::read shim had a bug: if underlying real call libc::read(fd, buf, N) returns M, then libc::read shim writes N bytes to buf instead of M. Remaining N - M bytes are filled with zeros. This commit fixes this bug and adds tests for new behavior --- src/tools/miri/src/shims/unix/fd.rs | 7 +++- .../libc-read-and-uninit-premature-eof.rs | 27 ++++++++++++++ .../libc-read-and-uninit-premature-eof.stderr | 15 ++++++++ src/tools/miri/tests/pass-dep/libc/libc-fs.rs | 35 +++++++++++++++++++ 4 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 src/tools/miri/tests/fail-dep/libc/libc-read-and-uninit-premature-eof.rs create mode 100644 src/tools/miri/tests/fail-dep/libc/libc-read-and-uninit-premature-eof.stderr diff --git a/src/tools/miri/src/shims/unix/fd.rs b/src/tools/miri/src/shims/unix/fd.rs index 599f78e712a2..c144f1cdccb4 100644 --- a/src/tools/miri/src/shims/unix/fd.rs +++ b/src/tools/miri/src/shims/unix/fd.rs @@ -394,7 +394,12 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> { match result { Ok(read_bytes) => { // If reading to `bytes` did not fail, we write those bytes to the buffer. - this.write_bytes_ptr(buf, bytes)?; + // Crucially, if fewer than `bytes.len()` bytes were read, only write + // that much into the output buffer! + this.write_bytes_ptr( + buf, + bytes[..usize::try_from(read_bytes).unwrap()].iter().copied(), + )?; Ok(read_bytes) } Err(e) => { diff --git a/src/tools/miri/tests/fail-dep/libc/libc-read-and-uninit-premature-eof.rs b/src/tools/miri/tests/fail-dep/libc/libc-read-and-uninit-premature-eof.rs new file mode 100644 index 000000000000..98ef454c88b3 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/libc/libc-read-and-uninit-premature-eof.rs @@ -0,0 +1,27 @@ +//! We test that if we requested to read 4 bytes, but actually read 3 bytes, +//! then 3 bytes (not 4) will be initialized. +//@ignore-target-windows: no file system support on Windows +//@compile-flags: -Zmiri-disable-isolation + +use std::ffi::CString; +use std::fs::remove_file; +use std::mem::MaybeUninit; + +#[path = "../../utils/mod.rs"] +mod utils; + +fn main() { + let path = + utils::prepare_with_content("fail-libc-read-and-uninit-premature-eof.txt", &[1u8, 2, 3]); + let cpath = CString::new(path.clone().into_os_string().into_encoded_bytes()).unwrap(); + unsafe { + let fd = libc::open(cpath.as_ptr(), libc::O_RDONLY); + assert_ne!(fd, -1); + let mut buf: MaybeUninit<[u8; 4]> = std::mem::MaybeUninit::uninit(); + // Read 4 bytes from a 3-byte file. + assert_eq!(libc::read(fd, buf.as_mut_ptr().cast::(), 4), 3); + buf.assume_init(); //~ERROR: Undefined Behavior: constructing invalid value at .value[3]: encountered uninitialized memory, but expected an integer + assert_eq!(libc::close(fd), 0); + } + remove_file(&path).unwrap(); +} diff --git a/src/tools/miri/tests/fail-dep/libc/libc-read-and-uninit-premature-eof.stderr b/src/tools/miri/tests/fail-dep/libc/libc-read-and-uninit-premature-eof.stderr new file mode 100644 index 000000000000..e4c7aba07e34 --- /dev/null +++ b/src/tools/miri/tests/fail-dep/libc/libc-read-and-uninit-premature-eof.stderr @@ -0,0 +1,15 @@ +error: Undefined Behavior: constructing invalid value at .value[3]: encountered uninitialized memory, but expected an integer + --> $DIR/libc-read-and-uninit-premature-eof.rs:LL:CC + | +LL | ... buf.assume_init(); + | ^^^^^^^^^^^^^^^^^ constructing invalid value at .value[3]: encountered uninitialized memory, but expected an integer + | + = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior + = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information + = note: BACKTRACE: + = note: inside `main` at $DIR/libc-read-and-uninit-premature-eof.rs:LL:CC + +note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace + +error: aborting due to 1 previous error + diff --git a/src/tools/miri/tests/pass-dep/libc/libc-fs.rs b/src/tools/miri/tests/pass-dep/libc/libc-fs.rs index 80c9757e9c95..f35229a205fb 100644 --- a/src/tools/miri/tests/pass-dep/libc/libc-fs.rs +++ b/src/tools/miri/tests/pass-dep/libc/libc-fs.rs @@ -35,6 +35,7 @@ fn main() { #[cfg(target_os = "linux")] test_sync_file_range(); test_isatty(); + test_read_and_uninit(); } fn test_file_open_unix_allow_two_args() { @@ -362,3 +363,37 @@ fn test_isatty() { remove_file(&path).unwrap(); } } + +fn test_read_and_uninit() { + use std::mem::MaybeUninit; + { + // We test that libc::read initializes its buffer. + let path = utils::prepare_with_content("pass-libc-read-and-uninit.txt", &[1u8, 2, 3]); + let cpath = CString::new(path.clone().into_os_string().into_encoded_bytes()).unwrap(); + unsafe { + let fd = libc::open(cpath.as_ptr(), libc::O_RDONLY); + assert_ne!(fd, -1); + let mut buf: MaybeUninit<[u8; 2]> = std::mem::MaybeUninit::uninit(); + assert_eq!(libc::read(fd, buf.as_mut_ptr().cast::(), 2), 2); + let buf = buf.assume_init(); + assert_eq!(buf, [1, 2]); + assert_eq!(libc::close(fd), 0); + } + remove_file(&path).unwrap(); + } + { + // We test that if we requested to read 4 bytes, but actually read 3 bytes, then + // 3 bytes (not 4) will be overwritten, and remaining byte will be left as-is. + let path = utils::prepare_with_content("pass-libc-read-and-uninit-2.txt", &[1u8, 2, 3]); + let cpath = CString::new(path.clone().into_os_string().into_encoded_bytes()).unwrap(); + unsafe { + let fd = libc::open(cpath.as_ptr(), libc::O_RDONLY); + assert_ne!(fd, -1); + let mut buf = [42u8; 5]; + assert_eq!(libc::read(fd, buf.as_mut_ptr().cast::(), 4), 3); + assert_eq!(buf, [1, 2, 3, 42, 42]); + assert_eq!(libc::close(fd), 0); + } + remove_file(&path).unwrap(); + } +}