From 8e97599af47ba3a1e4ed2185b8d0dddd9c69f0dd Mon Sep 17 00:00:00 2001 From: asquared31415 <34665709+asquared31415@users.noreply.github.com> Date: Sat, 5 Feb 2022 20:41:29 -0500 Subject: [PATCH] allow varargs for libc::open when it is allowed by the second argument --- src/shims/posix/foreign_items.rs | 5 +- src/shims/posix/fs.rs | 41 +++++++---- .../fs/unix_open_missing_required_mode.rs | 16 +++++ .../fs/unix_open_too_many_args.rs | 16 +++++ tests/run-pass/fs.rs | 69 +++++++++++++++---- 5 files changed, 117 insertions(+), 30 deletions(-) create mode 100644 tests/compile-fail/fs/unix_open_missing_required_mode.rs create mode 100644 tests/compile-fail/fs/unix_open_too_many_args.rs diff --git a/src/shims/posix/foreign_items.rs b/src/shims/posix/foreign_items.rs index 83b4032cd98..02fb7089c34 100644 --- a/src/shims/posix/foreign_items.rs +++ b/src/shims/posix/foreign_items.rs @@ -55,8 +55,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // File related shims "open" | "open64" => { - let &[ref path, ref flag, ref mode] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.open(path, flag, mode)?; + // `open` is variadic, the third argument is only present when the second argument has O_CREAT (or on linux O_TMPFILE, but miri doesn't support that) set + this.check_abi_and_shim_symbol_clash(abi, Abi::C { unwind: false }, link_name)?; + let result = this.open(args)?; this.write_scalar(Scalar::from_i32(result), dest)?; } "fcntl" => { diff --git a/src/shims/posix/fs.rs b/src/shims/posix/fs.rs index 36c07603770..7956cfe4dce 100644 --- a/src/shims/posix/fs.rs +++ b/src/shims/posix/fs.rs @@ -474,23 +474,18 @@ fn maybe_sync_file( impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { - fn open( - &mut self, - path_op: &OpTy<'tcx, Tag>, - flag_op: &OpTy<'tcx, Tag>, - mode_op: &OpTy<'tcx, Tag>, - ) -> InterpResult<'tcx, i32> { + fn open(&mut self, args: &[OpTy<'tcx, Tag>]) -> InterpResult<'tcx, i32> { + if args.len() < 2 || args.len() > 3 { + throw_ub_format!( + "incorrect number of arguments for `open`: got {}, expected 2 or 3", + args.len() + ); + } + let this = self.eval_context_mut(); - let flag = this.read_scalar(flag_op)?.to_i32()?; - - // Get the mode. On macOS, the argument type `mode_t` is actually `u16`, but - // C integer promotion rules mean that on the ABI level, it gets passed as `u32` - // (see https://github.com/rust-lang/rust/issues/71915). - let mode = this.read_scalar(mode_op)?.to_u32()?; - if mode != 0o666 { - throw_unsup_format!("non-default mode 0o{:o} is not supported", mode); - } + let path_op = &args[0]; + let flag = this.read_scalar(&args[1])?.to_i32()?; let mut options = OpenOptions::new(); @@ -535,6 +530,22 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } let o_creat = this.eval_libc_i32("O_CREAT")?; if flag & o_creat != 0 { + // Get the mode. On macOS, the argument type `mode_t` is actually `u16`, but + // C integer promotion rules mean that on the ABI level, it gets passed as `u32` + // (see https://github.com/rust-lang/rust/issues/71915). + let mode = if let Some(arg) = args.get(2) { + this.read_scalar(arg)?.to_u32()? + } else { + throw_ub_format!( + "incorrect number of arguments for `open` with `O_CREAT`: got {}, expected 3", + args.len() + ); + }; + + if mode != 0o666 { + throw_unsup_format!("non-default mode 0o{:o} is not supported", mode); + } + mirror |= o_creat; let o_excl = this.eval_libc_i32("O_EXCL")?; diff --git a/tests/compile-fail/fs/unix_open_missing_required_mode.rs b/tests/compile-fail/fs/unix_open_missing_required_mode.rs new file mode 100644 index 00000000000..2bac7291254 --- /dev/null +++ b/tests/compile-fail/fs/unix_open_missing_required_mode.rs @@ -0,0 +1,16 @@ +// ignore-windows: No libc on Windows +// compile-flags: -Zmiri-disable-isolation + +#![feature(rustc_private)] + +extern crate libc; + +fn main() { + test_file_open_missing_needed_mode(); +} + +fn test_file_open_missing_needed_mode() { + let name = b"missing_arg.txt\0"; + let name_ptr = name.as_ptr().cast::(); + let _fd = unsafe { libc::open(name_ptr, libc::O_CREAT) }; //~ ERROR Undefined Behavior: incorrect number of arguments for `open` with `O_CREAT`: got 2, expected 3 +} diff --git a/tests/compile-fail/fs/unix_open_too_many_args.rs b/tests/compile-fail/fs/unix_open_too_many_args.rs new file mode 100644 index 00000000000..9df7415d313 --- /dev/null +++ b/tests/compile-fail/fs/unix_open_too_many_args.rs @@ -0,0 +1,16 @@ +// ignore-windows: No libc on Windows +// compile-flags: -Zmiri-disable-isolation + +#![feature(rustc_private)] + +extern crate libc; + +fn main() { + test_open_too_many_args(); +} + +fn test_open_too_many_args() { + let name = b"too_many_args.txt\0"; + let name_ptr = name.as_ptr().cast::(); + let _fd = unsafe { libc::open(name_ptr, libc::O_RDONLY, 0, 0) }; //~ ERROR Undefined Behavior: incorrect number of arguments for `open`: got 4, expected 2 or 3 +} diff --git a/tests/run-pass/fs.rs b/tests/run-pass/fs.rs index 568b7ab4e3b..9ab1d4c0338 100644 --- a/tests/run-pass/fs.rs +++ b/tests/run-pass/fs.rs @@ -5,13 +5,10 @@ extern crate libc; -use std::fs::{ - File, create_dir, OpenOptions, remove_dir, remove_dir_all, remove_file, rename, -}; use std::ffi::CString; -use std::io::{Read, Write, Error, ErrorKind, Result, Seek, SeekFrom}; -use std::path::{PathBuf, Path}; - +use std::fs::{create_dir, remove_dir, remove_dir_all, remove_file, rename, File, OpenOptions}; +use std::io::{Error, ErrorKind, Read, Result, Seek, SeekFrom, Write}; +use std::path::{Path, PathBuf}; fn main() { test_file(); @@ -26,6 +23,11 @@ fn main() { test_rename(); test_directory(); test_dup_stdout_stderr(); + + // These all require unix, if the test is changed to no longer `ignore-windows`, move these to a unix test + test_file_open_unix_allow_two_args(); + test_file_open_unix_needs_three_args(); + test_file_open_unix_extra_third_arg(); } fn tmp() -> PathBuf { @@ -41,7 +43,8 @@ fn tmp() -> PathBuf { #[cfg(not(windows))] return PathBuf::from(tmp.replace("\\", "/")); - }).unwrap_or_else(|_| std::env::temp_dir()) + }) + .unwrap_or_else(|_| std::env::temp_dir()) } /// Prepare: compute filename and make sure the file does not exist. @@ -93,6 +96,39 @@ fn test_file() { remove_file(&path).unwrap(); } +fn test_file_open_unix_allow_two_args() { + use std::os::unix::ffi::OsStrExt; + + let path = prepare_with_content("test_file_open_unix_allow_two_args.txt", &[]); + + let mut name = path.into_os_string(); + name.push("\0"); + let name_ptr = name.as_bytes().as_ptr().cast::(); + let _fd = unsafe { libc::open(name_ptr, libc::O_RDONLY) }; +} + +fn test_file_open_unix_needs_three_args() { + use std::os::unix::ffi::OsStrExt; + + let path = prepare_with_content("test_file_open_unix_needs_three_args.txt", &[]); + + let mut name = path.into_os_string(); + name.push("\0"); + let name_ptr = name.as_bytes().as_ptr().cast::(); + let _fd = unsafe { libc::open(name_ptr, libc::O_CREAT, 0o666) }; +} + +fn test_file_open_unix_extra_third_arg() { + use std::os::unix::ffi::OsStrExt; + + let path = prepare_with_content("test_file_open_unix_extra_third_arg.txt", &[]); + + let mut name = path.into_os_string(); + name.push("\0"); + let name_ptr = name.as_bytes().as_ptr().cast::(); + let _fd = unsafe { libc::open(name_ptr, libc::O_RDONLY, 42) }; +} + fn test_file_clone() { let bytes = b"Hello, World!\n"; let path = prepare_with_content("miri_test_fs_file_clone.txt", bytes); @@ -115,7 +151,10 @@ fn test_file_create_new() { // Creating a new file that doesn't yet exist should succeed. OpenOptions::new().write(true).create_new(true).open(&path).unwrap(); // Creating a new file that already exists should fail. - assert_eq!(ErrorKind::AlreadyExists, OpenOptions::new().write(true).create_new(true).open(&path).unwrap_err().kind()); + assert_eq!( + ErrorKind::AlreadyExists, + OpenOptions::new().write(true).create_new(true).open(&path).unwrap_err().kind() + ); // Optionally creating a new file that already exists should succeed. OpenOptions::new().write(true).create(true).open(&path).unwrap(); @@ -235,7 +274,6 @@ fn test_symlink() { symlink_file.read_to_end(&mut contents).unwrap(); assert_eq!(bytes, contents.as_slice()); - #[cfg(unix)] { use std::os::unix::ffi::OsStrExt; @@ -250,7 +288,9 @@ fn test_symlink() { // Make the buf one byte larger than it needs to be, // and check that the last byte is not overwritten. let mut large_buf = vec![0xFF; expected_path.len() + 1]; - let res = unsafe { libc::readlink(symlink_c_ptr, large_buf.as_mut_ptr().cast(), large_buf.len()) }; + let res = unsafe { + libc::readlink(symlink_c_ptr, large_buf.as_mut_ptr().cast(), large_buf.len()) + }; // Check that the resovled path was properly written into the buf. assert_eq!(&large_buf[..(large_buf.len() - 1)], expected_path); assert_eq!(large_buf.last(), Some(&0xFF)); @@ -259,18 +299,21 @@ fn test_symlink() { // Test that the resolved path is truncated if the provided buffer // is too small. let mut small_buf = [0u8; 2]; - let res = unsafe { libc::readlink(symlink_c_ptr, small_buf.as_mut_ptr().cast(), small_buf.len()) }; + let res = unsafe { + libc::readlink(symlink_c_ptr, small_buf.as_mut_ptr().cast(), small_buf.len()) + }; assert_eq!(small_buf, &expected_path[..small_buf.len()]); assert_eq!(res, small_buf.len() as isize); // Test that we report a proper error for a missing path. let bad_path = CString::new("MIRI_MISSING_FILE_NAME").unwrap(); - let res = unsafe { libc::readlink(bad_path.as_ptr(), small_buf.as_mut_ptr().cast(), small_buf.len()) }; + let res = unsafe { + libc::readlink(bad_path.as_ptr(), small_buf.as_mut_ptr().cast(), small_buf.len()) + }; assert_eq!(res, -1); assert_eq!(Error::last_os_error().kind(), ErrorKind::NotFound); } - // Test that metadata of a symbolic link is correct. check_metadata(bytes, &symlink_path).unwrap(); // Test that the metadata of a symbolic link is correct when not following it.