std: Fix IntoIter::as_mut_slice's signature

[alexcrichton]

This was intended to require &mut self, not &self, otherwise it's unsound!

Closes #39465

[alexcrichton]

Nominating for beta as well

[brson]

@rust-lang/libs Does this look like the right solution? Should we crater? The fallout seems likely to be low, and even if it is not we must make some change here.

[alexcrichton]

I'm ok running this through crater, but I don't think we should block on it. We'll want to land this change regardless of the results I'd imagine. In that sense we could just wait for normal beta/nightly reports to turn up regressions here and we can proactively send PRs to fix.

[notriddle]

We'll probably want this in 1.15.1, too.

[sfackler]

We don't publish point releases for every single codegen bug that is found/fixed. It's not clear to me that this is worth a point release either.

[steveklabnik]

How specifically is this permitted with our back-compat rules?

[Stebalien]

@sfackler this isn't just a codegen bug, it's a backwards comparability hazard.

[Thinkofname]

Isn't there another issue with the fact that IntoIter's ptr is *const T

rust/src/libcollections/vec.rs

Line 1915 in 1a2428f

ptr: *const T,

meaning this has a const to mut cast which is undefined?

[sfackler]

@steveklabnik

https://blog.rust-lang.org/2014/10/30/Stability.html

What are the stability caveats?

We reserve the right to fix compiler bugs, patch safety holes, and change type inference in ways that may occasionally require new type annotations. We do not expect any of these changes to cause headaches when upgrading Rust.

@Thinkofname it's undefined to go from a &T to a &mut T, not from a *const T.

[Manishearth]

const to mut casts in raw pointers are defined, IIRC. The const/mut separation is mostly a lint.

(Defined to the extent that most things are defined in Rust: actually-not-defined-because-nothing-is-but-it-isn't-dangerous-UB)

[Thinkofname]

Ah, my mistake. Thanks @sfackler @Manishearth

[steveklabnik]

@sfackler cool, just making sure that we have an exact justification here.

[aturon]

The thing crater tells us in a case like this is whether we can make the fix directly, or we have to deprecate instead. But I suspect that this function is seeing very little use.

cc @rust-lang/core on the question of making a point release. Given how obscure this function is, I doubt a point release will have much practical impact. But we may want to do it anyway, just on principle.

[alexcrichton]

I personally would not want to worry with a point release, it's enough effort and this is minor enough that I don't think it's worth it.

[withoutboats]

I would say this bug is pretty brazen, but simultaneously the impact is likely to be low. It seems like the question of the point release is mostly a question of what gesture the project wants to make in response, balanced against the amount of work required in performing a point release.

This seems like a bigger issue than what the previous point release was for.

[aturon]

@withoutboats

This seems like a bigger issue than what the previous point release was for.

For reference, see the bottom of https://blog.rust-lang.org/2016/10/20/Rust-1.12.1.html for notes on the last point release.

I'm not sure I agree that the current issue is a bigger deal. On the one hand, it's a soundness issue, rather than a compiler crash -- which is potentially worse, yes. On the other hand, it's pretty obscure.

In general, we have a handful of known soundness problems in the compiler, which have not been given top priority due to their being very niche and hard to weaponize. I think that's the right call. But the point is, it's not like this one API is the sole thing making the release unsound :-)

[est31]

I guess running it in crater now doesn't bring much. Except of nightly users (which do know how to adapt to change), this API was usable for only a very short time, and therefore the number of crates impacted would be very small.

[est31]

About point release, now is your chance to get the fixed version into Ubuntu 17.04. If I read the schedule correctly, Rust 1.15 will most likely get into Ubuntu Zesty (17.04), while Rust 1.16 (released on Mar 17) won't.

Of course, its only a small bug, not a big one. I'm neutral on whether to do a point release or not, just wanted to provide this thought.

[Manishearth]

I think Boats' point is that it's a bigger issue because of the message it sends. This is a more brazen kind of soundness vulnerability (most others are harder to trigger by accident, except perhaps for the sneaky repr(packed) one) -- it's literally a safe API that is basically always unsafe to use, and the only reason it wouldn't be triggered in the wild is because it's a new API. It's a more serious issue because we care about safety. Rust isn't "Rust: the language with a compiler that doesn't crash" (indeed, Rust has had a crashy compiler until relatively recently). Rust is "Rust: the safe language".

[withoutboats]

Yeah, my point was along the lines of @Manishearth's thoughts. This isn't a corner case in the compiler, this API violates a really core selling point about Rust's ability to encapsulate unsafe code in a safe API. I think we should be very committed to std having a safe interface.

That might not mean a point release is necessary, but its what's motivating my comment.

[aturon]

@Manishearth @withoutboats Thanks, that clarifies things nicely, and I can see the argument. I now lean toward a point release.

@brson, can you comment on the effort needed to make a point release?

[notriddle]

Can a rustc reviewer r+ this, so it at least gets into master, first?

[Manishearth]

@bors r+ p=100

[bors]

📌 Commit 80f7db6 has been approved by Manishearth

[bstrie]

I feel like a soundness bug making it through the stability process so casually is a bit concerning. A point release would help drive home our commitment to memory safety (while it's true that the compiler contains other soundness bugs, we shouldn't consider that to be no worse than adding new ones). It will also give us moral ground to stand on just in case this becomes a backcompat issue somehow.

I'd also like to see a minimal postmortem discussing how this made it past our stability process and how we're going to change the process to ensure that nothing like this happens in the future (and by minimal I do mean minimal, no need to go overboard with prostration, and it can be as simple as a post to irlo).

[bors]

⌛️ Testing commit 80f7db6 with merge 7f294e4...

[Manishearth]

In general I would like to do a more thorough audit of the unsafe blocks in the stdlib. I've done this in bits and pieces before (and it's been quite illuminating in helping me understand how unsafe code should be written). This audit can also spend time explaining why unsafe code is safe -- I see libs in the ecosystem do this, but the stdlib itself does not.

Besides that, we probably should require two reviews for any code touching unsafe. Servo's highfive even warns if there's unsafe in the diff, and we often have the code double checked. Given that unsafe code in Rust is a much bigger deal (Servo's in alpha, segfaulting our negative five users is no big deal) we might want a more formal process there.

(Of course, since Rust is the compiler, it's easy to cause unsafety by breaking codegen or whatever. I personally think that that is less problematic and is easier to catch in tests, whereas things like this require a user to write bad code that the compiler allows (which won't show up in the tests))

[shahn]

As a user and advocator of Rust I think this really deserves a point release. It's a bit of a mess-up that this got stabilized, it allows writing code now that will break next release, it is an unsafety when using an API as intended. I think Rust can show that it is willing to go the extra mile and effort here, even though other soundness bugs remain unaddressed for various reasons. This can turn into a positive thing if handled correctly.

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: Manishearth
Pushing 7f294e4 to master...

[bstrie]

@Manishearth given that for a few years now Mozilla has been sponsoring security audits of OSS codebases that it uses, I don't think it's beyond the realm of possibility that we could convince them to hire a third party to audit the unsafe blocks in the stdlib. But there's a lot of things that we should do on our end before we're ready for that, and in any case that's a different conversation best had elsewhere.

[brson]

@brson, can you comment on the effort needed to make a point release?

We could do it in 2-3 days. Backport, bump, build, test, publish. I can't think of anything likely to be problematic.

We might want to fix #39476 as well.

[Manishearth]

Sounds like an interesting idea. Usually they hire security experts, but in this case it probably makes more sense to hire a person familiar with unsafe rust.

But I agree, there's a lot we can do as a community first.

[gnzlbg]

The cost of fixing this are:

• creating a new point release.
• introducing a breaking change (even though it's allowed, since it fixes a soundness issue).
• those who have already updated will need to do so again.

The wins of fixing this are:

• less chance for programs/libraries to have UB,
• less chance for new code to rely on a signature that we are going to break in the future.
• we send a message that soundness issues in standard are taken seriously.

I think the wins outweight the costs.

@steveklabnik I'd like to see a constructive post-mortem of the issue.

I think that a constructive way to proceed from now on is to prepare the release notes for the beta releases, and announce those to the rust community so that everything in beta gets more attention during the 6 week period. Doing a new release announcement for stable should basically just then become "take the release announcement from beta and apply the changes that happened during the last 6 weeks".

[bluss]

I love the thought of putting out point releases to fix bugs. Rust really hasn't done it before for regular bugs, so the precedent says the next release will simply be the fix. This is absolutely a regular kind of bug. Browse the issue archives if you want to find serious, long time open soundness issues. If we change gears so that bug fix releases show up more often I'm very supportive of that, but I don't want to pretend this kind bug is something we've never had before.

[aturon]

@gnzlbg The idea of putting together the release notes for beta and announcing them internally is a really good one! Getting more attention on the beta would be really helpful -- we've had multiple regressions that should have been caught on beta, but weren't.

It seems clear that we should put out a point release for this fix; I also agree with @brson that we should fix #39476 as well. I'll touch base with the rest of the core team today to try to get this going.

Separately, I think it'd be good to have a broader discussion about how to think about soundness issues. @bluss is correct that there are some pretty serious open soundness bugs; there are also some longstanding but very obscure ones. We'll produce a post-mortem and also try to open a general discussion on the topic.

[Manishearth]

but I don't want to pretend this kind bug is something we've never had before.

No, this is one of the more blatant kind of soundness holes. It's a safe API that's almost always UB to use. That's ... as blatant as you can get. All the other soundness holes are pretty hard to hit in normal code, and often require the confluence of various features used in a particular way.

The only reason it's not that big a deal is that it's a new safe API that folks don't know about yet and aren't using it in their applications (unlike older safe APIs like BTreeMap which have been around for enough that it's plausible).

[kylone]

@bluss It's worth noting that this was caught immediately after going stable. I feel that a point release can only encourage scrutiny.

On a different note, how effective has the beta train been? Most of the discussion I've seen about Rust releases has been about nightly and stable. The hope was for the beta release would catch errors like these. Can we improve the communication around beta branches? I sometimes feel like the current mood around beta is a 6 week time-out, not a 6-week background check.

Would focusing auditing efforts on the beta branch (as opposed to pull requests) be worthwhile?

[aturon]

@kylone I definitely think more attention on the beta branch would be worthwhile.

It does get some testing today via projects that use CI to test against it. So at least we hear about obvious breakage. But @gnzlbg's idea of drawing attention to beta release notes within the community could be a good way to get more scrutiny.

[oyvindln]

Wouldn't it make sense to output some sort of warning a member of an immutably referenced struct is cast to *mut T like what happened here? Or am I missing something.

[alexcrichton]

I'm gonna go ahead and tag this as beta-accepted and send a PR to beta.

[sfackler]

I'm not really sure that usage of the beta branch would be the thing that would have found this - the beta branch is mostly useful to make sure code that used to work continues to, or use big new features like macros 1.1. This method is pretty obscure, and anyone that is actually calling it would probably work just fine if you used it in the "right" way.

I feel deeper looks into release notes would probably be a more targeted approach to find these kinds of issues, but I'm honestly more worried about another thread::scoped style issue where the problem is way more subtle.

@oyvindln those kinds of casts are super common in FFI code where C libraries tend to not be super precise with pointer constness.

[aturon]

@sfackler The suggestion from @gnzlbg was not so much about usage of beta, as much as getting attention on the release notes when beta is branched -- which at least gives people an entry point to kick the tires on various APIs.

[sfackler]

Ah gotcha, that makes sense.

[kylone]

@sfackler I was looking at this from a process point of view. There are a lot of community members using nightly and stable, but it's mostly automated tool that interact with beta. I've been thinking out this, and I'm wondering if there's opportunity to get developers that generally work on stable to take a look at beta.

Something like publishing what's new on beta, with a less abstract take on the change than the stable releases than the stable releases. Things that could be worthwhile to include are the new signatures added to std (or core) libraries, a file diff summary, and unsafe code added.

This is all in an effort to encourage the rust community to take a look at what's coming for the next stable release. I feel that there's so much going on with Rust that it's rather overwhelming for people who don't contribute RFCs or code, and that the beta branch can be a rallying point for people who would like to test.

[sfackler]

Yeah, that definitely makes sense. Maybe posting the release blog post that'll go up on the next release on internals.rust-lang.org when the beta is cut would be a good way to do this?