From cd495cb04f10eb2149f3a57bc0310af4c8c3ac3b Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Mon, 30 Sep 2019 11:46:07 -0500 Subject: [PATCH 1/5] Add file writing capabilities --- src/shims/foreign_items.rs | 4 +-- src/shims/io.rs | 62 +++++++++++++++++++++++++++++++++---- tests/hello.txt | 1 - tests/run-pass/file_read.rs | 10 ++++-- 4 files changed, 65 insertions(+), 12 deletions(-) delete mode 100644 tests/hello.txt diff --git a/src/shims/foreign_items.rs b/src/shims/foreign_items.rs index b8609fd2ef..847169f38d 100644 --- a/src/shims/foreign_items.rs +++ b/src/shims/foreign_items.rs @@ -494,9 +494,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Err(_) => -1, } } else { - eprintln!("Miri: Ignored output to FD {}", fd); - // Pretend it all went well. - n as i64 + this.write(args[0], args[1], args[2])? }; // Now, `result` is the value we return back to the program. this.write_scalar( diff --git a/src/shims/io.rs b/src/shims/io.rs index 0d1adcce65..afb6e6311e 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; -use std::fs::File; -use std::io::Read; +use std::fs::{ File, OpenOptions }; +use std::io::{ Read, Write }; use rustc::ty::layout::Size; @@ -42,8 +42,29 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let flag = this.read_scalar(flag_op)?.to_i32()?; - if flag != this.eval_libc_i32("O_RDONLY")? && flag != this.eval_libc_i32("O_CLOEXEC")? { - throw_unsup_format!("Unsupported flag {:#x}", flag); + let mut options = OpenOptions::new(); + + // The first two bits of the flag correspond to the access mode of the file in linux. + let access_mode = flag & 0b11; + + if access_mode == this.eval_libc_i32("O_RDONLY")? { + options.read(true); + } else if access_mode == this.eval_libc_i32("O_WRONLY")? { + options.write(true); + } else if access_mode == this.eval_libc_i32("O_RDWR")? { + options.read(true).write(true); + } else { + throw_unsup_format!("Unsupported access mode {:#x}", access_mode); + } + + if flag & this.eval_libc_i32("O_APPEND")? != 0 { + options.append(true); + } + if flag & this.eval_libc_i32("O_TRUNC")? != 0 { + options.truncate(true); + } + if flag & this.eval_libc_i32("O_CREAT")? != 0 { + options.create(true); } let path_bytes = this @@ -51,7 +72,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx .read_c_str(this.read_scalar(path_op)?.not_undef()?)?; let path = std::str::from_utf8(path_bytes) .map_err(|_| err_unsup_format!("{:?} is not a valid utf-8 string", path_bytes))?; - let fd = File::open(path).map(|file| { + + 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 }); @@ -151,8 +173,36 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx ) } + fn write( + &mut self, + fd_op: OpTy<'tcx, Tag>, + buf_op: OpTy<'tcx, Tag>, + count_op: OpTy<'tcx, Tag>, + ) -> InterpResult<'tcx, i64> { + let this = self.eval_context_mut(); + + if !this.machine.communicate { + throw_unsup_format!("`write` not available when isolation is enabled") + } + + let tcx = &{ this.tcx.tcx }; + + let fd = this.read_scalar(fd_op)?.to_i32()?; + let buf = this.force_ptr(this.read_scalar(buf_op)?.not_undef()?)?; + let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; + + // `to_vec` is needed to avoid borrowing issues when writing to the file. + let bytes = this.memory().get(buf.alloc_id)?.get_bytes(tcx, buf, Size::from_bytes(count))?.to_vec(); + + this.remove_handle_and(fd, |mut handle, this| { + let bytes = handle.file.write(&bytes).map(|bytes| bytes as i64); + this.machine.file_handler.handles.insert(fd, handle); + this.consume_result(bytes) + }) + } + /// Helper function that gets a `FileHandle` immutable reference and allows to manipulate it - /// using `f`. + /// using the `f` closure. /// /// If the `fd` file descriptor does not corresponds to a file, this functions returns `Ok(-1)` /// and sets `Evaluator::last_error` to `libc::EBADF` (invalid file descriptor). diff --git a/tests/hello.txt b/tests/hello.txt deleted file mode 100644 index 8ab686eafe..0000000000 --- a/tests/hello.txt +++ /dev/null @@ -1 +0,0 @@ -Hello, World! diff --git a/tests/run-pass/file_read.rs b/tests/run-pass/file_read.rs index a60640d1b3..b0f2618920 100644 --- a/tests/run-pass/file_read.rs +++ b/tests/run-pass/file_read.rs @@ -2,10 +2,16 @@ // compile-flags: -Zmiri-disable-isolation use std::fs::File; -use std::io::Read; +use std::io::{ Read, Write }; fn main() { - // FIXME: create the file and delete it when `rm` is implemented. + // FIXME: remove the file and delete it when `rm` is implemented. + + // Test creating, writing and closing a file (closing is tested when `file` is dropped). + let mut file = File::create("./tests/hello.txt").unwrap(); + file.write(b"Hello, World!\n").unwrap(); + + // Test opening, reading and closing a file. let mut file = File::open("./tests/hello.txt").unwrap(); let mut contents = String::new(); file.read_to_string(&mut contents).unwrap(); From 5a05c04c857538a60cd006455686124162f736dc Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Tue, 1 Oct 2019 10:57:12 -0500 Subject: [PATCH 2/5] Correct name of each shim when erroring --- src/shims/io.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/shims/io.rs b/src/shims/io.rs index afb6e6311e..9f08141eb3 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -92,7 +92,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let this = self.eval_context_mut(); if !this.machine.communicate { - throw_unsup_format!("`open` not available when isolation is enabled") + throw_unsup_format!("`fcntl` not available when isolation is enabled") } let fd = this.read_scalar(fd_op)?.to_i32()?; @@ -125,7 +125,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let this = self.eval_context_mut(); if !this.machine.communicate { - throw_unsup_format!("`open` not available when isolation is enabled") + throw_unsup_format!("`close` not available when isolation is enabled") } let fd = this.read_scalar(fd_op)?.to_i32()?; @@ -145,7 +145,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let this = self.eval_context_mut(); if !this.machine.communicate { - throw_unsup_format!("`open` not available when isolation is enabled") + throw_unsup_format!("`read` not available when isolation is enabled") } let tcx = &{ this.tcx.tcx }; @@ -204,7 +204,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx /// Helper function that gets a `FileHandle` immutable reference and allows to manipulate it /// using the `f` closure. /// - /// If the `fd` file descriptor does not corresponds to a file, this functions returns `Ok(-1)` + /// If the `fd` file descriptor does not correspond to a file, this functions returns `Ok(-1)` /// and sets `Evaluator::last_error` to `libc::EBADF` (invalid file descriptor). /// /// This function uses `T: From` instead of `i32` directly because some IO related @@ -227,7 +227,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx /// to modify `MiriEvalContext` at the same time, so you can modify the handle and reinsert it /// using `f`. /// - /// If the `fd` file descriptor does not corresponds to a file, this functions returns `Ok(-1)` + /// If the `fd` file descriptor does not correspond to a file, this functions returns `Ok(-1)` /// and sets `Evaluator::last_error` to `libc::EBADF` (invalid file descriptor). /// /// This function uses `T: From` instead of `i32` directly because some IO related From 78e0d309efe507afab2e71a2e3a0f8bec1cd4f1a Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Tue, 1 Oct 2019 13:48:59 -0500 Subject: [PATCH 3/5] Avoid early return after handles are removed --- src/shims/io.rs | 55 ++++++++++++++++++++++++------------------------- 1 file changed, 27 insertions(+), 28 deletions(-) diff --git a/src/shims/io.rs b/src/shims/io.rs index 9f08141eb3..49ad06e0df 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; -use std::fs::{ File, OpenOptions }; -use std::io::{ Read, Write }; +use std::fs::{File, OpenOptions}; +use std::io::{Read, Write}; use rustc::ty::layout::Size; @@ -130,10 +130,9 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let fd = this.read_scalar(fd_op)?.to_i32()?; - this.remove_handle_and( - fd, - |handle, this| this.consume_result(handle.file.sync_all().map(|_| 0i32)), - ) + this.remove_handle_and(fd, |handle, this| { + this.consume_result(handle.file.sync_all().map(|_| 0i32)) + }) } fn read( @@ -155,22 +154,18 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; // Remove the file handle to avoid borrowing issues - this.remove_handle_and( - fd, - |mut handle, this| { - let bytes = handle - .file - .read(this.memory_mut().get_mut(buf.alloc_id)?.get_bytes_mut( - tcx, - buf, - Size::from_bytes(count), - )?) - .map(|bytes| bytes as i64); - // Reinsert the file handle - this.machine.file_handler.handles.insert(fd, handle); - this.consume_result(bytes) - }, - ) + this.remove_handle_and(fd, |mut handle, this| { + // Don't use `?` to avoid returning before reinserting the handle + let bytes = this + .memory_mut() + .get_mut(buf.alloc_id).and_then(|alloc| + alloc.get_bytes_mut(tcx, buf, Size::from_bytes(count)) + .map(|buffer| handle.file.read(buffer).map(|bytes| bytes as i64)) + ); + // Reinsert the file handle + this.machine.file_handler.handles.insert(fd, handle); + this.consume_result(bytes?) + }) } fn write( @@ -191,13 +186,14 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let buf = this.force_ptr(this.read_scalar(buf_op)?.not_undef()?)?; let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; - // `to_vec` is needed to avoid borrowing issues when writing to the file. - let bytes = this.memory().get(buf.alloc_id)?.get_bytes(tcx, buf, Size::from_bytes(count))?.to_vec(); - this.remove_handle_and(fd, |mut handle, this| { - let bytes = handle.file.write(&bytes).map(|bytes| bytes as i64); + let bytes = this.memory().get(buf.alloc_id).and_then(|alloc| { + alloc + .get_bytes(tcx, buf, Size::from_bytes(count)) + .map(|bytes| handle.file.write(bytes).map(|bytes| bytes as i64)) + }); this.machine.file_handler.handles.insert(fd, handle); - this.consume_result(bytes) + this.consume_result(bytes?) }) } @@ -251,7 +247,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx /// /// This function uses `T: From` instead of `i32` directly because some IO related /// functions return different integer types (like `read`, that returns an `i64`) - fn consume_result>(&mut self, result: std::io::Result) -> InterpResult<'tcx, T> { + fn consume_result>( + &mut self, + result: std::io::Result, + ) -> InterpResult<'tcx, T> { match result { Ok(ok) => Ok(ok), Err(e) => { From f5022b19d38777c72a308028663c7b6994d04648 Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Wed, 2 Oct 2019 08:43:23 -0500 Subject: [PATCH 4/5] Fix dangling pointer bug for zero-sized reads --- src/shims/io.rs | 22 ++++++++++++++-------- tests/run-pass/file_read.rs | 20 ++++++++++++-------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/src/shims/io.rs b/src/shims/io.rs index 49ad06e0df..9d3e3d2616 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -150,21 +150,27 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let tcx = &{ this.tcx.tcx }; let fd = this.read_scalar(fd_op)?.to_i32()?; - let buf = this.force_ptr(this.read_scalar(buf_op)?.not_undef()?)?; + let buf_scalar = this.read_scalar(buf_op)?.not_undef()?; let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; // Remove the file handle to avoid borrowing issues this.remove_handle_and(fd, |mut handle, this| { // Don't use `?` to avoid returning before reinserting the handle - let bytes = this - .memory_mut() - .get_mut(buf.alloc_id).and_then(|alloc| - alloc.get_bytes_mut(tcx, buf, Size::from_bytes(count)) - .map(|buffer| handle.file.read(buffer).map(|bytes| bytes as i64)) - ); + let bytes = + if count == 0 { + Ok(handle.file.read(&mut [])) + } else { + this.force_ptr(buf_scalar).and_then(|buf| this + .memory_mut() + .get_mut(buf.alloc_id).and_then(|alloc| + alloc.get_bytes_mut(tcx, buf, Size::from_bytes(count)) + .map(|buffer| handle.file.read(buffer)) + )) + + }; // Reinsert the file handle this.machine.file_handler.handles.insert(fd, handle); - this.consume_result(bytes?) + this.consume_result(bytes?.map(|bytes| bytes as i64)) }) } diff --git a/tests/run-pass/file_read.rs b/tests/run-pass/file_read.rs index b0f2618920..a17f948980 100644 --- a/tests/run-pass/file_read.rs +++ b/tests/run-pass/file_read.rs @@ -6,14 +6,18 @@ use std::io::{ Read, Write }; fn main() { // FIXME: remove the file and delete it when `rm` is implemented. - + let path = "./tests/hello.txt"; + let bytes = b"Hello, World!\n"; // Test creating, writing and closing a file (closing is tested when `file` is dropped). - let mut file = File::create("./tests/hello.txt").unwrap(); - file.write(b"Hello, World!\n").unwrap(); - + let mut file = File::create(path).unwrap(); + file.write(bytes).unwrap(); // Test opening, reading and closing a file. - let mut file = File::open("./tests/hello.txt").unwrap(); - let mut contents = String::new(); - file.read_to_string(&mut contents).unwrap(); - assert_eq!("Hello, World!\n", contents); + let mut file = File::open(path).unwrap(); + let mut contents = Vec::new(); + // Reading 0 bytes should not fill `contents`. + file.read(&mut contents).unwrap(); + assert!(contents.is_empty()); + // Reading until EOF should get the whole text. + file.read_to_end(&mut contents).unwrap(); + assert_eq!(bytes, contents.as_slice()); } From 6c36a8c949a3e506b48481d746df85e6596d097d Mon Sep 17 00:00:00 2001 From: Christian Poveda Date: Wed, 2 Oct 2019 08:50:32 -0500 Subject: [PATCH 5/5] Return earlier when reading/writing 0 bytes --- src/shims/io.rs | 30 ++++++++++++++++-------------- tests/run-pass/file_read.rs | 10 ++++++---- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/shims/io.rs b/src/shims/io.rs index 9d3e3d2616..74b8dde5c7 100644 --- a/src/shims/io.rs +++ b/src/shims/io.rs @@ -149,25 +149,23 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let tcx = &{ this.tcx.tcx }; + let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; + // Reading zero bytes should not change `buf` + if count == 0 { + return Ok(0); + } let fd = this.read_scalar(fd_op)?.to_i32()?; let buf_scalar = this.read_scalar(buf_op)?.not_undef()?; - let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; // Remove the file handle to avoid borrowing issues this.remove_handle_and(fd, |mut handle, this| { // Don't use `?` to avoid returning before reinserting the handle - let bytes = - if count == 0 { - Ok(handle.file.read(&mut [])) - } else { - this.force_ptr(buf_scalar).and_then(|buf| this - .memory_mut() - .get_mut(buf.alloc_id).and_then(|alloc| - alloc.get_bytes_mut(tcx, buf, Size::from_bytes(count)) - .map(|buffer| handle.file.read(buffer)) - )) - - }; + let bytes = this.force_ptr(buf_scalar).and_then(|buf| { + this.memory_mut() + .get_mut(buf.alloc_id)? + .get_bytes_mut(tcx, buf, Size::from_bytes(count)) + .map(|buffer| handle.file.read(buffer)) + }); // Reinsert the file handle this.machine.file_handler.handles.insert(fd, handle); this.consume_result(bytes?.map(|bytes| bytes as i64)) @@ -188,9 +186,13 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let tcx = &{ this.tcx.tcx }; + let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; + // Writing zero bytes should not change `buf` + if count == 0 { + return Ok(0); + } let fd = this.read_scalar(fd_op)?.to_i32()?; let buf = this.force_ptr(this.read_scalar(buf_op)?.not_undef()?)?; - let count = this.read_scalar(count_op)?.to_usize(&*this.tcx)?; this.remove_handle_and(fd, |mut handle, this| { let bytes = this.memory().get(buf.alloc_id).and_then(|alloc| { diff --git a/tests/run-pass/file_read.rs b/tests/run-pass/file_read.rs index a17f948980..666abd65c1 100644 --- a/tests/run-pass/file_read.rs +++ b/tests/run-pass/file_read.rs @@ -2,7 +2,7 @@ // compile-flags: -Zmiri-disable-isolation use std::fs::File; -use std::io::{ Read, Write }; +use std::io::{Read, Write}; fn main() { // FIXME: remove the file and delete it when `rm` is implemented. @@ -10,13 +10,15 @@ fn main() { let bytes = b"Hello, World!\n"; // Test creating, writing and closing a file (closing is tested when `file` is dropped). let mut file = File::create(path).unwrap(); + // Writing 0 bytes should not change the file contents. + file.write(&mut []).unwrap(); + file.write(bytes).unwrap(); // Test opening, reading and closing a file. let mut file = File::open(path).unwrap(); let mut contents = Vec::new(); - // Reading 0 bytes should not fill `contents`. - file.read(&mut contents).unwrap(); - assert!(contents.is_empty()); + // Reading 0 bytes should not move the file pointer. + file.read(&mut []).unwrap(); // Reading until EOF should get the whole text. file.read_to_end(&mut contents).unwrap(); assert_eq!(bytes, contents.as_slice());