Is it really a good idea for Cargo to silently choose out-of-date versions of crates to avoid pulling in multiple compatible versions of another crate?

I ran cargo audit in a project of mine today to discover that my Cargo.lock included version 0.22.0 of the nix crate, which has a memory corruption vulnerability (RUSTSEC-2021-0119). This was strange, because a fix had been published in nix version 0.22.2 for almost a month; I'd run cargo update recently; and rustyline 9.0.0, the dependency that was pulling nix in, specified it as nix = "0.22", not nix = "=0.22.0".

It took me a while to figure out what was going on. An empty crate with only rustyline = "9.0" in its dependencies got nix 0.22.2, but I eventually found that if bitflags >= 1.3.0 ended up somewhere in the dependency graph, nix got downgraded to 0.22.0.

nix 0.22.0 had a dependency bitflags = "1.1", and bitflags 1.3.0 broke compatibility with certain old versions of rustc that nix aimed to support, so the author added a "< 1.3.0" version bound to bitflags in nix 0.22.1. Cargo therefore seems to see that if bitflags 1.3.x and nix 0.22.x are both needed, the only way to avoid pulling in a version of bitflags in the 1.1 to 1.2 range alongside one in the bitflags 1.3 range is to use the broken nix 0.22.0. It's clever, but it leads to a vulnerable version of nix being silently chosen.

I don't necessarily have solutions, and I'm not affected by the vulnerability, I just thought it might be wise to bring this case to the attention of the people who work on Cargo. A warning would be nice, and I wouldn't consider an error unreasonable here if the "no multiple versions of a crate within the same compatibility range" rule is non-negotiable. Cargo automatically downgrading crates to potentially buggy versions due to distant interactions with other crates just doesn't seem right to me.

15 Likes

I agree with this. I would prefer to know when vulnerable or potentially buggy code is being inserted into my dependency graph because some version specification forced Cargo to do so. I'd even go so far as to make a Cargo.toml option that tells Cargo that if it encounters a situation like that it errors out and refuses to proceed until its resolved.

6 Likes

Can't you nix = "0.22.1"?

Since you're depending on the bug fix in 0.22.1...

2 Likes

The bugfix is in 0.22.2. And depending on it explicitly just causes an error, for the same reason Cargo passes over 0.22.1 and 0.22.2 normally: its version requirements for bitflags conflict with bitflags = "1.3".

Here's the error:

    Updating crates.io index
error: failed to select a version for `bitflags`.
    ... required by package `nix v0.22.2`
    ... which satisfies dependency `nix = "^0.22.2"` of package `xyz v0.1.0 (/tmp/xyz)`
versions that meet the requirements `>=1.1.0, <1.3.0` are: 1.2.1, 1.2.0, 1.1.0

all possible versions conflict with previously selected packages.

  previously selected package `bitflags v1.3.1`
    ... which satisfies dependency `bitflags = "^1.3"` of package `xyz v0.1.0 (/tmp/xyz)`

failed to select a version for `bitflags` which could resolve this conflict
1 Like

I’d say there are two different root problems here.

First, cargo didn’t have first-class Minimal Supported Rust Version support. It has some of it now, so hopefully in the future Cargo would just handle all MSRV best practices automatically.

Second, cargo doesn’t warn on publish for non-default dependency specifications. Basically, if you publish a crate with = or < dependencies, you are adding very subtle problems downstream, but nothing signals about that. The problem here is that there are subtle cases where you do want at least =, and it’s hard to automatically figure out if unusual dependency specification is used correctly.

In this specific case, nix library shouldn’t be adding < constraint to pin bitflags to older MSRV. Instead, the response to the issue should be “pin bitflags to older version in your end application lockfile with cargo update -p bitflags —precise”. That way, people who use latest rustc could use latest bitflags and latest nix, and people who need older rustc can use it with latest nix and older bitflags. It also correctly puts maintainance burden to the party who needs old rust version.

19 Likes

Tangential, but I just realized I can now formulate the core of MSRV debate:

MSRV of X for library means that there exists a set of packages which satisfies constrains in Cargo.toml, and which can be compiled with X version of Rust.

MSRV of X doesn’t mean that for all sets of packages that satisfies constraints, that set can be compiled with X.

“Exists” formulation is more useful, as it gives more choices to the end application with a lockfile.

5 Likes

nix's maintainer seems to take a different view of who's responsible for ensuring compatibility with old Rust versions and is solving the problem by vendoring bitflags into nix instead of depending on it. Regardless, this sort of situation could happen in any case where the maintainers of two crates with a dependency relation disagree on what changes require a major version bump.

3 Likes

When cargo starts using the pubgrub dependency resolver, we should be able to give better explanations for why dependency resolution selects a specific version.

8 Likes

Oh no. Indeed I agree with the view that msrv should mean that there exists a set of dependencies. However, they have no power to help their users find that set except by upper bounding dependencies. So it's quite understandable that this occurs in practise. I believe we must either ship msrv-aware resolver fast or provide a separate tool to create such a lockfiles and popularize that quickly. Such a tool would also be quite useful for testing in CI pipelines of crates with lower msrv.

2 Likes

FWIW, cargo +nightly generate-lockfile -Zminimal-versions gives you that set, provided that you (and your deps) specified minimal versions correctly.

In general, while we don't have super-nice tooling in place here, it certainly isn't an insurmountable task to find lockfile that works for your Rust version -- you can iteratively downgrade dependencies which fail to compile using cargo update --package dep-name --precise --vers major.minor.patch until you find the set that works.

Similarly, for testing MSRV on CI you want to have Cargo.lock.msrv committed to the repo and substitute that for Cargo.lock during MSRV CI (example).

8 Likes

For deeper dependency trees I highly doubt this approach works efficiently enough. Each compile will take a considerable time itself before even hitting an issue. Consider that your failure might be due to a dependency-of-a-dependency, so downgrading must begin with the leafs but then you can't differentiate between the different kinds of failures. But starting from below you must avoid all 'naturally' broken versions. It's really not an easy task at scale. I think this can easily require some hundreds of compile cycles in pathological cases.

Maybe with rust-version it will get us a lot closer but a) it's not enforced so the info can just be wrong and then you run into the same issue b) it will only work with some future version c) it must be present in all dependencies in the whole tree to be effective.

cargo +nightly generate-lockfile -Zminimal-versions is as bad as upper bounds, or worse. You don't get feasible security updates. Of course it's maybe a good starting point for an iteration from below but it's not a replacement for actual version selection based on msrv. But no sane crate author today publishes updates that only bump the minimal compatible version so this will give a strictly worse set of dependencies than an upper bound. And it's still not msrv aware.

3 Likes

This sounds like a good formulation. To extend this, I think we can say that there is currently no mechanism to formally describe this set of packages. This is where some problems arise, because sometimes people try to use Cargo.toml for this.

To be quite honest: if the bug fix or security update impacts use of their crate, they should. Because it's then a bugfix/security update for their crate as well.

The problem with this is social; because there's no easy way (nightly -Z flag isn't easy) to check your minimal versions specification is correct, it's uncommon for authors to do so. When a dependency isn't minimal versions correct, it makes it quite difficult to actually check your own minimal version correctness.

I run into issues with this surprisingly often! I have an old lockfile, then add either a new dependency or upgrade one of my dependencies, and stuff breaks because I'm using old dependency-of-dependency versions. (I've had this happen with a cargo upgrade!) So far I've only had it cause compilation issues, but if unlucky, it could've caused buggy behavior or security issues.

I'm of half a mind to suggest that the cargo publish preflight checks should ignore your lockfile and test minimal versions instead, to encourage people to be more minimal versions correct. But because there are major dependencies out there that see minimal version correctness as a non-issue (there's no practical way to run into it with stable tooling, supposedly), actually trying to enforce minimal versions correctness is difficult. (And IIRC this is part of what's blocking stabilization; how useless the resolver is when major dependencies break under the flag.)

3 Likes

I think we might have to disagree here but as a maintainer I find that just silly. For any largeish crate they would pretty much do nothing but release patch versions bumping dependencies. And do you want any crate, no matter how complete, to release further bumps and bumps? Firstly that's not going to happen in practice and secondly having this expectation would be bind huge numbers of maintainer hours. The job of the Cargo.toml is to define compatibilities, the job of the resolver to find the best compatible version. Not the other way around.

2 Likes

Note that a majority of upstream fixes aren't going to impact your use to mean you should force a version bump, though! Most fixes are to edge case behavior; if you don't exercise that edge case, it's not important to your downstream which version of your upstream they're using.

Does the dependency update fix a user-facing bug? Then it's a meaningful update to publish to require the version which fixes the bug. A trivial and automatable update, but a meaningful one.

It doesn't necessarily have to be proactive updates (users get that from cargo update, obviously), but what I'm asking boils down to

  • your test suite should pass with minimal version resolution, i.e. your Cargo.toml specifies sufficient dependency versions for your library to work (at least, assuming your upstream's latest is also minimal-versions correct; patching upstream to fix that is not possible), and
  • any issue/bug report/etc fixed by a cargo update should be fixed by upgrading your dependency to always fix the issue, not just by telling the user to cargo update. (Ideally, a test is added, and the dependency update falls out of the above.)

A maintainer can be more proactive than that, and watch their direct dependencies for updates that might impact use of their library.

dep = "X.Y.Z" in your Cargo.toml means you work with version X.Y.Z of dep (and any semver compatible version). All I'm asking is for that promise to actually be true. If your library has a bug when used with the dependency version(s) it says it works with, I consider that a bug worth a bugfix release.

Obviously the best experience will be with the latest improvements in upstream. But you should work with the versions you claim to work with.

4 Likes

I believe you are missing my point and I had misunderstood yours. I tried to understand why crate authors may choose to add upper bounds (i.e. what process failure lead to OPs initial problem). Let me recap my understanding of the discussion:

  • matclad suggested that generate-lockfile -Z minimal-versions could be used to deliver a set of msrv compatible dependencies 'if you specified minimal versions correctly'.
  • I tried to convey that this generates a very different set of dependencies from cargo update, and it's not the usual workflow for users. I, now, acknowledge it could hopefully be useful for testing the existance of a set (i.e. msrv compliance). But it wasn't related to the point I was trying to make: you can not rely on this as an eventual downstream users and packager because it does not provide the same deep update semantics that cargo's version resolution usually gives. And that it's indeed not easy or trivial to recreate those semantics with msrv constraints through an iterative process.
  • You remarked that you could increase the version numbers of dependencies.
  • And I think here we missed our points. You probably said this in reference to -Z minimal-versions but I did not understand it that way. (Indeed, the library author should fix by publishing a new version). Because I had seen it from the PoV of the workflow of downstream users, it seemed to me like you were arguing that authors should bump their version specs to the most recent stable version downstream so as to help -Z minimal-versions do the equivalent of selecting an as-up-to-date-as-possible dependency set.

So, alas, I hope we can agree on two counts:

  • We'd like more widespread use of correct minimal depency specification so that there is useful tooling to test the msrv-specification (and this tooling can then probably be provided by -Z minimal-versions).
  • But the problem of selecting up-to-date dependencies under the constraint of msrv is separate. And cargo update is the usual workflow for packagers and other downstream users (even implicitly, as you've observed). And this 'does the wrong thing' _except if there are upper bounds on dependencies; and since this interferes in the usual work flow of developers, it's tempting to introduce upper bounds; and if you don't know the nightly flag indeed even more so.
3 Likes

Yep. Having done a number of "minvers" PRs to projects (with a lot of hit-or-miss merging nevermind actual deployment), the issues usually arise around the "core" libraries. Small features creeping in from newer libc that isn't properly declared, serde fixes or such, etc. I have a minvers CI pipeline for GitLab on my projects (which are already minvers-supported), but I haven't done one for GitHub Actions yet. Maybe if we could get a template workflow file put together that folks could use to actually test their minvers, there'd be more traction?

First, cargo didn’t have first-class Minimal Supported Rust Version support. It has some of it now, so hopefully in the future Cargo would just handle all MSRV best practices automatically.

FWIW, post-rust-version discussion on incorporating MSRV information in the dependency resolver:

Maybe publishing a library crate with < or = dependencies should generate a warning from Cargo?

2 Likes

See Lint againt non semver-open dependencies · Issue #5340 · rust-lang/cargo · GitHub. Seems like a good idea at first, but it's hard to fully get rid of false errors.

I think authors who care about using latest versions should specify so, instead of relying on Cargo implicitly upgrading things. cargo-edit has cargo upgrade command that does it for all deps. I highly recommend it.

-Zminimal-versions is unusable, because crate authors don't see a problem with specifying foo = "1" as their dependency, rely on cargo picking the latest, and assume that version flexibility is a good thing. This breakage affects many crates, including uses of cc and serde. I've had push-back from crate authors against yanking old broken versions and using semver-minor versions correctly, so I think crate authors collectively have decided that minimal-versions is going to die.

Rust versions older than 3-6 months are problematic, 1-year old rustc is a nightmare. This needs a real fix in Cargo, otherwise crate authors who try to support old Rust are IMHO just wasting their time. It takes only one incompatible crate in the entire crate graph to break compilation, so it's not enough that some authors care. Few crates push MSRV up for everyone.

Registry source replacement feature is viable for simulating MSRV-dependent version resolution in Cargo. LTS — Rust/Cargo add-on // Lib.rs

4 Likes