Fix linker error

[inquisitivecrystal]

Currently, fs::hard_link determines whether platforms have linkat based on the OS, and uses link if they don't. However, this heuristic does not work well if a platform provides linkat on newer versions but not on older ones. On old MacOS, this currently causes a linking error.

This commit fixes fs::hard_link by telling it to use weak! on macOS. This means that, on that operating system, we now check for linkat at runtime and use link if it is not available.

Fixes #80804.

@rustbot label T-libs-impl

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[inquisitivecrystal]

Actually, I just thought of one potential problem. I'm not sure if it's a problem, and if so how to fix it. This may cause the symlink_hard_link() test to fail if Rust is built on MacOS version 10.9. That's better than the current behavior (currently, I think it would fail to link at all), but possibly not ideal.

[inquisitivecrystal]

It's been pointed out to me on Zulip that I may want to change the way this works for Android. It's probably possible to use syscall to access linkat, in order to work around versions of libc that don't support linkat. I need to get comments somewhere to figure out what to do. Marking as a draft until then.

[inquisitivecrystal]

@kennytm: This is now ready for you to review. I'm sorry it wasn't before.

[inquisitivecrystal]

It doesn't look like kennytm has had a chance to review for a while (which is valid!), so asking for a review from someone else. I think I'm allowed to do that? I apologize if I'm not.

r? @yaahc

[yaahc]

Just did a first pass review and left some comments in the related zulip thread.

• link for reference: https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/Using.20weak!.20in.20fs.3A.3Ahard_link

[inquisitivecrystal]

Status report

The situation before this PR

At present, fs::hard_link uses a system call that is available on newer versions of macOS, but not on older versions (10.9 or lower). As a result, std will not link on older versions of macOS.

Before I move on, I want to call attention to the priority of this problem. #80804, the relevant issue, is P-high. std not building on a tier 1 platform would normally be P-critical, but the issue was tagged P-high because only very old versions of macOS are affected. It would be good to get this dealt with soonish, and it may be worth considering a backport to beta.

How we got here

fs::hard_link is std's utility for creating a hard link. On Unix systems, fs::hard_link used to use the link syscall. However, the link syscall follows symlinks on some systems but not others, causing inconsistency. #78026 replaced the link syscall where possible with the newer linkat syscall, which allows us to always not follow symlinks, guaranteeing consistent behavior. Three systems still use link, including Android, which will be relevant later on.

What this PR does

This PR changes the implementation of fs::hard_link for macOS. The new implementation checks at runtime whether linkat is present, using it if it is there and falling back on link if it is not. The implementation for this involves the weak! macro, but that's an implementation detail.

Unresolved problem 1: nonstandard behavior on older macOS

This PR would mean that fs::hard_link would follow symlinks on older versions of macOS. Unfortunately, #78026 added test a test called symlink_hard_link, which will only pass if symlinks are not followed. Thus, if we make it so that symlinks are followed on old versions of macOS, this test will fail on those versions. The unresolved question here is what to do about the test breakage. There are a few main options, though there may be others.

Option 1A: Allow the test to fail

This option means that, when the std tests are run on old macOS versions, the symlink_hard_link test would fail. This is what this PR currently does.

Pros: This is a minimal change. The test still runs on newer macOS versions. If we were willing to let std not build for nearly half a year on this system, a failing test is relatively minor.
Cons: A test fails on some versions of a tier-1 system.

Option 1B: Don't run the test on macOS

This option means that the symlink_hard_link test is always ignored on macOS. Unfortunately, there is no way I know of to make this version specific.

Pros: Tests always pass.
Cons: We don't test the desired behavior on newer macOS versions.

Option 1C: Write a special version of the test for macOS

With a bit of work, we might be able to write a version of the symlink_hard_link test specifically for macOS, which checks whether linkat is available when it's run and only runs the real test if it is.

Pros: Tests always pass.
Cons: We have to write a weird, slightly messy alternative version of a test just for one operating system.

Unresolved problem 2: what to do with Android

This problem is minor: all the options work fine and the differences are marginal.

linkat is only in libc on newer versions of Android, although it is available in the kernel even on older versions. The current behavior is to use link on all versions of Android. Using link doesn't cause the same problems on Android as on macOS, because on Android link does what we want it to. However, if we wanted to, we could use the syscall! macro to call linkat on all versions of Android, performing a direct syscall when linkat isn't in libc. This option isn't available on macOS.

Option 2A: Don't change Android

This option suggests we continue using link on Android. This is what the PR currently does.

Pros: It isn't broken, so why fix it? This may have marginally better performance.
Cons: This is, in a sense, less correct than the alternative.

Note: I'm in favor of this option. I would prefer to fix the problem on macOS and consider making to changes to Android out of scope for this PR. However, I am willing to do whatever the libs team wants me to do.

Option 2B: Use syscall! and linkat on Android

This option suggests we use syscall! to invoke linkat on android.

Pros: This is in a sense more correct, given that it does not rely on platform specific behavior.

Cons: This means that the system call is resolved dynamically instead of statically, which may hurt performance a bit. This is a change on a system that's already working fine.

@rustbot label +I-needs-decision -S-waiting-on-review +S-waiting-on-team

[joshtriplett]

Is there any chance the link behavior could lead to a security issue in a caller of fs::hard_link?

If so, we need some reasonable way for the caller to make sure they use linkat or fall, and the test failure shouldn't be worked around.

If not, just disabling that test on old macOS seems fine.

[inquisitivecrystal]

@joshtriplett Technically, there could be a security problem if a file that the program expects to not be a link is replaced by a link and the function creates a hard link to the wrong file. However, I don't think this is really worth worrying about. The fs::hard_link documentation is very clear that we do not guarantee that symlinks won't be followed. Additionally, I don't think any of our other filesystem functions make any guarantees at all about symlinks, so it would be strange to treat this case differently from the others.

Actually, it looks like I need to update the platform specific behavior section of that documentation. I'll do that along with the test change.

[inquisitivecrystal]

@joshtriplett I have fixed the test as you suggested.

To make it work, I needed to use weak! for the test, so that I could disable it on old macOS systems that don't have. That posed a bit of a problem, since the test is in a different std module than weak!. Making weak! available to that module would have required rearranging module declarations in std/src/lib.rs. At least, I couldn't find any other way of doing it. That was a bit too evil for my taste.

I fixed the problem by making weak! use macros 2.0. I also brought the syscall! macro along with it, since having two macros declared in the same small file use different macro engines would just have been confusing. This necessitated some minor changes to add imports to other files, but at least the result is reasonably clean. (Well, apart from the documentation hack, which I don't really know how to improve on. I certainly don't want to write actual docs for these macros.)

I would appreciate the opportunity to do an interactive rebase before final merging, since the commit history is currently an unholy mess. Still, I think this is ready for review.

@rustbot label -I-needs-decision -S-waiting-on-team +S-waiting-on-review

[joshtriplett]

We discussed this in today's @rust-lang/libs meeting. We agreed to merge this change to provide interim support for older macOS versions:

@bors r+

We also agreed that we should start the conversation about how much longer to support older macOS versions, following the process for the target tier policy.

[bors]

📌 Commit dac351ea8a010ed3bcddfb88a76dab4485b52927 has been approved by joshtriplett

[bors]

⌛ Testing commit 64b61ac10a6425710e2f337f37a79c891d3a0afb with merge c7a3413527a32bc7c9c7546c4e251a4c5246ceba...

[bors]

💥 Test timed out

[inquisitivecrystal]

@bors retry

[inquisitivecrystal]

@bors r-

There's an utterly minor nit that's been bothering me, and if the tests need to be rerun anyway, this is as good a time as any to fix it.

[inquisitivecrystal]

Okay, now that the imports in that one spot are alphabetized (look, it was really bothering me)...

@bors r=joshtriplett

[bors]

📌 Commit 5999a5f has been approved by joshtriplett

[bors]

⌛ Testing commit 5999a5f with merge dfd7b8d...

[bors]

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing dfd7b8d to master...

[inquisitivecrystal]

Yay! Finally merged!

If this is accepted for backporting to beta, which I do think is a good idea, only dc38d87 and 5022c06 should be backported. Limiting the backport to those two commits will vastly decrease the chance of inadvertently breaking the build.

[yaahc]

Hey @inquisitivecrystal, we discussed backporting this PR in the last Library team meeting and decided against it on the basis that this breakage occurred 6 months ago and hasn't seen many people complaining about it so we're assuming its relatively low impact. That plus the fact that devs can either pin to an old stable version or update to nightly to get a compiler that does work on the old macos platforms made us prefer to avoid even doing a risk analysis on the back port and just wait a few weeks until the fix hits stable again. If there are any issues that we missed please let me know and thank you again for the PR!

[inquisitivecrystal]

@yaahc Thank you very much for considering my nomination!

There are a few people eagerly waiting for this fix: some of them commented on #80804. It is true that there don't seem to be all that many people who need this though.

I'd still personally do it, just on the basis that #80804 is a P-high and ideally would have been dealt with sooner. It feels a bit odd that a delay in fixing a problem would cause it to effectively become lower priority. But the number of users this is causing breakage for has to be fairly small, and it may not be worth backporting for that reason, especially this close to release.

TL;DR: I'd backport, but I see why libs might not want to. If you don't feel that I'm bringing up anything that would change the team's mind, and it's sounding like I'm not, don't worry about it. I totally understand.

Thank you again for all the support throughout the process. I've greatly enjoyed the experience of contributing to the standard library, and it has been fun enough that I'm going to keep doing it again in future. :)