Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Define fs::hard_link to not follow symlinks. #78026

Merged
merged 5 commits into from
Nov 9, 2020

Conversation

sunfishcode
Copy link
Member

@sunfishcode sunfishcode commented Oct 16, 2020

POSIX leaves it implementation-defined whether link follows symlinks.
In practice, for example, on Linux it does not and on FreeBSD it does.
So, switch to linkat, so that we can pick a behavior rather than
depending on OS defaults.

Pick the option to not follow symlinks. This is somewhat arbitrary, but
seems the less surprising choice because hard linking is a very
low-level feature which requires the source and destination to be on
the same mounted filesystem, and following a symbolic link could end
up in a different mounted filesystem.

POSIX leaves it implementation-defined whether `link` follows symlinks.
In practice, for example, on Linux it does not and on FreeBSD it does.
So, switch to `linkat`, so that we can pick a behavior rather than
depending on OS defaults.

Pick the option to not follow symlinks. This is somewhat arbitrary, but
seems the less surprising choice because hard linking is a very
low-level feature which requires the source and destination to be on
the same mounted filesystem, and following a symbolic link could end
up in a different mounted filesystem.
@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 16, 2020
@camelid camelid added A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 17, 2020
@bjorn3
Copy link
Member

bjorn3 commented Oct 18, 2020

What if an OS doesn't have support for this?

@sunfishcode
Copy link
Member Author

What if an OS doesn't have support for this?

Good question. linkat is in POSIX.1-2008 and pretty widely supported, but it appears at least VxWorks and Redox don't support it, so I've now added a patch special-casing those two platforms.

@steveklabnik
Copy link
Member

cc @rust-lang/libs

@dtolnay
Copy link
Member

dtolnay commented Oct 20, 2020

@rfcbot fcp merge
@rust-lang/libs:

POSIX leaves it implementation-defined whether link follows symlinks. In practice, for example, on Linux it does not and on FreeBSD it does. So, switch to linkat, so that we can pick a behavior rather than depending on OS defaults.

Pick the option to not follow symlinks. This is somewhat arbitrary, but seems the less surprising choice because hard linking is a very low-level feature which requires the source and destination to be on the same mounted filesystem, and following a symbolic link could end up in a different mounted filesystem.

@rfcbot
Copy link

rfcbot commented Oct 20, 2020

Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 20, 2020
@sfackler
Copy link
Member

I don't think @bjorn3's question was really answered. It seems like it happens to be the case that VxWorks and Redox's link implementations don't follow symlinks, but what happens if we want to support a platform that does not have linkat and has a link that does follow them?

@dtolnay
Copy link
Member

dtolnay commented Oct 20, 2020

Would we be comfortable with wording that indicates "symlink is not followed if such a thing is possible on the platform"? I think the concern motivating the PR was that even on platforms which support the behavior we prefer for fs::hard_link, you can't count on that to be the standard library's behavior.

This implies if VxWorks or Redox were to grow support for this operation later, we'd accept a change to the behavior to use it. In other words users would have the new guarantee from this PR on platforms that support it, and would continue to have the old weaker guarantee on other platforms until such time as those support it.

@sfackler
Copy link
Member

I'd be happy with that language.

Also mention that where possible, `hard_link` does not follow symlinks.
@sunfishcode
Copy link
Member Author

I've now updated the comments to take a "if such a thing is possible on the platform" stance.

@rfcbot
Copy link

rfcbot commented Oct 23, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 23, 2020
@dtolnay
Copy link
Member

dtolnay commented Oct 23, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Oct 23, 2020

📌 Commit d0178b4 has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 23, 2020
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 7, 2020
…olnay

Define `fs::hard_link` to not follow symlinks.

POSIX leaves it [implementation-defined] whether `link` follows symlinks.
In practice, for example, on Linux it does not and on FreeBSD it does.
So, switch to `linkat`, so that we can pick a behavior rather than
depending on OS defaults.

Pick the option to not follow symlinks. This is somewhat arbitrary, but
seems the less surprising choice because hard linking is a very
low-level feature which requires the source and destination to be on
the same mounted filesystem, and following a symbolic link could end
up in a different mounted filesystem.

[implementation-defined]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/link.html
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 7, 2020
…olnay

Define `fs::hard_link` to not follow symlinks.

POSIX leaves it [implementation-defined] whether `link` follows symlinks.
In practice, for example, on Linux it does not and on FreeBSD it does.
So, switch to `linkat`, so that we can pick a behavior rather than
depending on OS defaults.

Pick the option to not follow symlinks. This is somewhat arbitrary, but
seems the less surprising choice because hard linking is a very
low-level feature which requires the source and destination to be on
the same mounted filesystem, and following a symbolic link could end
up in a different mounted filesystem.

[implementation-defined]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/link.html
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 7, 2020
…olnay

Define `fs::hard_link` to not follow symlinks.

POSIX leaves it [implementation-defined] whether `link` follows symlinks.
In practice, for example, on Linux it does not and on FreeBSD it does.
So, switch to `linkat`, so that we can pick a behavior rather than
depending on OS defaults.

Pick the option to not follow symlinks. This is somewhat arbitrary, but
seems the less surprising choice because hard linking is a very
low-level feature which requires the source and destination to be on
the same mounted filesystem, and following a symbolic link could end
up in a different mounted filesystem.

[implementation-defined]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/link.html
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 8, 2020
…olnay

Define `fs::hard_link` to not follow symlinks.

POSIX leaves it [implementation-defined] whether `link` follows symlinks.
In practice, for example, on Linux it does not and on FreeBSD it does.
So, switch to `linkat`, so that we can pick a behavior rather than
depending on OS defaults.

Pick the option to not follow symlinks. This is somewhat arbitrary, but
seems the less surprising choice because hard linking is a very
low-level feature which requires the source and destination to be on
the same mounted filesystem, and following a symbolic link could end
up in a different mounted filesystem.

[implementation-defined]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/link.html
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 8, 2020
…olnay

Define `fs::hard_link` to not follow symlinks.

POSIX leaves it [implementation-defined] whether `link` follows symlinks.
In practice, for example, on Linux it does not and on FreeBSD it does.
So, switch to `linkat`, so that we can pick a behavior rather than
depending on OS defaults.

Pick the option to not follow symlinks. This is somewhat arbitrary, but
seems the less surprising choice because hard linking is a very
low-level feature which requires the source and destination to be on
the same mounted filesystem, and following a symbolic link could end
up in a different mounted filesystem.

[implementation-defined]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/link.html
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 9, 2020
Rollup of 12 pull requests

Successful merges:

 - rust-lang#77640 (Refactor IntErrorKind to avoid "underflow" terminology)
 - rust-lang#78026 (Define `fs::hard_link` to not follow symlinks.)
 - rust-lang#78114 (Recognize `private_intra_doc_links` as a lint)
 - rust-lang#78228 (Promote aarch64-unknown-linux-gnu to Tier 1)
 - rust-lang#78345 (Fix handling of item names for HIR)
 - rust-lang#78437 (BTreeMap: stop mistaking node for an orderly place)
 - rust-lang#78476 (fix some incorrect aliasing in the BTree)
 - rust-lang#78674 (inliner: Use substs_for_mir_body)
 - rust-lang#78748 (Implement destructuring assignment for tuples)
 - rust-lang#78868 (Fix tab focus on restyled switches)
 - rust-lang#78878 (Avoid overlapping cfg attributes when both macOS and aarch64)
 - rust-lang#78882 (Nicer hunk headers for rust files)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 41134be into rust-lang:master Nov 9, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 9, 2020
@sunfishcode sunfishcode deleted the symlink-hard-link branch November 9, 2020 03:25
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Nov 17, 2020
std: Fix test `symlink_hard_link` on Windows

The test was introduced in rust-lang#78026 and fails depending on Windows version and admin rights.
Other similar tests check for symlink creation permissions before doing anything, this PR performs the same check for `symlink_hard_link` as well.
sunfishcode added a commit to sunfishcode/rust that referenced this pull request Feb 11, 2021
Following rust-lang#78026, `std::fs::hard_link` on most platforms does not follow
symlinks. Change the WASI implementation to also not follow symlinks.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 20, 2021
Make WASI's `hard_link` behavior match other platforms.

Following rust-lang#78026, `std::fs::hard_link` on most platforms does not follow
symlinks. Change the WASI implementation to also not follow symlinks.

r? `@alexcrichton`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 23, 2021
Make WASI's `hard_link` behavior match other platforms.

Following rust-lang#78026, `std::fs::hard_link` on most platforms does not follow
symlinks. Change the WASI implementation to also not follow symlinks.

r? ``@alexcrichton``
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 23, 2021
Make WASI's `hard_link` behavior match other platforms.

Following rust-lang#78026, `std::fs::hard_link` on most platforms does not follow
symlinks. Change the WASI implementation to also not follow symlinks.

r? ```@alexcrichton```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-io Area: `std::io`, `std::fs`, `std::net` and `std::path` disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.