rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
97.5k stars 12.61k forks source link

Backport (security) fixes and release 1.26.3 #52320

Closed dekellum closed 6 years ago

dekellum commented 6 years ago

Before flat out denying this request, please give people time to provide situational reasons for or express pros/cons. Thank you!

dekellum commented 6 years ago

Background

From the Announcing Rust 1.27 blog post:

Additionally, we would like to draw attention to something: just before the release of 1.27.0, we found a bug in the ‘default match bindings’ feature introduced in 1.26.0 that can possibly introduce unsoundness. Since it was discovered very late in the release process, and has been present since 1.26.0, we decided to stick to our release train model. We expect to put out a 1.27.1 with that fix applied soon, and if there’s demand, possibly a 1.26.3 as well. More information about the specifics here will come in that release announcement.

(emphasis mine)

The Announcing Rust 1.27.1 blog post doesn't mention 1.26.3

A users forum: Rust 1.27.1 is out! thread linked to that post, has a response from @Mark-Simulacrum which sounds a bit less supportive of a 1.26.3 release.

pietroalbini commented 6 years ago

Why do you need a 1.26.3? Not trying to be dismissive, but if we do that there should be a pretty strong argument for it, especially because 1.27 has been released for some time and we're approaching 1.28.

dekellum commented 6 years ago

Motivation

For some published crates, I have an update policy of supporting the last 3 minor (M.m) stable releases. This policy is not unlike other prominent projects see tokio-rs/tokio#465 and its policy which is similarly expressed in CI, as an established standard, for lack of cargo support. As the discussion of tokio-rs/tokio#465 suggests, these policies must be stable for dependency crates, as they can have long reaching effects. For example, for the body-image crate, I updated the minimum rust support to 1.26.x for my 0.3.0 release which now uses impl Trait. It has been argued that if I was to increase the rust dependency again (say to 1.27.1) that I would need to publish it as a 1.x release. See rust-lang-nursery/api-guidelines#123 , this comment and also this. See also RFC 1619, this comment.

Your well intended stewardship and guidance has lead to this rather significant meaning of "1.26" that can not be adequately subsumed by and patched via "1.27.1". We need a "1.26.3" because your guidance and the ecosystem actors have made that a meaningful distinction.

Its also a practical and valuable distinction if you step back a little and consider the meaning and spirit of semantic versioning. There is a real and practical reason we reserve the patch release number for bug fixes. There is therefore a reason for a rust version dependency that allows for few M.N releases prior: stability! In fact it was made clear that the 1.27.1 release fixes issues arising from new features added in 1.26. There is similarly a chance for regressions to be induced by new 1.27 features like SIMD.

From the detailed release notes of 1.27.1, here are the things I believe should be backported to a 1.26.3 patch fix release:

cuviper commented 6 years ago

My understanding is that match-ergo code which compiles successfully with the newer rustc should also be fine with the 1.26.x releases. In other words, the older ones would allow things that they shouldn't, but truly correct code should be fine either way.

Mark-Simulacrum commented 6 years ago

rustdoc would execute plugins in the /tmp/rustdoc/plugins directory when running ....

This change presumably does not affect library authors in that if you support 1.27.x then downstream users can utilize this patch. Rust does not currently support anything except for the last stable release for security and other similar patches; distros are of course welcome to backport as much as they want.

The borrow checker was fixed to avoid an additional potential unsoundness when using match ergonomics ...

This change (like @cuviper says) only stops incorrect code from functioning; if your library compiles on latest stable and 1.26.2 then it will do the right thing on 1.26.2 as well; the borrow checker does not affect the code generated.

So, I think that any given library which wants to support 1.26.x can continue to do so without an additional 1.26.3 release with backports from 1.27; we have not changed functionality in such a way that a correct library (i.e., a compiling one on latest stable) will function correctly or only on one of these releases.

Do you have arguments against that position?

dekellum commented 6 years ago

I think I understand your position, but it considerably weakens the minimum rust version promise that I'm attempting to offer users, and their expectations, if they can't find a patched version of 1.26.x without security or soundness problems? Makes me look the fool that should have stayed on 1.25 and avoided using 'impl Trait'. To recast this as a fairness argument: it seems you've guided the ecosystem toward stringent minimum rust vesrion policies while also not being willing to backport. Is it really super difficult to produce a 1.26.3 with these fixes in place? If so, what's the root cause of that difficulty? Can I help (beyond shutting up here)?

Ixrec commented 6 years ago

I thought every version of Rust since 1.0 has had some known soundness bugs. For example, https://github.com/rust-lang/rust/issues/10184 and https://github.com/rust-lang/rust/issues/18510 and https://github.com/rust-lang/rust/issues/27282 have been known for years (though now all of them have at least one failed or upcoming attempt to fix).

dekellum commented 6 years ago

Yet another way of saying it: Your position effectively is an End-Of-Life for rust 1.26.x. I wouldn't have bumped my minimum version to 1.26.x if I knew that EOL was coming so soon. I would have instead waited to see if 1.27 or 1.28 were effectively stable for practical use.

cuviper commented 6 years ago

Rust doesn't do LTS (yet). Every release except the current stable is EOL, if you want to think of it that way.

dekellum commented 6 years ago

"LTS" is a straw-man conflation of what I've asked for here. Industry use of Long Term would imply at least a 1 year of Support, but 1.27.1 is the latest stable, and 1.26.3 is only one minor release prior, with 1.26.0 released May 10, 2018 (2 months ago).

If you on one hand think it reasonable that ecosystem crate libs have minimum rust version policies like 2 or 3 stable releases from the latest stable, then you shouldn't, for consistency and practical application, be unwilling to patch that many stable releases back.

Applying your position on 1.26.3 as precedent, logically also effects the practical direction of rust-lang-nursery/api-guidelines#123 and any future rebirth of rust-lang/rfcs#1619.

Close this if you prefer.

Mark-Simulacrum commented 6 years ago

I think what (my, at least) confusion with your request is rooted in is that I don't understand how a library supporting compilation on 1.26.x is affected by future patches to the compiler. While there may be soundness bugs in a 1.26.x compiler, or security flaws, that's the users decision. The library may want to test itself on nightly to make sure it does not contain code that accidentally exploited any unsound behavior, but that does not affect the minimum version of Rust it supports.

With that in mind, can you explain why these patches would be necessary?

dekellum commented 6 years ago

In practical terms I think a known security issue (rustdoc is part of compiler or part of an effective rust runtime?) and soundness compiler bugs have a chilling effect on users when they are making decisions on when and what to upgrade to (in development, CI, and production), and if a library's minimum rust is acceptable.

You may not think this request is reasonable now, but like it or not, you are going to receive more requests like this when something like rust-lang/rfcs#2495 is implemented.

Mark-Simulacrum commented 6 years ago

rustdoc ... part of an effective rust runtime

No, rustdoc is not shipped as part of your library, or the users application. The user of the library (i.e., the person compiling the final binary) may run it, but your library does not need to protect them by bumping its own minimally supported version. Your library works on 1.26.2. It is correct, and completely so, on that compiler version.

I agree that soundness bugs and security flaws in the compiler are non-ideal, and may influence the packagers/final user's decision as to what version of the compiler to use. But, importantly for this decision, they should not affect library author's decision of what minimum version of Rust to support. Why would they? Your library compiles fine on 1.26.2, and issuing a 1.26.3 does not help your library.

It does potentially help production users, but that doesn't seem to be the argument you are making. Historically we've not supported (patches or otherwise) any version of Rust except for the latest stable, beta, and nightly channel releases. Users are expected to be able to seamlessly upgrade. We acknowledge that some users cannot (at least in production), but again, the bugs fixed in the 1.27.1 release don't make code compile that didn't before. If you've tested some piece of Rust code as compiling on the latest stable/nightly, then it shouldn't compile incorrectly on a previous version (at least with the current set of bugs in 1.27.1).

dekellum commented 6 years ago

I agree with your suggestion that I don't need to change the minimum version that is currently established by CI only, in my lib. However in practice, this means prospective users will need to decide if an upgrade to 1.27.1 is acceptable, and my stated project policy (at least 2, ideally 3, stable versions) is not really being fulfilled without a 1.26.3 release being cut. I don't like the position I'm in.

Mark-Simulacrum commented 6 years ago

Your project supports 1.26.2, though? That is, I'm not sure why the project in question needs to change anything or is in violation of any of its policies.

dekellum commented 6 years ago

We are getting circular here, but to recap: Yes, my CI for the current release and current master says 1.26.2 as MSRV. In practice: My prospective users ~must~ will either upgrade to 1.27.1 (which is at odds with my stated policy) OR not use the current or future versions of my lib.

vi commented 6 years ago

Also 1.26.2 seems to be currently packaged rustc verison in Debian Sid.

Debian tends to tends to avoid frequent big updates, but accepts frequent-ish security fixes (i.e. minor updates).

I expect at least rustdoc and /tmp issue be patched during the packaging anyway (regardless of version number), but having 1.26.3 would probably decrease size of the Debian-specific patch.

pietroalbini commented 6 years ago

My prospective users must either upgrade to 1.27.1 (which is at odds with my stated policy) OR not use the current or future versions of my lib.

Your prospective users might also be using a rustc 1.26.2 packaged by a downstream project (like Debian) with the security patch applied.

I expect at least rustdoc and /tmp issue be patched during the packaging anyway (regardless of version number), but having 1.26.3 would probably decrease size of the Debian-specific patch.

Well, Debian will need to manually apply the patch anyway, at least for Debian stable (I'm pretty sure we're not going to also release 1.14.1).

dekellum commented 6 years ago

Possibly, while other prospective users want to rustup install 1.26.3.

fintelia commented 6 years ago

It makes sense for library authors not to immediately expect their users to upgrade to new versions of cargo/rustc as soon as they come out. At the same time, this takes additional resources (i.e. doing things the old way, even if it is more complex). There currently seems to be a lack of guidance on how to balance these two factors.

If I understand correctly, @dekellum seems to be advocating for this guidance in the form of a defined number of previous rustc versions which are guaranteed to get backports. To me personally, this seems like a reasonable request: if the core Rust developers aren't supporting a rustc version, why should library authors?

But I think we disagree on what warrants a backport. Rustc is probably always going to have bugs (soundness or otherwise) which cause it to generate binaries that don't act how the author intended. Each version will hopefully have fewer, but rustc is a complex piece of software and bugs like these are inevitable. If users want to avoid them as much as possible, they should use be sure to always track the latest toolchain versions. At the same time, I think that security issues triggered when simply running rustc and associated tools should get backported fixes. Not risking your computer getting compromised by known issues is a pretty reasonable expectation of supported software.

cuviper commented 6 years ago

I brought up LTS because RFC 2483 is proposing such additional channels that would be eligible for longer-term backports like this. The proposed term is still relatively short, but it would be a start.

dekellum commented 6 years ago

OK fair enough. FWIW, at the time I decided to call 1.26 my minimum supported rust, that RFC didn't exist. Obviously choosing an LTS, if one exists in the future, would be much wiser to avoid my particular predicament.

Edit, also note: that RFC has been postponed (nice way of saying rejected).

Ixrec commented 6 years ago

I suspect we should try to pull this discussion into this internals thread since some of the comments there are highly relevant and provide additional context.

In particular, from @withoutboats's post:

I haven’t seen a soundness hole demonstrated anywhere related to [the] ICE [which was fixed in 1.27.1]. We’re now releasing patch releases with backports of potential soundness fixes.

What we’ve seen is a huge shift in our capacity to make point releases, rather than a shift in the necessity of making point releases.

This and the rest of the discussion here leads to a lot of interesting questions. If a patch release is for only a possible soundness fix, does that affect whether it should be backported? Would it be more worthy of backporting if someone had managed to cause an actual soundness bug with it? For any particular library, what makes the 1.27.1 fix a bigger concern than all of the known soundness bugs that have no fix at all yet (e.g., the ones NLL is expected to fix)? What about all the soundness bugs that have been fixed only in a normal minor release, not as a patch release on any preexisting minor version; are those less serious?

My only opinions so far are that there's no objectively correct answer to any of those questions, the community hasn't agreed on any guidelines yet, this is deeply interrelated with the unresolved questions about what a "Rust LTS" might mean or how to solve the MSRV (minimum supported Rust version) problem, and the 1.27.1 doesn't seem like a particularly exceptional or catastrophic issue. In fact, the most exceptional thing about it seems to be the fact that it got a patch release (though I wouldn't mind if this standard for patch-worthiness became the new normal).


Edit, also note: that RFC has been postponed (nice way of saying rejected).

Postponed and closed are two very different statuses. Postponed RFCs actually will get revisited at some point, and some already have been. Many postponed RFCs have been postponed with comments that amount to "we're almost certainly going to do this at some point, but there's just too much else going on right now". No need to assume bad faith here.

dekellum commented 6 years ago

On the postponed vs rejected point, OK, fair enough.

Mark-Simulacrum commented 6 years ago

Closing as we're not going to do this (per the reasons stated above, for the most part).