From cdf636120e55da29d5b17e48c22bde1fd39cb015 Mon Sep 17 00:00:00 2001 From: Paul Dicker <pitdicker@gmail.com> Date: Sat, 4 Jul 2015 18:12:26 +0200 Subject: [PATCH 1/8] Fix different behaviour of OpenOptions between Windows en Linux case #1: A file is opened as read-only and with `.truncate(true)`. On Windows this fails with `Error { repr: Os { code: 87, message: "The parameter is incorrect.\r\n" } }`. On Unix the behaviour is undefined. Solution: also fail on Linux with `Error { repr: Os { code: 22, message: "Invalid argument" } }` This is a breaking change, but only for code that relies on the current undefined behaviour. case #2: A file is opened with `.append(true)`, but without `.write(true)`. On Windows it is possible to append to the file, on linux it fails. Solution: let `.append(true)` imply `.write(true)` on linux. case #3: A file is opened without access options. On Windows it fails at the first read or write. On Linux this falls back to read-only mode. Solution: I prefer to also fail on Linux, with "Invalid argument". But this is a breaking change... It is also possible to add the same fallback on Windows. --- src/libstd/sys/unix/fs.rs | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/libstd/sys/unix/fs.rs b/src/libstd/sys/unix/fs.rs index 128284834ab01..cc94fdbdc26a8 100644 --- a/src/libstd/sys/unix/fs.rs +++ b/src/libstd/sys/unix/fs.rs @@ -251,6 +251,24 @@ impl OpenOptions { self.flags &= !bit; } } + + fn get_flags(&self) -> io::Result<c_int> { + let append = (self.flags & libc::O_APPEND) == libc::O_APPEND; + let truncate = (self.flags & libc::O_TRUNC) == libc::O_TRUNC; + + if truncate && !(self.write || append) { + // The result of using O_TRUNC with O_RDONLY is undefined. + // Not allowed, for consistency with Windows. + Err(Error::from_raw_os_error(libc::EINVAL)) + } else { + match (self.read, self.write | append) { + (true, true) => Ok(libc::O_RDWR | self.flags), + (false, true) => Ok(libc::O_WRONLY | self.flags), + (true, false) => Ok(libc::O_RDONLY | self.flags), + (false, false) => Err(Error::from_raw_os_error(libc::EINVAL)), + } + } + } } impl File { @@ -260,12 +278,7 @@ impl File { } pub fn open_c(path: &CStr, opts: &OpenOptions) -> io::Result<File> { - let flags = opts.flags | match (opts.read, opts.write) { - (true, true) => libc::O_RDWR, - (false, true) => libc::O_WRONLY, - (true, false) | - (false, false) => libc::O_RDONLY, - }; + let flags = try!(opts.get_flags()); let fd = try!(cvt_r(|| unsafe { libc::open(path.as_ptr(), flags, opts.mode) })); From b8c1737ef885e29139c0291f70a0fc0fa51b69af Mon Sep 17 00:00:00 2001 From: Paul Dicker <pitdicker@gmail.com> Date: Sat, 4 Jul 2015 18:12:54 +0200 Subject: [PATCH 2/8] Remove non-working work-around --- src/libstd/sys/windows/fs.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/libstd/sys/windows/fs.rs b/src/libstd/sys/windows/fs.rs index 36fabe72aa0c1..ef2e2d01c3d4d 100644 --- a/src/libstd/sys/windows/fs.rs +++ b/src/libstd/sys/windows/fs.rs @@ -201,13 +201,7 @@ impl OpenOptions { (true, true) => libc::CREATE_ALWAYS, (true, false) => libc::OPEN_ALWAYS, (false, false) => libc::OPEN_EXISTING, - (false, true) => { - if self.write && !self.append { - libc::CREATE_ALWAYS - } else { - libc::TRUNCATE_EXISTING - } - } + (false, true) => libc::TRUNCATE_EXISTING, } }) } From da203803f951ba94cb21a28670794d8b0fd83ac9 Mon Sep 17 00:00:00 2001 From: Paul Dicker <pitdicker@gmail.com> Date: Sat, 4 Jul 2015 18:15:16 +0200 Subject: [PATCH 3/8] Make append have the same flags as write on Windows --- src/libstd/sys/windows/fs.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/libstd/sys/windows/fs.rs b/src/libstd/sys/windows/fs.rs index ef2e2d01c3d4d..a4466da429575 100644 --- a/src/libstd/sys/windows/fs.rs +++ b/src/libstd/sys/windows/fs.rs @@ -179,8 +179,9 @@ impl OpenOptions { let mut base = if self.read {libc::FILE_GENERIC_READ} else {0} | if self.write {libc::FILE_GENERIC_WRITE} else {0}; if self.append { + /* append has the same bits set as write, but without FILE_WRITE_DATA */ + base |= libc::FILE_GENERIC_WRITE; base &= !libc::FILE_WRITE_DATA; - base |= libc::FILE_APPEND_DATA; } base }) From e3751977151efdf4fccbb0ae90a41f19317a44d2 Mon Sep 17 00:00:00 2001 From: Paul Dicker <pitdicker@gmail.com> Date: Sat, 4 Jul 2015 18:54:09 +0200 Subject: [PATCH 4/8] Clarify documentation and examples --- src/libstd/fs.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index c2d3d2fb0c848..5d08d96576246 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -359,7 +359,7 @@ impl<'a> Seek for &'a File { } impl OpenOptions { - /// Creates a blank net set of options ready for configuration. + /// Creates a blank new set of options ready for configuration. /// /// All options are initially set to `false`. /// @@ -368,7 +368,8 @@ impl OpenOptions { /// ```no_run /// use std::fs::OpenOptions; /// - /// let file = OpenOptions::new().open("foo.txt"); + /// let mut options = OpenOptions::new(); + /// let file = options.read(true).open("foo.txt"); /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn new() -> OpenOptions { @@ -419,7 +420,7 @@ impl OpenOptions { /// ```no_run /// use std::fs::OpenOptions; /// - /// let file = OpenOptions::new().write(true).append(true).open("foo.txt"); + /// let file = OpenOptions::new().append(true).open("foo.txt"); /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn append(&mut self, append: bool) -> &mut OpenOptions { @@ -431,6 +432,8 @@ impl OpenOptions { /// If a file is successfully opened with this option set it will truncate /// the file to 0 length if it already exists. /// + /// The file must be opened with write access for truncate to work. + /// /// # Examples /// /// ```no_run @@ -453,7 +456,7 @@ impl OpenOptions { /// ```no_run /// use std::fs::OpenOptions; /// - /// let file = OpenOptions::new().create(true).open("foo.txt"); + /// let file = OpenOptions::new().write(true).create(true).open("foo.txt"); /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn create(&mut self, create: bool) -> &mut OpenOptions { From cae20d039c6e4c53f7ded0981f6321a548ab5ca1 Mon Sep 17 00:00:00 2001 From: Paul Dicker <pitdicker@gmail.com> Date: Sun, 12 Jul 2015 13:53:28 +0200 Subject: [PATCH 5/8] Add extra tests --- src/libstd/fs.rs | 104 ++++++++++++++++++++++++++++---------- src/libstd/sys/unix/fs.rs | 2 +- 2 files changed, 78 insertions(+), 28 deletions(-) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index 5d08d96576246..007ae050c79a1 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -1963,61 +1963,111 @@ mod tests { let mut r = OO::new(); r.read(true); let mut w = OO::new(); w.write(true); - let mut rw = OO::new(); rw.write(true).read(true); + let mut rw = OO::new(); rw.read(true).write(true); + let mut a = OO::new(); a.append(true); + let mut ra = OO::new(); ra.read(true).append(true); + // Test opening a non-existing file match r.open(&tmpdir.join("a")) { Ok(..) => panic!(), Err(..) => {} } - // Perform each one twice to make sure that it succeeds the second time - // (where the file exists) - check!(c(&w).create(true).open(&tmpdir.join("b"))); + // Test various combinations of access modes with open options. + // Tested combinations: + // - .create(true).open() when file does not exist + // - .create(true).open() when file exists + // - .open() when file exists + // - .truncate.open() when file exists + check!(c(&r).create(true).open(&tmpdir.join("b"))); assert!(tmpdir.join("b").exists()); - check!(c(&w).create(true).open(&tmpdir.join("b"))); - check!(w.open(&tmpdir.join("b"))); + check!(c(&r).create(true).open(&tmpdir.join("b"))); + check!(r.open(&tmpdir.join("b"))); + error!(c(&r).truncate(true).open(&tmpdir.join("b")), invalid_options); - check!(c(&rw).create(true).open(&tmpdir.join("c"))); + check!(c(&w).create(true).open(&tmpdir.join("c"))); assert!(tmpdir.join("c").exists()); - check!(c(&rw).create(true).open(&tmpdir.join("c"))); - check!(rw.open(&tmpdir.join("c"))); + check!(c(&w).create(true).open(&tmpdir.join("c"))); + check!(w.open(&tmpdir.join("c"))); + check!(c(&w).truncate(true).open(&tmpdir.join("c"))); - check!(c(&w).append(true).create(true).open(&tmpdir.join("d"))); + check!(c(&rw).create(true).open(&tmpdir.join("d"))); assert!(tmpdir.join("d").exists()); - check!(c(&w).append(true).create(true).open(&tmpdir.join("d"))); - check!(c(&w).append(true).open(&tmpdir.join("d"))); + check!(c(&rw).create(true).open(&tmpdir.join("d"))); + check!(rw.open(&tmpdir.join("d"))); + check!(c(&rw).truncate(true).open(&tmpdir.join("d"))); - check!(c(&rw).append(true).create(true).open(&tmpdir.join("e"))); + check!(c(&a).create(true).open(&tmpdir.join("e"))); assert!(tmpdir.join("e").exists()); - check!(c(&rw).append(true).create(true).open(&tmpdir.join("e"))); - check!(c(&rw).append(true).open(&tmpdir.join("e"))); + check!(c(&a).create(true).open(&tmpdir.join("e"))); + check!(a.open(&tmpdir.join("e"))); + error!(c(&a).truncate(true).open(&tmpdir.join("e")), invalid_options); - check!(c(&w).truncate(true).create(true).open(&tmpdir.join("f"))); + check!(c(&ra).create(true).open(&tmpdir.join("f"))); assert!(tmpdir.join("f").exists()); - check!(c(&w).truncate(true).create(true).open(&tmpdir.join("f"))); - check!(c(&w).truncate(true).open(&tmpdir.join("f"))); - - check!(c(&rw).truncate(true).create(true).open(&tmpdir.join("g"))); + check!(c(&ra).create(true).open(&tmpdir.join("f"))); + check!(ra.open(&tmpdir.join("f"))); + error!(c(&ra).truncate(true).open(&tmpdir.join("f")), invalid_options); + + // Test combination of .create(true) and .truncate(true) + // Tested combinations: + // - .truncate.open() when file does not exist + // - .create(true).truncate.open() when file does not exist + // - .create(true).truncate.open() when file exists + assert!(c(&w).truncate(true).open(&tmpdir.join("g")).is_err()); + check!(c(&w).create(true).truncate(true).open(&tmpdir.join("g"))); assert!(tmpdir.join("g").exists()); - check!(c(&rw).truncate(true).create(true).open(&tmpdir.join("g"))); - check!(c(&rw).truncate(true).open(&tmpdir.join("g"))); + check!(c(&w).create(true).truncate(true).open(&tmpdir.join("g"))); - check!(check!(File::create(&tmpdir.join("h"))).write("foo".as_bytes())); + // Test open options have effect + // Test write works + check!(check!(File::create(&tmpdir.join("h"))).write("foobar".as_bytes())); + // Test write fails for read-only check!(r.open(&tmpdir.join("h"))); { let mut f = check!(r.open(&tmpdir.join("h"))); assert!(f.write("wut".as_bytes()).is_err()); } + // Test write overwrites + { + let mut f = check!(c(&w).open(&tmpdir.join("h"))); + check!(f.write("baz".as_bytes())); + } + { + let mut f = check!(c(&r).open(&tmpdir.join("h"))); + let mut b = vec![0; 6]; + check!(f.read(&mut b)); + assert_eq!(b, "bazbar".as_bytes()); + } + // Test truncate works + { + let mut f = check!(c(&w).truncate(true).open(&tmpdir.join("h"))); + check!(f.write("foo".as_bytes())); + } + assert_eq!(check!(fs::metadata(&tmpdir.join("h"))).len(), 3); + // Test append works assert_eq!(check!(fs::metadata(&tmpdir.join("h"))).len(), 3); { - let mut f = check!(c(&w).append(true).open(&tmpdir.join("h"))); + let mut f = check!(c(&a).open(&tmpdir.join("h"))); check!(f.write("bar".as_bytes())); } assert_eq!(check!(fs::metadata(&tmpdir.join("h"))).len(), 6); + // Test .append(true) equals .write(true).append(true) { - let mut f = check!(c(&w).truncate(true).open(&tmpdir.join("h"))); - check!(f.write("bar".as_bytes())); + let mut f = check!(c(&w).append(true).open(&tmpdir.join("h"))); + check!(f.write("baz".as_bytes())); + } + assert_eq!(check!(fs::metadata(&tmpdir.join("h"))).len(), 9); + + // Test opening a file without setting an access mode + let mut blank = OO::new(); + if cfg!(unix) { + error!(blank.create(true).open(&tmpdir.join("i")), invalid_options); + } + if cfg!(windows) { + let mut f = check!(blank.create(true).open(&tmpdir.join("i"))); + let mut b = vec![0; 6]; + assert!(f.read(&mut b).is_err()); } - assert_eq!(check!(fs::metadata(&tmpdir.join("h"))).len(), 3); } #[test] diff --git a/src/libstd/sys/unix/fs.rs b/src/libstd/sys/unix/fs.rs index cc94fdbdc26a8..6b020b398c785 100644 --- a/src/libstd/sys/unix/fs.rs +++ b/src/libstd/sys/unix/fs.rs @@ -256,7 +256,7 @@ impl OpenOptions { let append = (self.flags & libc::O_APPEND) == libc::O_APPEND; let truncate = (self.flags & libc::O_TRUNC) == libc::O_TRUNC; - if truncate && !(self.write || append) { + if truncate && (!self.write || append) { // The result of using O_TRUNC with O_RDONLY is undefined. // Not allowed, for consistency with Windows. Err(Error::from_raw_os_error(libc::EINVAL)) From f7bf0798aeb08a31aef47bfdcd7e9c67884950de Mon Sep 17 00:00:00 2001 From: Paul Dicker <pitdicker@gmail.com> Date: Sun, 12 Jul 2015 14:29:45 +0200 Subject: [PATCH 6/8] Some extra documentation --- src/libstd/fs.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libstd/fs.rs b/src/libstd/fs.rs index 007ae050c79a1..edb6bee4cf1c6 100644 --- a/src/libstd/fs.rs +++ b/src/libstd/fs.rs @@ -414,6 +414,8 @@ impl OpenOptions { /// /// This option, when true, means that writes will append to a file instead /// of overwriting previous contents. + /// Note that setting `.write(true).append(true)` has the same effect as + /// setting only `.append(true)`. /// /// # Examples /// @@ -451,6 +453,9 @@ impl OpenOptions { /// This option indicates whether a new file will be created if the file /// does not yet already exist. /// + /// The access mode (e.g. read, write and/or append) does not influence whether + /// a new file is created, or with which permissions it is created. + /// /// # Examples /// /// ```no_run From ecbbe3a82708f056723784c0de9c22cb7ef3d550 Mon Sep 17 00:00:00 2001 From: Paul Dicker <pitdicker@gmail.com> Date: Thu, 6 Aug 2015 08:48:18 +0200 Subject: [PATCH 7/8] WIP --- src/libstd/sys/unix/fs.rs | 51 +++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 15 deletions(-) diff --git a/src/libstd/sys/unix/fs.rs b/src/libstd/sys/unix/fs.rs index 6b020b398c785..db1b1e21034c0 100644 --- a/src/libstd/sys/unix/fs.rs +++ b/src/libstd/sys/unix/fs.rs @@ -213,9 +213,13 @@ impl DirEntry { impl OpenOptions { pub fn new() -> OpenOptions { OpenOptions { - flags: 0, + flags: libc::O_CLOEXEC, read: false, write: false, + append: false, + truncate: false, + create: false, + create_new: false, mode: 0o666, } } @@ -229,15 +233,19 @@ impl OpenOptions { } pub fn append(&mut self, append: bool) { - self.flag(libc::O_APPEND, append); + self.append = append; } pub fn truncate(&mut self, truncate: bool) { - self.flag(libc::O_TRUNC, truncate); + self.truncate = truncate; } pub fn create(&mut self, create: bool) { - self.flag(libc::O_CREAT, create); + self.create = create; + } + + pub fn create_new(&mut self, create_new: bool) { + self.create_new = create_new; } pub fn mode(&mut self, mode: raw::mode_t) { @@ -253,21 +261,34 @@ impl OpenOptions { } fn get_flags(&self) -> io::Result<c_int> { - let append = (self.flags & libc::O_APPEND) == libc::O_APPEND; - let truncate = (self.flags & libc::O_TRUNC) == libc::O_TRUNC; + flags = self.flags; + + // Access mode + match (self.read, self.write | self.append) { + (true, true) => flags |= libc::O_RDWR, + (false, true) => flags |= libc::O_WRONLY, + (true, false) => flags |= libc::O_RDONLY, + (false, false) => return Err(Error::from_raw_os_error(libc::EINVAL)), + } + if self.append {flags |= libc::O_APPEND}; - if truncate && (!self.write || append) { - // The result of using O_TRUNC with O_RDONLY is undefined. - // Not allowed, for consistency with Windows. - Err(Error::from_raw_os_error(libc::EINVAL)) + // Flags + if self.create_new { + flags |= libc::O_CREAT | libc::O_EXCL; } else { - match (self.read, self.write | append) { - (true, true) => Ok(libc::O_RDWR | self.flags), - (false, true) => Ok(libc::O_WRONLY | self.flags), - (true, false) => Ok(libc::O_RDONLY | self.flags), - (false, false) => Err(Error::from_raw_os_error(libc::EINVAL)), + if self.truncate { + if !self.write || self.append { + // The result of using O_TRUNC with O_RDONLY or O_APPEND is undefined. + // Not allowed, for consistency with Windows. + return Err(Error::from_raw_os_error(libc::EINVAL)) + } else { + flags |= libc::O_TRUNC; + } } + if self.create {flags |= libc::O_CREAT}; } + + Ok(flags) } } From 0f5f111e572f63b58509196d0c21db966117dbe7 Mon Sep 17 00:00:00 2001 From: Paul Dicker <pitdicker@gmail.com> Date: Fri, 14 Aug 2015 17:38:40 +0200 Subject: [PATCH 8/8] Set generic access flags to unbreak truncate --- src/libstd/sys/windows/fs.rs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/libstd/sys/windows/fs.rs b/src/libstd/sys/windows/fs.rs index a4466da429575..9b5c866bab3a6 100644 --- a/src/libstd/sys/windows/fs.rs +++ b/src/libstd/sys/windows/fs.rs @@ -176,13 +176,14 @@ impl OpenOptions { fn get_desired_access(&self) -> libc::DWORD { self.desired_access.unwrap_or({ - let mut base = if self.read {libc::FILE_GENERIC_READ} else {0} | - if self.write {libc::FILE_GENERIC_WRITE} else {0}; - if self.append { - /* append has the same bits set as write, but without FILE_WRITE_DATA */ - base |= libc::FILE_GENERIC_WRITE; - base &= !libc::FILE_WRITE_DATA; - } + let base = if self.read {libc::GENERIC_READ} else {0} | + if self.append { + /* append has the same bits set as those that are implied + for write, but without FILE_WRITE_DATA */ + libc::FILE_GENERIC_WRITE & !libc::FILE_WRITE_DATA + } else if self.write { + libc::GENERIC_WRITE + } else {0}; base }) }