From a1c6797c5c145f597e37dae53031babb0aaf7735 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 11 Oct 2019 05:30:50 -0500 Subject: [PATCH 1/4] Error when there is an unsupported flag --- src/shims/fs.rs | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 7e684489b5c..a6a1eb947b0 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -44,7 +44,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let mut options = OpenOptions::new(); - // The first two bits of the flag correspond to the access mode of the file in linux. + // The first two bits of the flag correspond to the access mode of the file in linux. This + // is done this way because `O_RDONLY` is zero in several platforms. let access_mode = flag & 0b11; if access_mode == this.eval_libc_i32("O_RDONLY")? { @@ -56,15 +57,35 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } else { throw_unsup_format!("Unsupported access mode {:#x}", access_mode); } + // We need to check that there aren't unsupported options in `flag`. For this we try to + // reproduce the content of `flag` in the `mirror` variable using only the supported + // options. + let mut mirror = access_mode; - if flag & this.eval_libc_i32("O_APPEND")? != 0 { + let o_append = this.eval_libc_i32("O_APPEND")?; + if flag & o_append != 0 { options.append(true); + mirror |= o_append; } - if flag & this.eval_libc_i32("O_TRUNC")? != 0 { + let o_trunc = this.eval_libc_i32("O_TRUNC")?; + if flag & o_trunc != 0 { options.truncate(true); + mirror |= o_trunc; } - if flag & this.eval_libc_i32("O_CREAT")? != 0 { + let o_creat = this.eval_libc_i32("O_CREAT")?; + if flag & o_creat != 0 { options.create(true); + mirror |= o_creat; + } + let o_cloexec = this.eval_libc_i32("O_CLOEXEC")?; + if flag & o_cloexec != 0 { + // This flag is a noop for now because `std` already sets it. + mirror |= o_cloexec; + } + // If `flag` is not equal to `mirror`, there is an unsupported option enabled in `flag`, + // then we throw an error. + if flag != mirror { + throw_unsup_format!("unsupported flags {:#x}", flag); } let path_bytes = this From d73fae1b28b3a12a5c22df71fc7be5fdb5f66e67 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Fri, 11 Oct 2019 12:17:54 -0500 Subject: [PATCH 2/4] Remove F_SETFD command --- src/shims/fs.rs | 26 ++++---------------------- 1 file changed, 4 insertions(+), 22 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index a6a1eb947b0..443a61c46e9 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -108,7 +108,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx &mut self, fd_op: OpTy<'tcx, Tag>, cmd_op: OpTy<'tcx, Tag>, - arg_op: Option>, + _arg_op: Option>, ) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); @@ -118,29 +118,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let fd = this.read_scalar(fd_op)?.to_i32()?; let cmd = this.read_scalar(cmd_op)?.to_i32()?; - - if cmd == this.eval_libc_i32("F_SETFD")? { - // This does not affect the file itself. Certain flags might require changing the file - // or the way it is accessed somehow. - let flag = this.read_scalar(arg_op.unwrap())?.to_i32()?; - // The only usage of this in stdlib at the moment is to enable the `FD_CLOEXEC` flag. - let fd_cloexec = this.eval_libc_i32("FD_CLOEXEC")?; - if let Some(FileHandle { flag: old_flag, .. }) = - this.machine.file_handler.handles.get_mut(&fd) - { - // Check that the only difference between the old flag and the current flag is - // exactly the `FD_CLOEXEC` value. - if flag ^ *old_flag == fd_cloexec { - *old_flag = flag; - } else { - throw_unsup_format!("Unsupported arg {:#x} for `F_SETFD`", flag); - } - } - Ok(0) - } else if cmd == this.eval_libc_i32("F_GETFD")? { + // We only support getting the flags for a descriptor + if cmd == this.eval_libc_i32("F_GETFD")? { this.get_handle_and(fd, |handle| Ok(handle.flag)) } else { - throw_unsup_format!("Unsupported command {:#x}", cmd); + throw_unsup_format!("The {:#x} command is not supported for `fcntl`)", cmd); } } From f76f8ce63bc160b127356c01624bbc8442844c22 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Sat, 12 Oct 2019 20:12:26 -0500 Subject: [PATCH 3/4] Correct fcntl behavior --- src/shims/fs.rs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 443a61c46e9..4d93ca4fd31 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -9,7 +9,6 @@ use crate::*; pub struct FileHandle { file: File, - flag: i32, } pub struct FileHandler { @@ -79,13 +78,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } let o_cloexec = this.eval_libc_i32("O_CLOEXEC")?; if flag & o_cloexec != 0 { - // This flag is a noop for now because `std` already sets it. + // We do not need to do anything for this flag because `std` already sets it. + // (Technically we do not support *not* setting this flag, but we ignore that.) mirror |= o_cloexec; } // If `flag` is not equal to `mirror`, there is an unsupported option enabled in `flag`, // then we throw an error. if flag != mirror { - throw_unsup_format!("unsupported flags {:#x}", flag); + throw_unsup_format!("unsupported flags {:#x}", flag & !mirror); } let path_bytes = this @@ -97,7 +97,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let fd = options.open(path).map(|file| { let mut fh = &mut this.machine.file_handler; fh.low += 1; - fh.handles.insert(fh.low, FileHandle { file, flag }); + fh.handles.insert(fh.low, FileHandle { file }); fh.low }); @@ -108,7 +108,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx &mut self, fd_op: OpTy<'tcx, Tag>, cmd_op: OpTy<'tcx, Tag>, - _arg_op: Option>, + _arg1_op: Option>, ) -> InterpResult<'tcx, i32> { let this = self.eval_context_mut(); @@ -120,7 +120,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let cmd = this.read_scalar(cmd_op)?.to_i32()?; // We only support getting the flags for a descriptor if cmd == this.eval_libc_i32("F_GETFD")? { - this.get_handle_and(fd, |handle| Ok(handle.flag)) + // Currently this is the only flag that `F_GETFD` returns. It is OK to just return the + // `FD_CLOEXEC` value without checking if the flag is set for the file because `std` + // always sets this flag when opening a file. However we still need to check that the + // file itself is open. + this.get_handle_and(fd, |_| Ok(0))?; + this.eval_libc_i32("FD_CLOEXEC") } else { throw_unsup_format!("The {:#x} command is not supported for `fcntl`)", cmd); } From 24872230dc25f40b5683549bdb1e288964744e77 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Mon, 14 Oct 2019 16:42:29 -0500 Subject: [PATCH 4/4] Check that access mode flags only use the first two bits --- src/shims/fs.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/shims/fs.rs b/src/shims/fs.rs index 4d93ca4fd31..c3dc5e60f0b 100644 --- a/src/shims/fs.rs +++ b/src/shims/fs.rs @@ -43,15 +43,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let mut options = OpenOptions::new(); - // The first two bits of the flag correspond to the access mode of the file in linux. This - // is done this way because `O_RDONLY` is zero in several platforms. + let o_rdonly = this.eval_libc_i32("O_RDONLY")?; + let o_wronly = this.eval_libc_i32("O_WRONLY")?; + let o_rdwr = this.eval_libc_i32("O_RDWR")?; + // The first two bits of the flag correspond to the access mode in linux, macOS and + // windows. We need to check that in fact the access mode flags for the current platform + // only use these two bits, otherwise we are in an unsupported platform and should error. + if (o_rdonly | o_wronly | o_rdwr) & !0b11 != 0 { + throw_unsup_format!("Access mode flags on this platform are unsupported"); + } + // Now we check the access mode let access_mode = flag & 0b11; - if access_mode == this.eval_libc_i32("O_RDONLY")? { + if access_mode == o_rdonly { options.read(true); - } else if access_mode == this.eval_libc_i32("O_WRONLY")? { + } else if access_mode == o_wronly { options.write(true); - } else if access_mode == this.eval_libc_i32("O_RDWR")? { + } else if access_mode == o_rdwr { options.read(true).write(true); } else { throw_unsup_format!("Unsupported access mode {:#x}", access_mode); @@ -124,8 +132,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx // `FD_CLOEXEC` value without checking if the flag is set for the file because `std` // always sets this flag when opening a file. However we still need to check that the // file itself is open. - this.get_handle_and(fd, |_| Ok(0))?; - this.eval_libc_i32("FD_CLOEXEC") + let fd_cloexec = this.eval_libc_i32("FD_CLOEXEC")?; + this.get_handle_and(fd, |_| Ok(fd_cloexec)) } else { throw_unsup_format!("The {:#x} command is not supported for `fcntl`)", cmd); }