From 9c569189c8b8b5d635b6704c489a8410c81570a0 Mon Sep 17 00:00:00 2001 From: Paul Dicker Date: Fri, 15 Jan 2016 19:04:53 +0100 Subject: [PATCH] Addressed comments --- src/libstd/fs.rs | 41 ++++++++++++++---------- src/libstd/sys/unix/ext/fs.rs | 9 ++++-- src/libstd/sys/unix/fs.rs | 16 +++++----- src/libstd/sys/windows/ext/fs.rs | 53 ++++++++++++++++---------------- src/libstd/sys/windows/fs.rs | 16 +++++----- 5 files changed, 75 insertions(+), 60 deletions(-) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index fcaa2fede2e..40c08b17680 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -414,8 +414,8 @@ impl OpenOptions { /// This option, when true, will indicate that the file should be /// `write`-able if opened. /// - /// If a file already exist, the contents of that file get overwritten, but it is - /// not truncated. + /// If a file already exist, any write calls on the file will overwrite its + /// contents, without truncating it. /// /// # Examples /// @@ -436,19 +436,20 @@ impl OpenOptions { /// Note that setting `.write(true).append(true)` has the same effect as /// setting only `.append(true)`. /// - /// For most filesystems the operating system guarantees all writes are atomic: - /// no writes get mangled because another process writes at the same time. + /// For most filesystems the operating system guarantees all writes are + /// atomic: no writes get mangled because another process writes at the same + /// time. /// - /// One maybe obvious note when using append-mode: make sure that all data that - /// belongs together, is written the the file in one operation. This can be done - /// by concatenating strings before passing them to `write()`, or using a buffered - /// writer (with a more than adequately sized buffer) and calling `flush()` when the - /// message is complete. + /// One maybe obvious note when using append-mode: make sure that all data + /// that belongs together, is written the the file in one operation. This + /// can be done by concatenating strings before passing them to `write()`, + /// or using a buffered writer (with a more than adequately sized buffer) + /// and calling `flush()` when the message is complete. /// - /// If a file is opened with both read and append access, beware that after opening - /// and after every write the position for reading may be set at the end of the file. - /// So before writing save the current position (using `seek(SeekFrom::Current(0))`, - /// and restore it before the next read. + /// If a file is opened with both read and append access, beware that after + /// opening and after every write the position for reading may be set at the + /// end of the file. So before writing save the current position (using + /// `seek(SeekFrom::Current(0))`, and restore it before the next read. /// /// # Examples /// @@ -507,7 +508,12 @@ impl OpenOptions { /// No file is allowed to exist at the target location, also no (dangling) /// symlink. /// - /// if `.create_new(true)` is set, `.create()` and `.truncate()` are ignored. + /// This option is usefull because it as atomic. Otherwise between checking + /// whether a file exists and creating a new one, the file may have been + /// created by another process (a TOCTOU race condition / attack). + /// + /// If `.create_new(true)` is set, `.create()` and `.truncate()` are + /// ignored. /// /// The file must be opened with write or append access in order to create /// a new file. @@ -518,7 +524,9 @@ impl OpenOptions { /// #![feature(expand_open_options)] /// use std::fs::OpenOptions; /// - /// let file = OpenOptions::new().write(true).create_new(true).open("foo.txt"); + /// let file = OpenOptions::new().write(true) + /// .create_new(true) + /// .open("foo.txt"); /// ``` #[unstable(feature = "expand_open_options", reason = "recently added", @@ -534,7 +542,8 @@ impl OpenOptions { /// This function will return an error under a number of different /// circumstances, to include but not limited to: /// - /// * Opening a file that does not exist without setting `create` or `create_new`. + /// * Opening a file that does not exist without setting `create` or + /// `create_new`. /// * Attempting to open a file with access that the user lacks /// permissions for /// * Filesystem-level errors (full disk, etc) diff --git a/src/libstd/sys/unix/ext/fs.rs b/src/libstd/sys/unix/ext/fs.rs index 33f29d44f87..46416753c02 100644 --- a/src/libstd/sys/unix/ext/fs.rs +++ b/src/libstd/sys/unix/ext/fs.rs @@ -107,10 +107,11 @@ pub trait OpenOptionsExt { /// Pass custom flags to the `flags` agument of `open`. /// - /// The bits that define the access mode are masked out with `O_ACCMODE`, to ensure - /// they do not interfere with the access mode set by Rusts options. + /// The bits that define the access mode are masked out with `O_ACCMODE`, to + /// ensure they do not interfere with the access mode set by Rusts options. /// /// Custom flags can only set flags, not remove flags set by Rusts options. + /// This options overwrites any previously set custom flags. /// /// # Examples /// @@ -121,7 +122,9 @@ pub trait OpenOptionsExt { /// /// let mut options = OpenOptions::new(); /// options.write(true); - /// if cfg!(unix) { options.custom_flags(libc::O_NOFOLLOW); } + /// if cfg!(unix) { + /// options.custom_flags(libc::O_NOFOLLOW); + /// } /// let file = options.open("foo.txt"); /// ``` #[unstable(feature = "expand_open_options", diff --git a/src/libstd/sys/unix/fs.rs b/src/libstd/sys/unix/fs.rs index 31ea6b7dd2e..1e5dc972bc5 100644 --- a/src/libstd/sys/unix/fs.rs +++ b/src/libstd/sys/unix/fs.rs @@ -275,13 +275,15 @@ impl OpenOptions { fn get_creation_mode(&self) -> io::Result { match (self.write, self.append) { - (true, false) => {} - (false, false) => if self.truncate || self.create || self.create_new { - return Err(Error::from_raw_os_error(libc::EINVAL)); - }, - (_, true) => if self.truncate && !self.create_new { - return Err(Error::from_raw_os_error(libc::EINVAL)); - }, + (true, false) => {} + (false, false) => + if self.truncate || self.create || self.create_new { + return Err(Error::from_raw_os_error(libc::EINVAL)); + }, + (_, true) => + if self.truncate && !self.create_new { + return Err(Error::from_raw_os_error(libc::EINVAL)); + }, } Ok(match (self.create, self.truncate, self.create_new) { diff --git a/src/libstd/sys/windows/ext/fs.rs b/src/libstd/sys/windows/ext/fs.rs index 4bb67b0fad9..d060c902fba 100644 --- a/src/libstd/sys/windows/ext/fs.rs +++ b/src/libstd/sys/windows/ext/fs.rs @@ -27,9 +27,9 @@ pub trait OpenOptionsExt { /// with the specified value. /// /// This will override the `read`, `write`, and `append` flags on the - /// `OpenOptions` structure. This method provides fine-grained control - /// over the permissions to read, write and append data, attributes - /// (like hidden and system) and extended attributes. + /// `OpenOptions` structure. This method provides fine-grained control over + /// the permissions to read, write and append data, attributes (like hidden + /// and system) and extended attributes. /// /// # Examples /// @@ -38,8 +38,8 @@ pub trait OpenOptionsExt { /// use std::fs::OpenOptions; /// use std::os::windows::fs::OpenOptionsExt; /// - /// // Open without read and write permission, for example if you only need to call `stat()` - /// // on the file + /// // Open without read and write permission, for example if you only need + /// // to call `stat()` on the file /// let file = OpenOptions::new().access_mode(0).open("foo.txt"); /// ``` fn access_mode(&mut self, access: u32) -> &mut Self; @@ -47,9 +47,10 @@ pub trait OpenOptionsExt { /// Overrides the `dwShareMode` argument to the call to `CreateFile` with /// the specified value. /// - /// By default `share_mode` is set to `FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE`. - /// Specifying less permissions denies others to read from, write to and/or - /// delete the file while it is open. + /// By default `share_mode` is set to + /// `FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE`. Specifying + /// less permissions denies others to read from, write to and/or delete the + /// file while it is open. /// /// # Examples /// @@ -58,17 +59,20 @@ pub trait OpenOptionsExt { /// use std::fs::OpenOptions; /// use std::os::windows::fs::OpenOptionsExt; /// + /// // Do not allow others to read or modify this file while we have it open + /// // for writing /// let file = OpenOptions::new().write(true) - /// .share_mode(0) // Do not allow others to read or modify + /// .share_mode(0) /// .open("foo.txt"); /// ``` fn share_mode(&mut self, val: u32) -> &mut Self; - /// Sets extra flags for the `dwFileFlags` argument to the call to `CreateFile2` - /// (or combines it with `attributes` and `security_qos_flags` to set the - /// `dwFlagsAndAttributes` for `CreateFile`). + /// Sets extra flags for the `dwFileFlags` argument to the call to + /// `CreateFile2` (or combines it with `attributes` and `security_qos_flags` + /// to set the `dwFlagsAndAttributes` for `CreateFile`). /// /// Custom flags can only set flags, not remove flags set by Rusts options. + /// This options overwrites any previously set custom flags. /// /// # Examples /// @@ -79,7 +83,9 @@ pub trait OpenOptionsExt { /// /// let mut options = OpenOptions::new(); /// options.create(true).write(true); - /// if cfg!(windows) { options.custom_flags(winapi::FILE_FLAG_DELETE_ON_CLOSE); } + /// if cfg!(windows) { + /// options.custom_flags(winapi::FILE_FLAG_DELETE_ON_CLOSE); + /// } /// let file = options.open("foo.txt"); /// ``` #[unstable(feature = "expand_open_options", @@ -89,15 +95,16 @@ pub trait OpenOptionsExt { /// Sets the `dwFileAttributes` argument to the call to `CreateFile2` to /// the specified value (or combines it with `custom_flags` and - /// `security_qos_flags` to set the `dwFlagsAndAttributes` for `CreateFile`). + /// `security_qos_flags` to set the `dwFlagsAndAttributes` for + /// `CreateFile`). /// - /// If a _new_ file is created because it does not yet exist and `.create(true)` or - /// `.create_new(true)` are specified, the new file is given the attributes declared - /// with `.attributes()`. + /// If a _new_ file is created because it does not yet exist and + ///`.create(true)` or `.create_new(true)` are specified, the new file is + /// given the attributes declared with `.attributes()`. /// /// If an _existing_ file is opened with `.create(true).truncate(true)`, its - /// existing attributes are preserved and combined with the ones declared with - /// `.attributes()`. + /// existing attributes are preserved and combined with the ones declared + /// with `.attributes()`. /// /// In all other cases the attributes get ignored. /// @@ -119,10 +126,6 @@ pub trait OpenOptionsExt { /// the specified value (or combines it with `custom_flags` and `attributes` /// to set the `dwFlagsAndAttributes` for `CreateFile`). fn security_qos_flags(&mut self, flags: u32) -> &mut OpenOptions; - - /// Sets the `lpSecurityAttributes` argument to the call to `CreateFile` to - /// the specified value. - fn security_attributes(&mut self, attrs: sys::c::LPSECURITY_ATTRIBUTES) -> &mut OpenOptions; } #[unstable(feature = "open_options_ext", @@ -148,10 +151,6 @@ impl OpenOptionsExt for OpenOptions { fn security_qos_flags(&mut self, flags: u32) -> &mut OpenOptions { self.as_inner_mut().security_qos_flags(flags); self } - - fn security_attributes(&mut self, attrs: sys::c::LPSECURITY_ATTRIBUTES) -> &mut OpenOptions { - self.as_inner_mut().security_attributes(attrs); self - } } /// Extension methods for `fs::Metadata` to access the raw fields contained diff --git a/src/libstd/sys/windows/fs.rs b/src/libstd/sys/windows/fs.rs index b3b80be208b..df0a7bda18d 100644 --- a/src/libstd/sys/windows/fs.rs +++ b/src/libstd/sys/windows/fs.rs @@ -209,13 +209,15 @@ impl OpenOptions { const ERROR_INVALID_PARAMETER: i32 = 87; match (self.write, self.append) { - (true, false) => {} - (false, false) => if self.truncate || self.create || self.create_new { - return Err(Error::from_raw_os_error(ERROR_INVALID_PARAMETER)); - }, - (_, true) => if self.truncate && !self.create_new { - return Err(Error::from_raw_os_error(ERROR_INVALID_PARAMETER)); - }, + (true, false) => {} + (false, false) => + if self.truncate || self.create || self.create_new { + return Err(Error::from_raw_os_error(ERROR_INVALID_PARAMETER)); + }, + (_, true) => + if self.truncate && !self.create_new { + return Err(Error::from_raw_os_error(ERROR_INVALID_PARAMETER)); + }, } Ok(match (self.create, self.truncate, self.create_new) {