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

Backport (security) fixes and release 1.26.3 #52320

Closed
dekellum opened this issue Jul 12, 2018 · 25 comments
Closed

Backport (security) fixes and release 1.26.3 #52320

dekellum opened this issue Jul 12, 2018 · 25 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-core Relevant to the core team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue.

Comments

@dekellum
Copy link

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

@dekellum
Copy link
Author

dekellum commented Jul 12, 2018

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.

@kennytm kennytm added T-core Relevant to the core team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Jul 12, 2018
@pietroalbini
Copy link
Member

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.

@pietroalbini pietroalbini added the T-release Relevant to the release subteam, which will review and decide on the PR/issue. label Jul 12, 2018
@dekellum
Copy link
Author

dekellum commented Jul 12, 2018

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/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:

  • rustdoc would execute plugins in the /tmp/rustdoc/plugins directory when running, which enabled executing code as some other user on a given machine. This release fixes that vulnerability; you can read more about this on the blog. Thank you to Red Hat for responsibly disclosing this vulnerability to us.

  • The borrow checker was fixed to avoid an additional potential unsoundness when using match ergonomics: #51415, #49534.

@cuviper
Copy link
Member

cuviper commented Jul 12, 2018

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

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

dekellum commented Jul 13, 2018

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

Ixrec commented Jul 13, 2018

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

@dekellum
Copy link
Author

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

cuviper commented Jul 13, 2018

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

@dekellum
Copy link
Author

dekellum commented Jul 13, 2018

"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/api-guidelines#123 and any future rebirth of rust-lang/rfcs#1619.

Close this if you prefer.

@Mark-Simulacrum
Copy link
Member

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

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

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

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

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

dekellum commented Jul 13, 2018

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

vi commented Jul 13, 2018

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

pietroalbini commented Jul 13, 2018

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

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

@fintelia
Copy link
Contributor

fintelia commented Jul 13, 2018

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

cuviper commented Jul 13, 2018

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

dekellum commented Jul 13, 2018

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

Ixrec commented Jul 13, 2018

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

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

@Mark-Simulacrum
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-core Relevant to the core team, which will review and decide on the PR/issue. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants