Auto merge of #1970 - asquared31415:open_unix_varargs, r=RalfJung

Allow varargs for libc::open when it is allowed by the second argument

This PR allows `libc::open` to be called using two or three arguments as defined in https://man7.org/linux/man-pages/man2/open.2.html

The presence of the third argument depends on the value of the second argument.  If the second argument dictates that the third argument is *required* miri will emit an error if the argument is missing.  If the second argument does *not* require a third argument, then the argument is ignored and passed as 0 internally (it would be ignored by libc anyway)

Author: bors

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" => {

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> {
-        let this = self.eval_context_mut();
+    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 flag = this.read_scalar(flag_op)?.to_i32()?;
+        let this = self.eval_context_mut();
 
-        // 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")?;

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::<libc::c_char>();
+    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
+}

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::<libc::c_char>();
+    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
+}

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::<libc::c_char>();
+    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::<libc::c_char>();
+    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::<libc::c_char>();
+    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.