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

Cargo vendor doesn't replace all sources used by dependencies in its output #14729

Open
P-E-Meunier opened this issue Oct 25, 2024 · 4 comments
Labels
C-bug Category: bug S-triage Status: This issue is waiting on initial triage.

Comments

@P-E-Meunier
Copy link

Problem

Let's say we have three crates, A, B and C, where A depends on B and C and B depends on C.

We serve them using two identical registries x and y. Crate A has the following line in its Cargo.toml:

B = { version = ..., registry = "x" }
C = { version = ..., registry = "x" }

Now, let's also assume that the authors of B preferred registry y, B would have the following Cargo.toml:

C = { version = ..., registry = "y" }

Since the hashes of C, as served by x and y, are identical, cargo chooses only one source in Cargo.lock. In our example let's say it chooses x.

When running cargo vendor, the stdout shows what replacements are needed, and that stdout is meant to be consumed by Cargo itself when doing cargo build --offline.

Because the sources are taken from Cargo.lock, in our case the output will replace x only. While this seems intuitively correct (all crates are indeed vendored, and all their source registries are replaced), cargo build --offline still fails in our example, since the Cargo.toml of B mentions registry y.

Steps

No response

Possible Solution(s)

One fix could have been to tell cargo build to ignore source registries and only look for crate names and versions, but the same crate+version may appear multiple times in Cargo.lock if the different registries disagree on the hash.

A simpler fix, implemented by #14716, is to read the Cargo.toml of all dependencies when vendoring them to make sure we replace all the sources.

Notes

Now, I agree some parts of that explanation sound like shooting yourself in the foot:

  • Why would anyone mirror registries in this way rather than use the same domain name? Well, in our case this was caused by a migration to a different provider, with a transition period where both addresses were still in use.
  • Why would you want two different registries to serve the same crate name+version with different hashes? Now, that sounds like setting you up for really hard debugging sessions, but I suppose our mirror of B on x could have used C = { registry = "x" } to avoid this issue in the first place.

Version

Last commit on master.

@P-E-Meunier P-E-Meunier added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Oct 25, 2024
@P-E-Meunier
Copy link
Author

Minimal reproduction: https://github.com/P-E-Meunier/cargo-vendor-test

@Eh2406
Copy link
Contributor

Eh2406 commented Nov 14, 2024

Generally cargo uses Nominal typing (not Structural typing) for packages. That is to say that foo from crates.io does not equal foo from alternative registry, no matter how similar the artifacts are. In this case C from x is categorically not equal to C from y. This conceptual model was picked to avoid dependency confusion attacks. It comes with its advantages and disadvantages. I would find it rather confusing if A successfully built passing types defined in C to B. But then later stopped working because a new version of C is published to one registry and not the other.

However, from what you report, it looks like were using hash equality when constructing the lock file. And possibly not providing sufficient source replacement statements in the output from cargo vendor. So I'm not sure where that leaves us.

@P-E-Meunier
Copy link
Author

Note that I'm not questioning this choice. The main issue here is that cargo build works fine, but cargo vendor + cargo build --offline fails, which isn't expected.

@weihanglo
Copy link
Member

Same question in #14821 (comment). Could you give the full step of the repro? Sorry I had a hard timing reproducing it 😞.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

No branches or pull requests

3 participants