Fix rustc sysroot in systems using CAS #79253

rcvalle · 2020-11-21T04:13:22Z

Change filesearch::get_or_default_sysroot() to check if sysroot is found using env::args().next() if rustc in argv[0] is a symlink; otherwise, or if it is not found, use env::current_exe() to imply sysroot. This makes the rustc binary able to locate Rust libraries in systems using content-addressable storage (CAS).

rust-highfive · 2020-11-21T04:13:24Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (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.

jyn514 · 2020-11-21T13:50:53Z

r? @Mark-Simulacrum

jyn514 · 2020-11-21T13:51:47Z

@rcvalle what is content-addressable storage? Do you mean like a nix store or something?

Mark-Simulacrum · 2020-11-22T21:42:24Z

I don't think I can effectively review this patch without more information -- switching away from current_exe feels like the wrong approach though. If it was a "true" content-addressable storage system I would expect paths to be largely meaningless, so the popping that is still present wouldn't work really either...?

rcvalle · 2020-11-24T04:10:14Z

@rcvalle what is content-addressable storage? Do you mean like a nix store or something?

Content-addressable storage (CAS) is a method of storing information so it can be retrieved based on its content (usually using a hashing function appropriate for CAS) instead of its location. (Git itself is an userspace CAS filesystem.)

rcvalle · 2020-11-24T04:11:06Z

I don't think I can effectively review this patch without more information -- switching away from current_exe feels like the wrong approach though. If it was a "true" content-addressable storage system I would expect paths to be largely meaningless, so the popping that is still present wouldn't work really either...?

Sorry, I should have provided more information. We use CAS in our build systems with symlink forests to replicate the locations files originally belong. However, since both env::current_exe() and fs::canonicalize() follow symlinks/canonicalize the components of the path, the symlinks were being followed to the CAS locations, making the rustc binary unable to locate its distributed libraries.

Mark-Simulacrum · 2020-11-24T13:48:56Z

Hm, ok. I'm not sure I'm willing to land this change -- it seems like there are cases where that canonicalization would be necessary. Generally the sysroot discovery is a bit ad-hoc in rustc and not super well polished. I think my suggestion would be that we try both - it looks like perhaps the sysroot_candidates function can be extended to also visit the non-canonicalized path. I'm not actually quite sure why we don't use that function to discover the sysroot in librustc_session (cc @bjorn3 since you modified the sysroot code recently), but instead just call the current_exe-based function.

bjorn3 · 2020-11-24T14:25:16Z

If I run rustc a.rs, env::args().next() would return rustc, right? In that case it will look for the sysroot in ../, which is unlikely to be the real sysroot location.

However, since it can be set to an arbitrary value, fs::get_or_default_sysroot() and its return value should not be relied on for security-related decisions.

Rustc runs with the same permissions as the user. I personally don't see how security is relevant here. It can only be invoked by the user. Also the sysroot is most of the time only used to load the files the crate will link against. Only with -Zcodegen-backend can it dynamically link against a dylib in the sysroot.

bjorn3 · 2020-11-24T14:33:33Z

sysroot_candidates calls get_or_default_sysroot. The former also searches relative to the rustc_driver dylib for custom drivers, while the later only searches relative to the main executable. sysroot_candidates was necessary when cg_llvm was a hotpluggable codegen backend. Otherwise custom drivers wouldn't find the llvm codegen backend.

rcvalle · 2020-12-01T02:38:50Z

If I run rustc a.rs, env::args().next() would return rustc, right? In that case it will look for the sysroot in ../, which is unlikely to be the real sysroot location.

Indeed. We could perhaps check if sysroot is found using env::args().next(), and if it's not found, use env::current_exe(). What do you think?

However, since it can be set to an arbitrary value, fs::get_or_default_sysroot() and its return value should not be relied on for security-related decisions.

Rustc runs with the same permissions as the user. I personally don't see how security is relevant here. It can only be invoked by the user. Also the sysroot is most of the time only used to load the files the crate will link against. Only with -Zcodegen-backend can it dynamically link against a dylib in the sysroot.

It may be relevant in a multiuser or remote system, where the rustc binary may be executed by a different user with different privileges.

rcvalle · 2020-12-11T03:58:33Z

@bjorn3 @Mark-Simulacrum I've amended my commit to check if sysroot is found using env::args().next(), and if it's not found, use env::current_exe() instead. What do you think?

rcvalle · 2020-12-16T15:39:02Z

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Mark-Simulacrum · 2020-12-23T16:28:33Z

compiler/rustc_session/src/filesearch.rs

+    // This makes the rustc binary able to locate Rust libraries in systems
+    // using content-addressable storage (CAS). However, since it can be set to
+    // an arbitrary value, fs::get_or_default_sysroot() and its return value
+    // should not be relied on for security-related decisions.


This last sentence seems misleading, as it applies just as much to from_current_exe. I would perhaps expect to see it on the get_or_default_sysroot function itself.

Done. (I improved the changes a bit and fixed the comments.)

Mark-Simulacrum · 2020-12-23T16:39:23Z

I am still concerned about this change. It seems like it could easily lead to confusion if there are (by accident or intentionally) different sysroots at argv0 and current_exe. That said, it does seem like the problem you're running into is a real one - I just am not sure this is the right solution. Can you say more about how other compilers or programs that need more than one file deal with this in your system? I think overall I just don't have enough expertise to weigh in here. cc @rust-lang/compiler - maybe someone can chime in if they have experience or thoughts here.

wesleywiser · 2020-12-24T02:12:39Z

Perhaps we should consider adding a flag that would allow specifying the sysroot path instead?

jyn514 · 2020-12-24T02:15:07Z

@wesleywiser there's already a --sysroot flag:

$ rustc --help -v | grep sysroot
        --print [crate-name|file-names|sysroot|target-libdir|cfg|target-list|target-cpus|target-features|relocation-models|code-models|tls-models|target-spec-json|native-static-libs]
        --sysroot PATH  Override the system root

wesleywiser · 2020-12-24T02:21:20Z

Thanks @jyn514! GitHub hid this comment which has the motivation for the change.

bors · 2021-01-31T13:03:03Z

⌛ Testing commit 3f679fe with merge 9a61ec508e22d4488fa01c424e3bd85bb7d02027...

rust-log-analyzer · 2021-01-31T14:13:54Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2021-01-31T14:13:54Z

💔 Test failed - checks-actions

rcvalle · 2021-02-03T03:17:02Z

Thank you @nagisa, @Mark-Simulacrum, @jyn514, @bjorn3, and others for your time on this! Much appreciated.

rcvalle · 2021-02-03T03:17:56Z

@bors retry

bors · 2021-02-03T03:17:58Z

@rcvalle: 🔑 Insufficient privileges: not in try users

jyn514 · 2021-02-03T03:18:57Z

@bors retry

…gisa Fix rustc sysroot in systems using CAS Change filesearch::get_or_default_sysroot() to check if sysroot is found using env::args().next() if rustc in argv[0] is a symlink; otherwise, or if it is not found, use env::current_exe() to imply sysroot. This makes the rustc binary able to locate Rust libraries in systems using content-addressable storage (CAS).

bors · 2021-02-05T22:58:21Z

⌛ Testing commit 3f679fe with merge 16b8057...

bors · 2021-02-06T01:46:28Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 16b8057 to master...

rust-highfive assigned eddyb Nov 21, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 21, 2020

rust-highfive assigned Mark-Simulacrum and unassigned eddyb Nov 21, 2020

jyn514 added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Nov 21, 2020

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 22, 2020

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 16, 2020

Mark-Simulacrum reviewed Dec 23, 2020

View reviewed changes

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 31, 2021

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 Feb 3, 2021

m-ou-se mentioned this pull request Feb 3, 2021

Rollup of 5 pull requests #81707

Closed

Dylan-DPC-zz mentioned this pull request Feb 3, 2021

Rollup of 11 pull requests #81733

Closed

m-ou-se mentioned this pull request Feb 4, 2021

Rollup of 12 pull requests #81748

Closed

m-ou-se mentioned this pull request Feb 4, 2021

Rollup of 14 pull requests #81754

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 6, 2021

bors merged commit 16b8057 into rust-lang:master Feb 6, 2021

rustbot added this to the 1.51.0 milestone Feb 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rustc sysroot in systems using CAS #79253

Fix rustc sysroot in systems using CAS #79253

rcvalle commented Nov 21, 2020 •

edited

Loading

rust-highfive commented Nov 21, 2020

jyn514 commented Nov 21, 2020

jyn514 commented Nov 21, 2020

Mark-Simulacrum commented Nov 22, 2020

rcvalle commented Nov 24, 2020

rcvalle commented Nov 24, 2020

Mark-Simulacrum commented Nov 24, 2020 •

edited

Loading

bjorn3 commented Nov 24, 2020

bjorn3 commented Nov 24, 2020

rcvalle commented Dec 1, 2020 •

edited

Loading

rcvalle commented Dec 11, 2020

rcvalle commented Dec 16, 2020

Mark-Simulacrum Dec 23, 2020

rcvalle Jan 6, 2021

Mark-Simulacrum commented Dec 23, 2020

wesleywiser commented Dec 24, 2020

jyn514 commented Dec 24, 2020 •

edited

Loading

wesleywiser commented Dec 24, 2020

bors commented Jan 31, 2021

rust-log-analyzer commented Jan 31, 2021

bors commented Jan 31, 2021

rcvalle commented Feb 3, 2021

rcvalle commented Feb 3, 2021

bors commented Feb 3, 2021

jyn514 commented Feb 3, 2021

bors commented Feb 5, 2021

bors commented Feb 6, 2021

Fix rustc sysroot in systems using CAS #79253

Fix rustc sysroot in systems using CAS #79253

Conversation

rcvalle commented Nov 21, 2020 • edited Loading

rust-highfive commented Nov 21, 2020

jyn514 commented Nov 21, 2020

jyn514 commented Nov 21, 2020

Mark-Simulacrum commented Nov 22, 2020

rcvalle commented Nov 24, 2020

rcvalle commented Nov 24, 2020

Mark-Simulacrum commented Nov 24, 2020 • edited Loading

bjorn3 commented Nov 24, 2020

bjorn3 commented Nov 24, 2020

rcvalle commented Dec 1, 2020 • edited Loading

rcvalle commented Dec 11, 2020

rcvalle commented Dec 16, 2020

Mark-Simulacrum Dec 23, 2020

Choose a reason for hiding this comment

rcvalle Jan 6, 2021

Choose a reason for hiding this comment

Mark-Simulacrum commented Dec 23, 2020

wesleywiser commented Dec 24, 2020

jyn514 commented Dec 24, 2020 • edited Loading

wesleywiser commented Dec 24, 2020

bors commented Jan 31, 2021

rust-log-analyzer commented Jan 31, 2021

bors commented Jan 31, 2021

rcvalle commented Feb 3, 2021

rcvalle commented Feb 3, 2021

bors commented Feb 3, 2021

jyn514 commented Feb 3, 2021

bors commented Feb 5, 2021

bors commented Feb 6, 2021

rcvalle commented Nov 21, 2020 •

edited

Loading

Mark-Simulacrum commented Nov 24, 2020 •

edited

Loading

rcvalle commented Dec 1, 2020 •

edited

Loading

jyn514 commented Dec 24, 2020 •

edited

Loading