Skip to content

Commit

Permalink
std: Make unlink() more consistent
Browse files Browse the repository at this point in the history
Currently when a read-only file has unlink() invoked on it on windows, the call
will fail. On unix, however, the call will succeed. In order to have a more
consistent behavior across platforms, this error is recognized on windows and
the file is changed to read-write before removal is attempted.
  • Loading branch information
alexcrichton committed Jul 14, 2014
1 parent 996263a commit fe67d26
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 6 deletions.
1 change: 1 addition & 0 deletions src/librustuv/lib.rs
Expand Up @@ -420,6 +420,7 @@ pub fn uv_error_to_io_error(uverr: UvError) -> IoError {
uvll::EADDRNOTAVAIL => libc::WSAEADDRNOTAVAIL,
uvll::ECANCELED => libc::ERROR_OPERATION_ABORTED,
uvll::EADDRINUSE => libc::WSAEADDRINUSE,
uvll::EPERM => libc::ERROR_ACCESS_DENIED,
err => {
uvdebug!("uverr.code {}", err as int);
// FIXME: Need to map remaining uv error types
Expand Down
4 changes: 3 additions & 1 deletion src/librustuv/uvll.rs
Expand Up @@ -39,7 +39,7 @@ use libc::uintptr_t;

pub use self::errors::{EACCES, ECONNREFUSED, ECONNRESET, EPIPE, ECONNABORTED,
ECANCELED, EBADF, ENOTCONN, ENOENT, EADDRNOTAVAIL,
EADDRINUSE};
EADDRINUSE, EPERM};

pub static OK: c_int = 0;
pub static EOF: c_int = -4095;
Expand All @@ -63,6 +63,7 @@ pub mod errors {
pub static EBADF: c_int = -4083;
pub static EADDRNOTAVAIL: c_int = -4090;
pub static EADDRINUSE: c_int = -4091;
pub static EPERM: c_int = -4048;
}
#[cfg(not(windows))]
pub mod errors {
Expand All @@ -80,6 +81,7 @@ pub mod errors {
pub static EBADF : c_int = -libc::EBADF;
pub static EADDRNOTAVAIL : c_int = -libc::EADDRNOTAVAIL;
pub static EADDRINUSE : c_int = -libc::EADDRINUSE;
pub static EPERM: c_int = -libc::EPERM;
}

pub static PROCESS_SETUID: c_int = 1 << 0;
Expand Down
48 changes: 43 additions & 5 deletions src/libstd/io/fs.rs
Expand Up @@ -275,11 +275,41 @@ impl File {
/// user lacks permissions to remove the file, or if some other filesystem-level
/// error occurs.
pub fn unlink(path: &Path) -> IoResult<()> {
let err = LocalIo::maybe_raise(|io| {
io.fs_unlink(&path.to_c_str())
}).map_err(IoError::from_rtio_error);
err.update_err("couldn't unlink path",
|e| format!("{}; path={}", e, path.display()))
return match do_unlink(path) {
Ok(()) => Ok(()),
Err(e) => {
// On unix, a readonly file can be successfully removed. On windows,
// however, it cannot. To keep the two platforms in line with
// respect to their behavior, catch this case on windows, attempt to
// change it to read-write, and then remove the file.
if cfg!(windows) && e.kind == io::PermissionDenied {
let stat = match stat(path) {
Ok(stat) => stat,
Err(..) => return Err(e),
};
if stat.perm.intersects(io::UserWrite) { return Err(e) }

match chmod(path, stat.perm | io::UserWrite) {
Ok(()) => do_unlink(path),
Err(..) => {
// Try to put it back as we found it
let _ = chmod(path, stat.perm);
Err(e)
}
}
} else {
Err(e)
}
}
};

fn do_unlink(path: &Path) -> IoResult<()> {
let err = LocalIo::maybe_raise(|io| {
io.fs_unlink(&path.to_c_str())
}).map_err(IoError::from_rtio_error);
err.update_err("couldn't unlink path",
|e| format!("{}; path={}", e, path.display()))
}
}

/// Given a path, query the file system to get information about a file,
Expand Down Expand Up @@ -1591,4 +1621,12 @@ mod test {
let actual = check!(File::open(&tmpdir.join("test")).read_to_end());
assert!(actual.as_slice() == bytes);
})

iotest!(fn unlink_readonly() {
let tmpdir = tmpdir();
let path = tmpdir.join("file");
check!(File::create(&path));
check!(chmod(&path, io::UserRead));
check!(unlink(&path));
})
}

9 comments on commit fe67d26

@bors
Copy link
Contributor

@bors bors commented on fe67d26 Jul 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saw approval from brson
at alexcrichton@fe67d26

@bors
Copy link
Contributor

@bors bors commented on fe67d26 Jul 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging alexcrichton/rust/windows-unlink = fe67d26 into auto

@bors
Copy link
Contributor

@bors bors commented on fe67d26 Jul 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alexcrichton/rust/windows-unlink = fe67d26 merged ok, testing candidate = 482f53a6

@bors
Copy link
Contributor

@bors bors commented on fe67d26 Jul 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saw approval from brson
at alexcrichton@fe67d26

@bors
Copy link
Contributor

@bors bors commented on fe67d26 Jul 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merging alexcrichton/rust/windows-unlink = fe67d26 into auto

@bors
Copy link
Contributor

@bors bors commented on fe67d26 Jul 14, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alexcrichton/rust/windows-unlink = fe67d26 merged ok, testing candidate = b733592

@bors
Copy link
Contributor

@bors bors commented on fe67d26 Jul 15, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fast-forwarding master to auto = b733592

Please sign in to comment.