From 0088715411c2fcb3ea36cdbb969fb9966d722320 Mon Sep 17 00:00:00 2001 From: Tavian Barnes Date: Mon, 7 Mar 2022 12:46:53 -0500 Subject: [PATCH 1/2] Rename MiriMemoryKind::Env to Runtime In preparation to use it for other runtime-internal allocations. --- src/data_race.rs | 2 +- src/machine.rs | 9 +++++---- src/shims/env.rs | 25 +++++++++++++------------ src/stacked_borrows.rs | 2 +- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/data_race.rs b/src/data_race.rs index a6ef56a0c2..1e91c66b85 100644 --- a/src/data_race.rs +++ b/src/data_race.rs @@ -743,7 +743,7 @@ impl VClockAlloc { MemoryKind::Machine( MiriMemoryKind::Global | MiriMemoryKind::Machine - | MiriMemoryKind::Env + | MiriMemoryKind::Runtime | MiriMemoryKind::ExternStatic | MiriMemoryKind::Tls, ) diff --git a/src/machine.rs b/src/machine.rs index 481808dc78..bb43cb9550 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -71,8 +71,9 @@ pub enum MiriMemoryKind { /// Memory for args, errno, and other parts of the machine-managed environment. /// This memory may leak. Machine, - /// Memory for env vars. Separate from `Machine` because we clean it up and leak-check it. - Env, + /// Memory allocated by the runtime (e.g. env vars). Separate from `Machine` + /// because we clean it up and leak-check it. + Runtime, /// Globals copied from `tcx`. /// This memory may leak. Global, @@ -96,7 +97,7 @@ impl MayLeak for MiriMemoryKind { fn may_leak(self) -> bool { use self::MiriMemoryKind::*; match self { - Rust | C | WinHeap | Env => false, + Rust | C | WinHeap | Runtime => false, Machine | Global | ExternStatic | Tls => true, } } @@ -110,7 +111,7 @@ impl fmt::Display for MiriMemoryKind { C => write!(f, "C heap"), WinHeap => write!(f, "Windows heap"), Machine => write!(f, "machine-managed memory"), - Env => write!(f, "environment variable"), + Runtime => write!(f, "language runtime memory"), Global => write!(f, "global (static or const)"), ExternStatic => write!(f, "extern static"), Tls => write!(f, "thread-local static"), diff --git a/src/shims/env.rs b/src/shims/env.rs index dfd1ef207d..fd77286885 100644 --- a/src/shims/env.rs +++ b/src/shims/env.rs @@ -78,12 +78,12 @@ impl<'tcx> EnvVars<'tcx> { ) -> InterpResult<'tcx> { // Deallocate individual env vars. for (_name, ptr) in ecx.machine.env_vars.map.drain() { - ecx.memory.deallocate(ptr, None, MiriMemoryKind::Env.into())?; + ecx.memory.deallocate(ptr, None, MiriMemoryKind::Runtime.into())?; } // Deallocate environ var list. let environ = ecx.machine.env_vars.environ.unwrap(); let old_vars_ptr = ecx.read_pointer(&environ.into())?; - ecx.memory.deallocate(old_vars_ptr, None, MiriMemoryKind::Env.into())?; + ecx.memory.deallocate(old_vars_ptr, None, MiriMemoryKind::Runtime.into())?; Ok(()) } } @@ -96,7 +96,7 @@ fn alloc_env_var_as_c_str<'mir, 'tcx>( let mut name_osstring = name.to_os_string(); name_osstring.push("="); name_osstring.push(value); - ecx.alloc_os_str_as_c_str(name_osstring.as_os_str(), MiriMemoryKind::Env.into()) + ecx.alloc_os_str_as_c_str(name_osstring.as_os_str(), MiriMemoryKind::Runtime.into()) } fn alloc_env_var_as_wide_str<'mir, 'tcx>( @@ -107,7 +107,7 @@ fn alloc_env_var_as_wide_str<'mir, 'tcx>( let mut name_osstring = name.to_os_string(); name_osstring.push("="); name_osstring.push(value); - ecx.alloc_os_str_as_wide_str(name_osstring.as_os_str(), MiriMemoryKind::Env.into()) + ecx.alloc_os_str_as_wide_str(name_osstring.as_os_str(), MiriMemoryKind::Runtime.into()) } impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} @@ -186,7 +186,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } // Allocate environment block & Store environment variables to environment block. // Final null terminator(block terminator) is added by `alloc_os_str_to_wide_str`. - let envblock_ptr = this.alloc_os_str_as_wide_str(&env_vars, MiriMemoryKind::Env.into())?; + let envblock_ptr = + this.alloc_os_str_as_wide_str(&env_vars, MiriMemoryKind::Runtime.into())?; // If the function succeeds, the return value is a pointer to the environment block of the current process. Ok(envblock_ptr) } @@ -200,7 +201,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.assert_target_os("windows", "FreeEnvironmentStringsW"); let env_block_ptr = this.read_pointer(env_block_op)?; - let result = this.memory.deallocate(env_block_ptr, None, MiriMemoryKind::Env.into()); + let result = this.memory.deallocate(env_block_ptr, None, MiriMemoryKind::Runtime.into()); // If the function succeeds, the return value is nonzero. Ok(result.is_ok() as i32) } @@ -231,7 +232,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx if let Some((name, value)) = new { let var_ptr = alloc_env_var_as_c_str(&name, &value, &mut this)?; if let Some(var) = this.machine.env_vars.map.insert(name, var_ptr) { - this.memory.deallocate(var, None, MiriMemoryKind::Env.into())?; + this.memory.deallocate(var, None, MiriMemoryKind::Runtime.into())?; } this.update_environ()?; Ok(0) // return zero on success @@ -268,7 +269,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } else if this.ptr_is_null(value_ptr)? { // Delete environment variable `{name}` if let Some(var) = this.machine.env_vars.map.remove(&name) { - this.memory.deallocate(var, None, MiriMemoryKind::Env.into())?; + this.memory.deallocate(var, None, MiriMemoryKind::Runtime.into())?; this.update_environ()?; } Ok(1) // return non-zero on success @@ -276,7 +277,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let value = this.read_os_str_from_wide_str(value_ptr)?; let var_ptr = alloc_env_var_as_wide_str(&name, &value, &mut this)?; if let Some(var) = this.machine.env_vars.map.insert(name, var_ptr) { - this.memory.deallocate(var, None, MiriMemoryKind::Env.into())?; + this.memory.deallocate(var, None, MiriMemoryKind::Runtime.into())?; } this.update_environ()?; Ok(1) // return non-zero on success @@ -301,7 +302,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } if let Some(old) = success { if let Some(var) = old { - this.memory.deallocate(var, None, MiriMemoryKind::Env.into())?; + this.memory.deallocate(var, None, MiriMemoryKind::Runtime.into())?; } this.update_environ()?; Ok(0) @@ -437,7 +438,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // Deallocate the old environ list, if any. if let Some(environ) = this.machine.env_vars.environ { let old_vars_ptr = this.read_pointer(&environ.into())?; - this.memory.deallocate(old_vars_ptr, None, MiriMemoryKind::Env.into())?; + this.memory.deallocate(old_vars_ptr, None, MiriMemoryKind::Runtime.into())?; } else { // No `environ` allocated yet, let's do that. // This is memory backing an extern static, hence `ExternStatic`, not `Env`. @@ -455,7 +456,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let tcx = this.tcx; let vars_layout = this.layout_of(tcx.mk_array(tcx.types.usize, u64::try_from(vars.len()).unwrap()))?; - let vars_place = this.allocate(vars_layout, MiriMemoryKind::Env.into())?; + let vars_place = this.allocate(vars_layout, MiriMemoryKind::Runtime.into())?; for (idx, var) in vars.into_iter().enumerate() { let place = this.mplace_field(&vars_place, idx)?; this.write_pointer(var, &place.into())?; diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 37ecb749a4..0e47a9e1c3 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -535,7 +535,7 @@ impl Stacks { MiriMemoryKind::Global | MiriMemoryKind::ExternStatic | MiriMemoryKind::Tls - | MiriMemoryKind::Env + | MiriMemoryKind::Runtime | MiriMemoryKind::Machine, ) => (extra.base_tag(id), Permission::SharedReadWrite), // Heap allocations we only track precisely when raw pointers are tagged, for now. From 0886419524c5baf61d6f3980e81ba4e0429ac402 Mon Sep 17 00:00:00 2001 From: Tavian Barnes Date: Tue, 22 Feb 2022 17:06:05 -0500 Subject: [PATCH 2/2] Implement a readdir64() shim for Linux Partial fix for #1966. --- src/shims/posix/fs.rs | 120 ++++++++++++++----------- src/shims/posix/linux/foreign_items.rs | 8 +- tests/run-pass/fs.rs | 12 +-- 3 files changed, 76 insertions(+), 64 deletions(-) diff --git a/src/shims/posix/fs.rs b/src/shims/posix/fs.rs index 7956cfe4dc..300e3c514b 100644 --- a/src/shims/posix/fs.rs +++ b/src/shims/posix/fs.rs @@ -16,6 +16,7 @@ use rustc_target::abi::{Align, Size}; use crate::*; use helpers::{check_arg_count, immty_from_int_checked, immty_from_uint_checked}; +use shims::os_str::os_str_to_bytes; use shims::time::system_time_to_duration; #[derive(Debug)] @@ -421,6 +422,22 @@ trait EvalContextExtPrivate<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, ' } } +/// An open directory, tracked by DirHandler. +#[derive(Debug)] +pub struct OpenDir { + /// The directory reader on the host. + read_dir: ReadDir, + /// The most recent entry returned by readdir() + entry: Pointer>, +} + +impl OpenDir { + fn new(read_dir: ReadDir) -> Self { + // We rely on `free` being a NOP on null pointers. + Self { read_dir, entry: Pointer::null() } + } +} + #[derive(Debug)] pub struct DirHandler { /// Directory iterators used to emulate libc "directory streams", as used in opendir, readdir, @@ -432,7 +449,7 @@ pub struct DirHandler { /// the corresponding ReadDir iterator from this map, and information from the next /// directory entry is returned. When closedir is called, the ReadDir iterator is removed from /// the map. - streams: FxHashMap, + streams: FxHashMap, /// ID number to be used by the next call to opendir next_id: u64, } @@ -441,7 +458,7 @@ impl DirHandler { fn insert_new(&mut self, read_dir: ReadDir) -> u64 { let id = self.next_id; self.next_id += 1; - self.streams.try_insert(id, read_dir).unwrap(); + self.streams.try_insert(id, OpenDir::new(read_dir)).unwrap(); id } } @@ -1207,32 +1224,29 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } - fn linux_readdir64_r( - &mut self, - dirp_op: &OpTy<'tcx, Tag>, - entry_op: &OpTy<'tcx, Tag>, - result_op: &OpTy<'tcx, Tag>, - ) -> InterpResult<'tcx, i32> { + fn linux_readdir64(&mut self, dirp_op: &OpTy<'tcx, Tag>) -> InterpResult<'tcx, Scalar> { let this = self.eval_context_mut(); - this.assert_target_os("linux", "readdir64_r"); + this.assert_target_os("linux", "readdir64"); let dirp = this.read_scalar(dirp_op)?.to_machine_usize(this)?; // Reject if isolation is enabled. if let IsolatedOp::Reject(reject_with) = this.machine.isolated_op { - this.reject_in_isolation("`readdir64_r`", reject_with)?; - // Set error code as "EBADF" (bad fd) - return this.handle_not_found(); + this.reject_in_isolation("`readdir`", reject_with)?; + let eacc = this.eval_libc("EBADF")?; + this.set_last_error(eacc)?; + return Ok(Scalar::null_ptr(this)); } - let dir_iter = this.machine.dir_handler.streams.get_mut(&dirp).ok_or_else(|| { - err_unsup_format!("the DIR pointer passed to readdir64_r did not come from opendir") + let open_dir = this.machine.dir_handler.streams.get_mut(&dirp).ok_or_else(|| { + err_unsup_format!("the DIR pointer passed to readdir64 did not come from opendir") })?; - match dir_iter.next() { + + let entry = match open_dir.read_dir.next() { Some(Ok(dir_entry)) => { - // Write into entry, write pointer to result, return 0 on success. - // The name is written with write_os_str_to_c_str, while the rest of the + // Write the directory entry into a newly allocated buffer. + // The name is written with write_bytes, while the rest of the // dirent64 struct is written using write_packed_immediates. // For reference: @@ -1244,22 +1258,19 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // pub d_name: [c_char; 256], // } - let entry_place = this.deref_operand(entry_op)?; - let name_place = this.mplace_field(&entry_place, 4)?; + let mut name = dir_entry.file_name(); // not a Path as there are no separators! + name.push("\0"); // Add a NUL terminator + let name_bytes = os_str_to_bytes(&name)?; + let name_len = u64::try_from(name_bytes.len()).unwrap(); - let file_name = dir_entry.file_name(); // not a Path as there are no separators! - let (name_fits, _) = this.write_os_str_to_c_str( - &file_name, - name_place.ptr, - name_place.layout.size.bytes(), - )?; - if !name_fits { - throw_unsup_format!( - "a directory entry had a name too large to fit in libc::dirent64" - ); - } + let dirent64_layout = this.libc_ty_layout("dirent64")?; + let d_name_offset = dirent64_layout.fields.offset(4 /* d_name */).bytes(); + let size = d_name_offset.checked_add(name_len).unwrap(); - let entry_place = this.deref_operand(entry_op)?; + let entry = + this.malloc(size, /*zero_init:*/ false, MiriMemoryKind::Runtime)?; + + // FIXME: make use of dirent64_layout let ino64_t_layout = this.libc_ty_layout("ino64_t")?; let off64_t_layout = this.libc_ty_layout("off64_t")?; let c_ushort_layout = this.libc_ty_layout("c_ushort")?; @@ -1277,33 +1288,33 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let imms = [ immty_from_uint_checked(ino, ino64_t_layout)?, // d_ino immty_from_uint_checked(0u128, off64_t_layout)?, // d_off - immty_from_uint_checked(0u128, c_ushort_layout)?, // d_reclen + immty_from_uint_checked(size, c_ushort_layout)?, // d_reclen immty_from_int_checked(file_type, c_uchar_layout)?, // d_type ]; + let entry_layout = this.layout_of(this.tcx.mk_array(this.tcx.types.u8, size))?; + let entry_place = MPlaceTy::from_aligned_ptr(entry, entry_layout); this.write_packed_immediates(&entry_place, &imms)?; - let result_place = this.deref_operand(result_op)?; - this.write_scalar(this.read_scalar(entry_op)?, &result_place.into())?; + let name_ptr = entry.offset(Size::from_bytes(d_name_offset), this)?; + this.memory.write_bytes(name_ptr, name_bytes.iter().copied())?; - Ok(0) + entry } None => { - // end of stream: return 0, assign *result=NULL - this.write_null(&this.deref_operand(result_op)?.into())?; - Ok(0) + // end of stream: return NULL + Pointer::null() } - Some(Err(e)) => - match e.raw_os_error() { - // return positive error number on error - Some(error) => Ok(error), - None => { - throw_unsup_format!( - "the error {} couldn't be converted to a return value", - e - ) - } - }, - } + Some(Err(e)) => { + this.set_last_error_from_io_error(e.kind())?; + Pointer::null() + } + }; + + let open_dir = this.machine.dir_handler.streams.get_mut(&dirp).unwrap(); + let old_entry = std::mem::replace(&mut open_dir.entry, entry); + this.free(old_entry, MiriMemoryKind::Runtime)?; + + Ok(Scalar::from_maybe_pointer(entry, this)) } fn macos_readdir_r( @@ -1325,10 +1336,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx return this.handle_not_found(); } - let dir_iter = this.machine.dir_handler.streams.get_mut(&dirp).ok_or_else(|| { + let open_dir = this.machine.dir_handler.streams.get_mut(&dirp).ok_or_else(|| { err_unsup_format!("the DIR pointer passed to readdir_r did not come from opendir") })?; - match dir_iter.next() { + match open_dir.read_dir.next() { Some(Ok(dir_entry)) => { // Write into entry, write pointer to result, return 0 on success. // The name is written with write_os_str_to_c_str, while the rest of the @@ -1419,8 +1430,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx return this.handle_not_found(); } - if let Some(dir_iter) = this.machine.dir_handler.streams.remove(&dirp) { - drop(dir_iter); + if let Some(open_dir) = this.machine.dir_handler.streams.remove(&dirp) { + this.free(open_dir.entry, MiriMemoryKind::Runtime)?; + drop(open_dir); Ok(0) } else { this.handle_not_found() diff --git a/src/shims/posix/linux/foreign_items.rs b/src/shims/posix/linux/foreign_items.rs index 8d0f8487f5..280f24e9ea 100644 --- a/src/shims/posix/linux/foreign_items.rs +++ b/src/shims/posix/linux/foreign_items.rs @@ -43,11 +43,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let result = this.opendir(name)?; this.write_scalar(result, dest)?; } - "readdir64_r" => { - let &[ref dirp, ref entry, ref result] = + "readdir64" => { + let &[ref dirp] = this.check_shim(abi, Abi::C { unwind: false }, link_name, args)?; - let result = this.linux_readdir64_r(dirp, entry, result)?; - this.write_scalar(Scalar::from_i32(result), dest)?; + let result = this.linux_readdir64(dirp)?; + this.write_scalar(result, dest)?; } "ftruncate64" => { let &[ref fd, ref length] = diff --git a/tests/run-pass/fs.rs b/tests/run-pass/fs.rs index 9ab1d4c033..be680131f8 100644 --- a/tests/run-pass/fs.rs +++ b/tests/run-pass/fs.rs @@ -6,7 +6,9 @@ extern crate libc; use std::ffi::CString; -use std::fs::{create_dir, remove_dir, remove_dir_all, remove_file, rename, File, OpenOptions}; +use std::fs::{ + create_dir, read_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}; @@ -374,19 +376,17 @@ fn test_directory() { let path_2 = dir_path.join("test_file_2"); drop(File::create(&path_2).unwrap()); // Test that the files are present inside the directory - /* FIXME(1966) disabled due to missing readdir support let dir_iter = read_dir(&dir_path).unwrap(); let mut file_names = dir_iter.map(|e| e.unwrap().file_name()).collect::>(); file_names.sort_unstable(); - assert_eq!(file_names, vec!["test_file_1", "test_file_2"]); */ + assert_eq!(file_names, vec!["test_file_1", "test_file_2"]); // Clean up the files in the directory remove_file(&path_1).unwrap(); remove_file(&path_2).unwrap(); // Now there should be nothing left in the directory. - /* FIXME(1966) disabled due to missing readdir support - dir_iter = read_dir(&dir_path).unwrap(); + let dir_iter = read_dir(&dir_path).unwrap(); let file_names = dir_iter.map(|e| e.unwrap().file_name()).collect::>(); - assert!(file_names.is_empty());*/ + assert!(file_names.is_empty()); // Deleting the directory should succeed. remove_dir(&dir_path).unwrap();