cargo fix may attempt to write to the registry cache

[ehuss]

Problem
If rustc provides a suggestion to modify code in a dependency, cargo fix will happily oblige. However, cargo should never apply fixes to non-path dependencies. I might say it shouldn't apply fixes outside of the current package, but I'm not sure about that (or how to detect that reliably).

For example, it may attempt to write to ~/.cargo/registry/src/github.com-1ecc6299db9ec823 which really should be read-only.

rustc normally tries to avoid diagnostics that point into dependencies, but sometimes there are bugs where it points to the wrong span, and ends up giving a suggestion into a dependency. See rust-lang/rust#88514 for more detail.

Steps
It's hard to give a good example, since it either requires using a real crates.io package, or depending on diagnostics in rustc that are arguably not working correctly.

Method 1 — Use crates.io

With dependency proconio = "=0.3.8"

pub fn foo(
  source: proconio::source::line::LineSource<std::io::BufReader<&[u8]>>,
) {
  proconio::input! {
    from source,
    mut x:usize,
  }
}

This will suggest making a change to the remote package:

warning: unused variable: `x`
   --> /Users/eric/.cargo/registry/src/github.com-1ecc6299db9ec823/proconio-0.3.8/src/lib.rs:579:22
    |
579 |         let $($mut)* $var = $crate::read_value!(@source [$source] @kind [$($kind)*]);
    |                      ^^^^ help: if this is intentional, prefix it with an underscore: `_x`
    |
    = note: `#[warn(unused_variables)]` on by default

Method 2 — Sample macro

Put this macro in a dependency, and notice rustc will suggest modifying the macro.

#[macro_export]
macro_rules! m {
    ($i:tt) => {
        let $i = 1;
    };
}

The key part is that the identifier uses a tt instead of an ident.

Method 3 — cargo_test

This is a cargo testsuite test. I don't think we can use it since it is dependent on rustc's diagnostic output, but maybe we can simulate it with a rustc wrapper?

#[cargo_test]
fn fix_in_dependency() {
    Package::new("bar", "1.0.0")
        .file(
            "src/lib.rs",
            r#"
                #[macro_export]
                macro_rules! m {
                    ($i:tt) => {
                        let $i = 1;
                    };
                }
            "#,
        )
        .publish();

    let p = project()
        .file(
            "Cargo.toml",
            r#"
                [package]
                name = "foo"
                version = "0.1.0"

                [dependencies]
                bar = "1.0"
            "#,
        )
        .file(
            "src/lib.rs",
            r#"
                pub fn foo() {
                    bar::m!(abc);
                }
            "#,
        )
        .build();

    p.cargo("fix --allow-no-vcs")
        // This most definitely should not say "Fixed" in registry/src
        .with_stderr(
            "\
[UPDATING] `dummy-registry` index
[DOWNLOADING] crates ...
[DOWNLOADED] bar v1.0.0 [..]
[CHECKING] bar v1.0.0
[CHECKING] foo v0.1.0 [..]
[FIXED] [ROOT]/home/.cargo/registry/src/-88e05c124b5ecfe9/bar-1.0.0/src/lib.rs (1 fix)
warning: unused variable: `abc`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: `foo` (lib) generated 1 warning
warning: `foo` (lib test) generated 1 warning (1 duplicate)
[FINISHED] [..]
",
        )
        .run();
}

Possible Solution(s)
Cargo should detect diagnostic paths outside of the package and skip them.

Notes

Output of cargo version: cargo 1.56.0-nightly (f559c109c 2021-08-26)

[weihanglo]

Just jot it down: Tested method one and found that cargo 1.52 and later all got this issues.

[weihanglo]

IIUC it's hard to determine the package information since cargo-as-rustc does not know about workspace. Also, the emitted suggestions are from primary packages, so we cannot distinguish them by CARGO_PRIMARY_MANIFEST. 😞

Is there anything I can help investigate from the cargo side?

[ehuss]

I'm not certain the best approach for this, but one idea is to pass in the workspace root directory to cargo-as-rustc as an environment variable (we use __CARGO_FIX_ prefixes for env vars for passing information to the wrapper). Then, around here check if the path for the suggestion is within the workspace root, and if not, skip it.

I don't recall if starts_with is a reliable way to check if one path is a child of another. That is something to be careful about, especially with Windows oddities and case-insensitivity issues.

I think that should be fairly reliable? It is possible that people will do strange things like #[path] attributes or Cargo.toml definitions that point to source files outside of the workspace.

Another possibility is to check against CARGO_HOME and just skip anything that is within that directory. That sounds like it would probably be safer, so maybe that is a better choice.

[weihanglo]

I feel that going workspace-root way may lead to more problems in the future if we forget to take care of it. OTOH CARGO_HOME approach does not cover the local registry or path dependency cases. Despite that It's hard to choose a good one of these two options, I would go with the safer CARGO_HOME way since I believe this issue will be addressed in upstream 😆.

[weihanglo]

@rustbot claim

[epage]

For anyone else curious #9938 was a quick hack that prevents cargo fix from writing to anything in $CARGO_HOME. There is the (weird) risk of someone having code in $CARGO_HOME and there was the question in here if it was appropriate for cargo fix to modify all path dependencies.

One area of concern I thought of is vendored dependencies. Those should probably be untouched.

[ehuss]

Another issue that came up is that some suggestions are for the standard library if you have the rust-src component installed. Those should be protected as well. See rust-lang/rust#88514 (comment)

[weihanglo]

Another issue that came up is that some suggestions are for the standard library if you have the rust-src component installed. Those should be protected as well. See rust-lang/rust#88514 (comment)

Since cargo has no access to RustcTargetData to learn sysroot path in fix-proxy-mode, we may need to pass host sysroot path (i.e., <sysroot>/lib/rustlib/src/rust) as an environment variable to cargo-as-rustc-proxy. By doing so, we can safely skip that path. Cross compilation shouldn't affect the path of rust-src component I believe.

[ehuss]

Preferably I would like to get this fixed in rustc instead. I think it should not be too hard. rustc knows if spans are for the local crate. I'm wondering if the part of the code that adds suggestions could just filter out anything that has a non-local span. I have not investigated that, though.

[weihanglo]

Just opened #13792 as a temporary fix before rustc fix landed.