cargo
cargo copied to clipboard
rust-lang
Consider making the `src` cache read-only.
Registry dependencies get extracted into cargo home (in the src directory) with whatever metadata is in the tar file. One issue with this is that the files are usually writeable. This can cause a problem if the user accidentally modifies these files, which breaks cargo's assumption that they are immutable and reusable. One way this can happen is that in some editors, when there is an error or warning, they may open those files to display the error/warning (particularly with macros). The user may not realize that this is from a remote location, and may not understand the consequence of making changes.
We may want to consider making those files read-only when extracting them. This would help with confusing situations where the src cache is inadvertently corrupted.
This would not protect from general filesystem corruption, which is also an issue. This is also an issue with git dependencies, which may be more difficult to adjust permissions on.
There is some risk that this would introduce new problems. For example, if people are using tools to clean the src directory, and those tools have trouble with read-only files.
@matthiaskrgr would making files read-only give any problems to cargo-cache?
Oh that's an interesting idea!
While I fully agree that it makes sense to make the cache (or at least .cargo/registry/src and .cargo/git/checkout) write-only, it does indeed prevent cargo-cache from cleaning up anything;
$ cargo cache --autoclean
Clearing cache...
Warning: failed to recursively remove directory "/home/matthias/.cargo/registry/src".
error: Os { code: 13, kind: PermissionDenied, message: "Permission denied" }
Warning: failed to recursively remove directory "/home/matthias/.cargo/git/checkouts".
error: Os { code: 13, kind: PermissionDenied, message: "Permission denied" }
BUT I am already happy that it does not panic :D
If it turned out that cargo-cache does modify the cache in any way (anything that goes beyond just removing whole files) I would indeed consider this a bug.
Right now I am just running remove_dir_all::remove_dir_all() on directories without doing any checks whatsoever.
https://crates.io/crates/remove_dir_all
I never had a read-only cache in mind when writing cargo-cache but adding a little bit of extra logic which tries to change file permission to read-write before removing them does not sound too difficult, assuming that we have a crate around that handles file permissions on linux, macos and windows.
I'm bringing up a new rust user on my team, and I found that they ended up editing some files in $CARGO_HOME (which is actually quite easy to do with VSCode+RA) to add some debugging messages. A read-only src cache might have given them a sufficient hint that what they were doing was not the right approach.
Just did a PoC and benchmarks with rust-lang/cargo itself. It turns out that there is a slight performance hit (~5%) on macOS. It's in expectation, as there are some extra permission operations on every file for the first time checking out/unpacking a crate (Windows might do it better since file permissions can be inherited, though).
However, it does get new problems 🤯. When I removed sources residing in ~/.cargo/registry/src and re-run the build again, I got a permission denied error.
$ ./target/release/cargo b
$ rm -rf ~/.cargo/registry/src
$ ./target/release/cargo b
error: failed to run custom build command for `curl-sys v0.4.55+curl-7.83.1`
Caused by:
process didn't exit successfully: `/Users/myuser/projects/cargo/target/debug/build/curl-sys-f4af01668b6e1680/build-script-build` (exit status: 101)
--- stdout
...
--- stderr
fatal: not a git repository (or any of the parent directories): .git
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 13, kind: PermissionDenied, message: "Permission denied" }', /Users/myuser/.cargo/registry/src/github.com-1ecc6299db9ec823/curl-sys-0.4.55+curl-7.83.1/build.rs:93:10
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
I guess its curl-sys's build script that uses fs::copy to copy contents from registry/src to build script's OUT_DIR, but unfortunately fs::copy also copies permission bits. Any rebuild that triggers a re-run of the same build-script binary might fail because it cannot fs::copy from cache into existing readonly files under target-dir. I am afraid that more cases in the wild will be broken if registry/src becomes readonly. Here is the search result that a build.rs contains fs::copy throughout GitHub.
Personally, I love to see this enhancement landed, but it seems a bit risky to me after the analysis.
That's really unfortunate, I hadn't thought about that.
I'm wondering if a compromise would be to only adjust permissions on packages that don't have a build script? I don't know if it would be easy to detect that (that might require parsing Cargo.toml, which might be too much overhead). I also don't know what % of packages that would cover.
It's also unfortunate that the tar API doesn't allow us to override the permissions directly. Another option is to make the files readonly when creating the .crate file. That would only help with newly published packages, but might be an option to consider.
Given that most of these cases would be caught by crater, maybe its worthwhile to just make the change and make sure crates are fixed up?
Alternatively: maybe only apply this for crates published after a certain timestamp?
@weihanglo Asked about how to do a crater run to test the impact of changing the readonly status. The rough steps are:
- In your clone of cargo, make the changes to incorporate the new behavior.
- Get a clone of https://github.com/rust-lang/rust/
- Check out a branch to add your changes.
- Modify
.gitmodulesto point to your clone and branch of cargo with the changes you want to test. For example:git submodule set-url src/tools/cargo https://github.com/ehuss/cargo.git git submodule set-branch --branch my-awesome-feature src/tools/cargo git submodule update --remote src/tools/cargo git add .gitmodules src/tools/cargo git commit - Push the changes to GitHub and make a PR . Clearly label the PR as an experiment, and assign yourself or @ghost.
- Make a "try" build. I think all cargo members have this permission. Write a comment
@bors try. - After the try build finishes (an hour? I forget), ask someone to make a crater run. The Cargo team does not have that permission, so just ask someone on Zulip.
I think doing a "check" run should be sufficient? I'm concerned that by default it will not get dev-dependencies, so maybe add the --all-targets flag? I'm not sure if that's worth it. There's a bunch of documentation at https://github.com/rust-lang/crater/blob/master/docs/bot-usage.md on crater commands. You may also consider doing a top-100 run first just to make sure the experiment is configured properly.
I still feel that it might be good to ease into the change, such as only marking read-only if there are no build scripts. We discussed some other options to consider:
- Adding some kind of option so that build-scripts can say "don't mark me read-only" or "don't mark these directories read-only".
- Make all packages read-only on an edition boundary. This wouldn't be able to support automatic migrations, which conflicts with the spirit of Editions being easy to transition.
Doing a crater run can maybe give us a sense of the scale of the problem.
This could be limited to only *.rs files, leaving other data files with a writeable bit.
As suggested here, a possible alternative is to compute a hash when downloading a dependency, and to verify that hash whenever Cargo needs to access that dependency.
Somewhat related, we might consider sanitizing the executable bit as well, if we consider distribution of compiled executables via crate tarballs an unsupported use-case.
If these files are made readonly, what would the process be for updating dependencies in the cache? i.e. if cargo update detects a new version of a crate in the cache, it will need write permissions to update that crate.
Will cargo temporarily modify the file permissions before performing an update? On unix systems it might make more sense for cargo to run as its own user. Other users have read-only access, but the cargo user has write access. Thoughts?
Every crate version is stored in a separate directory. This is necessary to be able to cache multiple crate versions at once. And because of this we never need to modify the directory containing the cached crate after we are done unpacking the downloaded tarball.
On unix systems it might make more sense for cargo to run as its own user.
There is no way to do that as an unprivileged user. And requiring root permission to install rust would be a really bad idea.
Cargo doesn't update dependencies at per-file-level. Cargo basically downloads new tarballs, unpacks them, and put files in separate directories.
Ahh... I should have seen that. In that case, disregard my comment. I am fully in support of making the registry src files read-only.
We can in addition or instead of ownership or permission bits, use OS-specific flags for finer-grained semantics, like chattr to make files immutable under Linux. Packages that try to break these restrictions will often be detected and the fixes will benefit even users of other operating systems.
@sanmai-NL could you elaborate more on how the approach resolves the concern in https://github.com/rust-lang/cargo/issues/9455#issuecomment-1143026498? It may still fail people's builds, no?
The immutable flag on linux can only be set by root as it prevents even root from modifying it.
@weihanglo Better performance can be attained by setting the right inode metadata along with file creation. Can you test setting a umask to make newly created files by Cargo read-only by default? The immutable bit tip was more of an implementation enhancement suggestion on attributes other than performance.
As for the breakage, I think that it's a defect to perform these file manipulations in build scripts.
I don't know if it is possible within the Cargo architecture to restrict build scripts to not perform some actions. That would be good in general.
The umask is process global, but someone may be creating files concurrently with cargo when using cargo as library. Also for directories setting the umask I believe will require using chmod anyway to make them writable again while creating files inside the directory. And on Unix I'm not too worried about a single extra chmod per file and directory. On Windows I would be more concerned, but the Windows kernel doesn't have anything like umask afaik. The CRT supports the function, but I wouldn't be surprised if it internally calls the equivalent of chmod after every file creation.
This could be opt-in per crate. Cargo currently makes tarballs with -rw-r--r-- permissions (ignores original files' permissions). Cargo could be taught to make tarballs with -r--r--r-- permissions, and preserve that when unarchiving.
Just an idea here off the top of my head, but why extract source archives at all? Can't we get their contents as needed, in-memory? That may sound costly but I wonder how often that really happens and if extracting them amortizes well (the extra cost being the filesystem operations that notably result in syscalls). That would reduce the number of files to be created and changed, so even particular file metadata like immutable flag can be set at the cost of $O(n)$ instead of $O(nm)$ time in the average case, where $n$ is the number of crates (tar archives) and $m$ is the number of files per crate.
This could be opt-in per crate. Cargo currently makes tarballs with
-rw-r--r--permissions (ignores original files' permissions). Cargo could be taught to make tarballs with-r--r--r--permissions, and preserve that when unarchiving.
@kornelski
https://docs.rs/tar/latest/tar/struct.Entry.html#method.set_mask
Can't we get their contents as needed, in-memory?
Rustc currently doesn't support that. We could add it, but build scripts and proc macros will still need it extracted somewhere on the filesystem.
Can't we get their contents as needed, in-memory?
Tools communicate by filesystem paths, so how do you propose to send an in-memory "path" to sccache for caching? Or coverage tools that render the coverage data with the source code? That's even if we do manage to ignore build.rs scripts that do…whatever they do. Without some cross-platform FUSE-like solution that exposes such paths to arbitrary other tools, this would restrict the current set of use cases. Maybe that's fine and can be opt-in, but opt-out is, I believe, only going to continue finding more and more corner cases.
The invention of not-a-tarball crate storage system is irrelevant here, because the problem is crates relying on files existing in the file system, and existing with specific permissions. If you succeed in removing the files from src dir, you'll break them even worse.
Can't we get their contents as needed, in-memory?
Tools communicate by filesystem paths, so how do you propose to send an in-memory "path" to
sccachefor caching? Or coverage tools that render the coverage data with the source code? That's even if we do manage to ignorebuild.rsscripts that do…whatever they do. Without some cross-platform FUSE-like solution that exposes such paths to arbitrary other tools, this would restrict the current set of use cases. Maybe that's fine and can be opt-in, but opt-out is, I believe, only going to continue finding more and more corner cases.
Does sccache cache source code and would that be important for Rust? The challenge would be more to let rustc compile tar archives, no?
The invention of not-a-tarball crate storage system is irrelevant here, because the problem is crates relying on files existing in the file system, and existing with specific permissions. If you succeed in removing the files from
srcdir, you'll break them even worse.
What is your opinion on this breakage? Would it preclude moving forward with a more performant design in any case?
Sccache doesn't cache source code. It reads the .d file emitted by rustc to know when it needs to recompile. If rustc were to compile tarballs, this .d file would simply mention the tarball and thus sccache would work as expected. For something entirely in-memory passed in through eg an fd or unix socket, that wouldn't work though.