rust-lang/wg-cargo-std-aware

Usage of `--sysroot` may still be racy and/or allow false dependencies

alexcrichton opened this issue · 6 comments

I'm realizing now that after we've implemented --sysroot that we may still be susceptible races where you can depend on a sysroot crate without actually declaring that you depend on it. Let's say that #5 has already been implemented, and you say you only depend on core, I think you could also depend on alloc via the following:

  • First, you build your project. Something else depends on alloc, but your crate fails to build.
    • The alloc crate, however, did actually end up finish building by the time your crate received its error.
  • Next, you run cargo build again.
  • This time, very quickly, alloc is linked into the sysroot
  • Your crate, which has extern crate alloc, now "magically works"

I think the problem here is that we're giving, via --sysroot, crates access to anything in the sysroot. Cargo doesn't actually have any control over what can be loaded from the sysroot. While this will probably only rarely come up in practice, I think we may want to strive to have Cargo have a more intrusive understanding of what crates are where in the sysroot.

Assuming that #5 is implemented I think we'll have some sort of distinction between crates that have sysroot dependencies or not. For example your crate will either declare its sysroot dependencies explicitly, or you won't say anything (like all crates do today) and we'll have some default behavior (like assuming you can use up to proc_macro). That puts us in one of two buckets:

  • If dependencies are listed, we don't use --sysroot. We instead use --extern always. This should be fine since the feature isn't implemented yet! If you're building in -Zbuild-std mode Cargo has a path to pass, otherwise Cargo uses --extern std to activate the name resolution logic to work the same as if --extern name=path was passed.
  • If dependencies aren't listed, we instead use a new and invented --extern-sysroot flag to point to the compiled crates. This has the same name resolution logic that sysroot crates have today (wrt shadowing via macros and stuff like that).

The invented --extern-sysroot flag here would take the form --extern-sysroot name=path. In all cases Cargo would pass -L dependency=... for the sysroot, and the --extern flags would configure which crates can be loaded from the sysroot. The only reason for --extern-sysroot to exist is to match the name resolution behavior of sysroot crates today since there are subtle differences. Alternatively we could fix rustc so there are no differences!

ehuss commented

I have implemented a branch that removes --sysroot, and instead adds "options" to the --extern flag. In this case, it is --extern noprelude:alloc=/path/to/deps/liballoc.rlib. The "noprelude" option tells rustc not to add to the prelude which addresses the original concern in #40 that made us switch. It also is more careful about which crates it passes to rustc via --extern. It seems to work well.

Any objections to moving in that direction?

If that is OK, the next question I have: Should I prototype a new flag in rustc to disable crate searches in the sysroot? This would help with avoiding the dreaded "duplicate lang item" error if you have an errant extern crate for something you shouldn't have access to (and instead show E0463 or E0433 failed to resolve error). I think it might be a good idea, and would also ensure that things like rust-lld are still found. However, I recognize that it's a minor error message improvement, so may not be important enough. I'm not sure how careful rustc is about the proliferation of random flags. If that sounds ok, any opinion on the flag name? Maybe --no-sysroot-search?

Any objections to moving in that direction?

👍 from me!

Should I prototype a new flag in rustc to disable crate searches in the sysroot?

👍 from me as well

The name of --no-sysroot-search sounds good to me!

I think this should be closed after rust-lang/cargo#7699?

ehuss commented

I've been leaving it open because --no-sysroot-search still needs to be implemented.

Isn't that what #31 is, or am I missing something?

ehuss commented

Oh, good point, closing for #31.