install: create destination file with safer modes before copy #6972
Conversation
nerdroychan
force-pushed
the
install_copy_permission
branch
from
9d0a3cd to
c93c644
Compare
|
GNU testsuite comparison: |
src/uu/install/src/install.rs
Outdated
| return Err(InstallError::InstallFailed(from.to_path_buf(), to.to_path_buf(), e).into()); | ||
| } | ||
| let mut src = src.unwrap(); | ||
|
|
||
| // fs::copy fails if destination is a invalid symlink. |
There was a problem hiding this comment.
this is outdated as fs::copy itself is no longer used.
however, straced install from GNU coreutils and confirmed it also unlinks -- kind of surprised, i would expect creation of a temporary file and rename (space permitting). that is to say the comment can be removed altogether or replaced with a statement this lines up with gnu
There was a problem hiding this comment.
Updated the comments in the latest push.
src/uu/install/src/install.rs
Outdated
| } | ||
| }; | ||
| remove_destination(); |
There was a problem hiding this comment.
gnu install errors out on this. this code continues trying to create the file.
There was a problem hiding this comment.
This will raise an error in the latest commit.
| // use `std::io::copy` to avoid unsafe file modes | ||
| if let Err(err) = std::io::copy(&mut src, &mut dst) { | ||
| drop(src); | ||
| drop(dst); |
There was a problem hiding this comment.
are these drops necessary? unlink should proceed just fine at least on unix systems, no idea about windows
There was a problem hiding this comment.
These two early drops are for closing two file descriptors before unlink. Technically the unlink will succeed even when there are opened fds... WDYT?
nerdroychan
force-pushed
the
install_copy_permission
branch
from
c93c644 to
b3cc502
Compare
This is a followup PR to #6595.
Regarding the unconditional removal of the destination file problem, before this patch, the destination file is removed unconditionally when
copy_fileis called. Now a healthy check is added, which only removes the destination file if the source file exists and can be opened to read.Furthermore, this patch uses
std::io::copywith file descriptors to avoid the chmod 644 problem in the original patch. This is verified using strace.A new test case is added as well.
Ping @mjguzik.