Skip to content

Fix install: invalid link at destination#5851

Merged
sylvestre merged 1 commit intouutils:mainfrom
cre4ture:fix/install_invalid_link_at_destination
Mar 12, 2024
Merged

Fix install: invalid link at destination#5851
sylvestre merged 1 commit intouutils:mainfrom
cre4ture:fix/install_invalid_link_at_destination

Conversation

@cre4ture
Copy link
Copy Markdown
Contributor

@cre4ture cre4ture commented Jan 17, 2024 •
edited
Loading

addresses issue #3565

Problem is that fs::copy fails when at the destination an invalid symbolic link exists.
I guess it tries to write to the file which the existing destination link points to.
Not sure if this should be even reported as an issue to rustlib itself.

The workaround I implemented is to check if the existing file is any link and remove it.
I didn't implement to check for invalid links. So all existing links will be removed before fs::copy is called.
EDIT: We simplified it by just removing all existings target files before copy.

I'm no install usecase expert, but I hope that there is no usecase where the existing link at destination needs to be used to redirect the writing of the data.

Tests are extended accordingly.

EDIT Addition: As I have now a FreeBSD VM running, I was able to make some of the disabled tests running on FreeBSD.
Issue was mainly that we need a specific binary for stripping symbols for FreeBSD and that the objdump executable doesn't exists, but rather llvm-objdump.

@cre4ture cre4ture force-pushed the fix/install_invalid_link_at_destination branch from a9b9a50 to 5d5b430 Compare January 17, 2024 15:03
@cre4ture cre4ture mentioned this pull request Jan 17, 2024
Closed
@cakebaker cakebaker linked an issue Jan 17, 2024 that may be closed by this pull request
install: arch - yay build may failed due to not finding or permissions of files in cache #3565
Closed
@cre4ture cre4ture changed the title Fix/install invalid link at destination Fix install: invalid link at destination Jan 17, 2024
@cre4ture cre4ture marked this pull request as draft January 17, 2024 15:31
@cre4ture cre4ture force-pushed the fix/install_invalid_link_at_destination branch from b5f9578 to de8951b Compare January 17, 2024 15:40
@cre4ture cre4ture marked this pull request as ready for review January 17, 2024 15:42
@cre4ture cre4ture force-pushed the fix/install_invalid_link_at_destination branch 2 times, most recently from efc9e9a to b08bcca Compare January 17, 2024 15:46
@tertsdiepraam
Copy link
Copy Markdown
Collaborator

tertsdiepraam commented Jan 17, 2024

Nice find! I think the behaviour might be even more general. Looking at the output of strace, GNU always removes a file if it already exists, regardless of whether it's a symlink.

This is the GNU strace of me copying b to the folder to if to/b exists and is a regular file:

openat(AT_FDCWD, "to", O_RDONLY|O_PATH|O_DIRECTORY) = 3
newfstatat(AT_FDCWD, "b", {st_mode=S_IFREG|0644, st_size=0, ...}, 0) = 0
newfstatat(3, "b", {st_mode=S_IFREG|0755, st_size=0, ...}, AT_SYMLINK_NOFOLLOW) = 0
unlinkat(3, "b", 0)                     = 0
openat(AT_FDCWD, "b", O_RDONLY)         = 4
newfstatat(4, "", {st_mode=S_IFREG|0644, st_size=0, ...}, AT_EMPTY_PATH) = 0
openat(3, "b", O_WRONLY|O_CREAT|O_EXCL, 0600) = 5

@cre4ture
Copy link
Copy Markdown
Contributor Author

cre4ture commented Jan 17, 2024

The documentation of fs::copy states explicitly that any file at destination will be overwritten. So its not needed for us to delete the file explicitly before. But we could remove the check if the exisiting destination is a link. This would simplify the code a bit.
But the difference is quite small. Shall I do the adaption to also remove regular files?

@tertsdiepraam
Copy link
Copy Markdown
Collaborator

tertsdiepraam commented Jan 17, 2024

Yeah I think that makes sense! We could still include a little comment to explain why it's necessary.

@cre4ture
Copy link
Copy Markdown
Contributor Author

cre4ture commented Jan 18, 2024

done

@cakebaker cakebaker force-pushed the fix/install_invalid_link_at_destination branch from b29bda3 to b88dff6 Compare January 19, 2024 16:03
@cre4ture cre4ture force-pushed the fix/install_invalid_link_at_destination branch 3 times, most recently from ac88ea2 to 2559a51 Compare January 23, 2024 20:55
@cre4ture cre4ture force-pushed the fix/install_invalid_link_at_destination branch from 2559a51 to e113d77 Compare February 4, 2024 22:33
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 4, 2024

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

@cre4ture cre4ture force-pushed the fix/install_invalid_link_at_destination branch 2 times, most recently from 603e11e to 95728d1 Compare February 10, 2024 13:38
@cre4ture cre4ture force-pushed the fix/install_invalid_link_at_destination branch 3 times, most recently from 8bcb690 to 48b49a5 Compare February 25, 2024 21:09
@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 25, 2024

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

@cre4ture cre4ture force-pushed the fix/install_invalid_link_at_destination branch 2 times, most recently from ade9118 to ded50d4 Compare March 9, 2024 18:53
sylvestre
sylvestre reviewed Mar 10, 2024
View reviewed changes
src/uu/install/src/install.rs Outdated
Comment on lines +753 to +761
match fs::remove_file(to) {
Ok(()) => {}
Err(e) if e.kind() == std::io::ErrorKind::NotFound => { /* expected */ }
Err(e) => {
show_error!(
"Failed to remove existing file {}. Error: {:?}",
to.display(),
e
);
}
}
Copy link
Copy Markdown
Contributor

@sylvestre sylvestre Mar 10, 2024

Choose a reason for hiding this comment

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

can it be simplified this way ?

Suggested change
match fs::remove_file(to) {
Ok(()) => {}
Err(e) if e.kind() == std::io::ErrorKind::NotFound => { /* expected */ }
Err(e) => {
show_error!(
"Failed to remove existing file {}. Error: {:?}",
to.display(),
e
);
}
}
if let Err(e) = fs::remove_file(to) {
if e.kind() != std::io::ErrorKind::NotFound {
show_error!("Failed to remove existing file {}. Error: {:?}", to.display(), e);
}
}

Copy link
Copy Markdown
Contributor Author

@cre4ture cre4ture Mar 11, 2024

Choose a reason for hiding this comment

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

true. done. Thanks :-)

@cre4ture
Fix install: invalid link at destination
7cd754e 
also remove some FixMEs for FreeBsd
@cre4ture cre4ture force-pushed the fix/install_invalid_link_at_destination branch from ded50d4 to 7cd754e Compare March 11, 2024 22:31
@cre4ture cre4ture requested a review from sylvestre March 11, 2024 22:33
@sylvestre
Copy link
Copy Markdown
Contributor

sylvestre commented Mar 12, 2024

sweet, well done :)

@sylvestre sylvestre merged commit ffb7059 into uutils:main Mar 12, 2024
@chenrui333 chenrui333 mentioned this pull request Mar 24, 2024
Merged
@moonfruit moonfruit mentioned this pull request Mar 28, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sylvestre sylvestre Awaiting requested review from sylvestre

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

install: arch - yay build may failed due to not finding or permissions of files in cache

3 participants

@cre4ture @tertsdiepraam @sylvestre