Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
alexcrichton opened this issue Oct 2, 2019 · 6 comments · Fixed by rust-lang/cargo#7699
Closed

Comments

@alexcrichton
Copy link
Member

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
Copy link
Contributor

ehuss commented Nov 6, 2019

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?

@alexcrichton
Copy link
Member Author

alexcrichton commented Nov 6, 2019

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!

@Ericson2314
Copy link

Ericson2314 commented Jun 14, 2020

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

@ehuss
Copy link
Contributor

ehuss commented Jun 14, 2020

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

@Ericson2314
Copy link

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

@ehuss
Copy link
Contributor

ehuss commented Jun 14, 2020

Oh, good point, closing for #31.

@ehuss ehuss closed this as completed Jun 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants