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.

Sign up for GitHub

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

1.24.1 announcement #230

Merged
merged 5 commits into from
Mar 2, 2018
Merged

1.24.1 announcement #230

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
427e767
1.24.1 announcement
steveklabnik Mar 1, 2018
7557bb8
spellfix
steveklabnik Mar 1, 2018
7e0d2fe
revisions
steveklabnik Mar 1, 2018
f8c907f
more fixes
steveklabnik Mar 1, 2018
f0f99b3
one more thing
steveklabnik Mar 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 40 additions & 29 deletions _posts/2018-03-01-Rust-1.24.1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,29 +23,28 @@ appropriate page on our website, and check out the [detailed release notes for

## What's in 1.24.1 stable

As you know, we tend not to release point releases. In this case, we had a few regressions in 1.24.0.
Each of them individually was not a "hair on fire" moment, but there's two reasons we decided to
produce a point release. First, collectively, they were big enough to consider as a whole. Second, we
want our attitude towards regressions to be "if we're not sure, default to reverting." Given that
it was not clear that we *shouldn't* release a point release, we decided to err on the side of
caution.
As you know, we tend not to release point releases. In this case, Several minor
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you know, we tend not to release point releases.

I'd still kill this -- what is it trying to get across, exactly?

"In this case, Several" capitalization

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you know me, writers are trying to make it pretty :p

ill kill it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicky of me, but "Several" should not be capitalized.

regressions were found in 1.24.0 which collectively merited a release.

A quick summary of the changes: there are four.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we may want to call out specifically that only two of these are actual regressions, and the other two were... well.. sort of "piled on" the release already happening.

Specifically the unwind abort stuff caused the lua regression, the linker files were a "regression" in the sense that debug builds stopped working in some cases on windows, and the index generator/cargo warnings aren't fixing regressions but rather just fixing builds.

I'd personally be ok not mentioning the error index stuff or cargo pieces at all, they're sort of "neutral motivation" for doing a point release anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I figured they're pretty small, and this stuff happened so rarely, that it's worth mentioning them. I'd be okay removing the long explanations, and just linking to the issue, maybe that's a decent compromise?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah I think the longer explanations are fine, but could it be explicitly called out here though that the first two are regressions that we're fixing and the latter two are "changes on beta we decided were important enough to accelerate to 1.24.1, but are not fixing regressions"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, the error index stuff actually is a regression, in that self-rebuilds worked fine on earlier releases. But it's not something I would have requested a point release for, as I can easily patch this at the distro level (and I already have).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to call the Windows 7 TLS issue a regression as well. It might not have been caused by an immediate code change in Rust, but it was caused by the choice to use Github for the index, and inadequate preparation/research on our side to ensure there would be no impact from the TLS change. From an affected users point of view, builds went from working to not working with no action on their side. That's a regression IMO

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is basically the struggle with explaining this. I think they're all regressions, for the reasons laid out here, but weren't serious enough to normally request a release. I tried to explain that above, but I think that @aturon 's suggestion of conciseness is important too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"there are four" seems unnecessary given the bullets :)


* Do not abort when unwinding through FFI
* Do not abort when unwinding through FFI (this reverts behavior from 1.24.0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"behavior added in 1.24.0"

* Emit UTF-16 files for linker arguments on Windows
* Make the error index generator work again
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can cut this that'd be great, it's really only relevant for distros AFAIK.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it felt really weird to me with so few changes to leave one out

* Cargo will warn on Windows 7 if an update is needed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These last two are less immediately obvious than the first two. Is it worth linking to the respective issues here, even though they're explained below?


In general, if you haven't run into any build issues, the "do not abort when unwinding through FFI"
change is the only one you should care about. The others are also important, but if 1.24.0 has been
working well for you, this is the only change that affects you. We plan on bringing this behavior back
in 1.25 or 1.26, depending on how smoothly the new strategy goes.
If your code is continuing to build, then the only issue that may affect you is
the unwinding issue. We plan on bringing this behavior back in 1.25 or 1.26,
depending on how smoothly the new strategy goes.

With that, let's dig into the details!

### Do not abort when unwinding through FFI

TL;DR: the behavior in 1.24.0 broke the `rlua` crate, and is being reverted. If
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new behavior.

I'd also add at the end of the paragraph that "While we still plan to introduce this behavior eventually, we will be rolling it out more slowly and with a new implementation strategy."

Copy link
Member Author

@steveklabnik steveklabnik Mar 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this now feels like almost an entire duplicate of the paragraph after the exposition; is that okay, or should we remove it? then that all flows weird...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think i have a fix

you have since changed your code to take advantage of the behavior in 1.24.0,
you'll need to revert it for now.

Quoting [the 1.24 annoucement](https://blog.rust-lang.org/2018/02/15/Rust-1.24.html):

> There’s one other change we’d like to talk about here: undefined behavior.
Expand All @@ -65,9 +64,11 @@ extern "C" fn panic_in_ffi() {
>
> In Rust 1.24, this code will now abort instead of producing undefined behavior.

We have reverted this behavior, and 1.24.1 will reintroduce the undefined behavior.
Fundamentally, this is a soundness fix, and we still plan on reintroducing the
abort, but are going to be rolling it out a bit more slowly, and with a different
As implemented, this functionality broke some users, notably, integration with
the Lua language. We have reverted this behavior, and 1.24.1 will reintroduce
the undefined behavior. Fundamentally, the aborting behavior we originally
rolled out is a soundness fix, and we still plan on reintroducing it, but are
going to be rolling it out a bit more slowly, and with a different
implementation.

It started with [a bug filed against the `rlua`
Expand All @@ -81,9 +82,9 @@ language](https://www.lua.org/).
> production Rust users, and so handling this was a very high priority for the
> Rust team.

In short, `rlua` error handling just... broke. On Windows, and only on Windows,
any attempt to handle errors from Lua would simply abort. This makes `rlua`
unusable, as any error of any kind within Lua causes your program to die.
On Windows, and only on Windows, any attempt to handle errors from Lua would
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not important, but this is MSVC-specific (since MinGW doesn't use SEH).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems important to me, since people use MinGW and might want to know!

simply abort. This makes `rlua` unusable, as any error of any kind within Lua
causes your program to die.

After digging in, the culpurit was found: `setjmp`/`longjmp`. These functions
are provided by the C standard library as a means of handling errors. You
Expand Down Expand Up @@ -121,7 +122,7 @@ When we talked about `setjmp`/`longjmp` inititally, a key phrase here wasn't
highlighted. Here it is:

> After digging in, the culpurit was found: `setjmp`/`longjmp`. These functions
> are *provided by the C standard library* as a means of handling errors. You
> are *provided by the C standard library* as a means of handling errors.

These functions aren't part of the C language, but part of the standard
library. That means that platform authors implement these functions, and
Expand Down Expand Up @@ -156,9 +157,10 @@ Consider the code above, and imagine that `lua_newtable` here could call
3. The initial `lua_pcall` catches the `longjmp` with the `setjmp` it called earlier.
4. Everyone is happy.

However, `lua_newtable` is an `extern fn` to Rust. So, with the chances in
1.24.1, it sets up a panic handler that will cause an abort. In other words,
the code sorta looks something like this psuedo code now:
However, the implementation of `protect_lua_call` converts our closure to an
`extern fn`, since that's what Lua needs. So, with the changes in 1.24.0, it
sets up a panic handler that will cause an abort. In other words, the code
sorta looks something like this pseudo code now:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Sorta" is unnecessary here. (Side note: what's our desired level of formality for release notes? "Sorta" is sorta informal.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our voice is not totally formal. rust is a friendly language :)


```rust
protect_lua_call(self.state, 0, 1, |state| {
Expand All @@ -179,14 +181,15 @@ everything works. However, on Windows, since both `setjmp`/`longjmp` and Rust
panics use SEH, the `longjmp` gets "caught", and runs the new abort code!

The [solution here](https://github.com/rust-lang/rust/pull/48572) is to
generate the abort handler, but in a way that `longjmp` won't trigger. Eventually,
it's likely we'll have to register our own personality functions and such, but
this is good enough for now. It's not 100% clear if this will make it into Rust 1.25;
if the landing is smooth, we may backport, otherwise, this functionality will
be back in 1.26.
generate the abort handler, but in a way that `longjmp` won't trigger it. It's
not 100% clear if this will make it into Rust 1.25; if the landing is smooth,
we may backport, otherwise, this functionality will be back in 1.26.

### Emit UTF-16 files for linker arguments on Windows

TL;DR: `rustc` stopped working for some Windows users. If it's been working for you,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"for some Windows users" maybe add "in edge-case conditions".

you were not affected by this bug.

In constrast with the previous bug, which is very complex and tough to understand,
this bug's impact is simple: if you have non-ASCII paths in the directory where
you invoke `rustc`, in 1.24, it would incorrectly error with a message like
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/it would/it may/

perhaps?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it happened sometimes but not always, it should be "it could", not "it may".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my understanding is that this bug happens 100% of the time when the conditions were met. If that's not true, I'd go with "could", but I don't care a ton!

Expand Down Expand Up @@ -214,13 +217,20 @@ straightforward: produce a UTF-16 file with a BOM.

### Make the error index generator work again

TL;DR: building Rust 1.24.0 with Rust 1.24.0 broke in some circumstances.
If you weren't building Rust yourself, you were not affected by this bug.

When packaging Rust for various Linux distros, it was found that [building
1.24 with 1.24 fails](https://github.com/rust-lang/rust/issues/48308).
This was caused by an incorrect path, causing certain metadata to not
be generated properly.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This paragraph doesn't include the phrase "error index" at all and therefore (in my opinion) doesn't shed much light on what "Make the error index generator work" means. Could someone (who actually knows) provide a bit more detail?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the least interesting of these bugs, and the post is already long. Since it only affects a very small number of people, I decided to skimp on explaining it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@steveklabnik Maybe "certain metadata" could be replaced with "the index of (...???...) errors"? (I'm not exactly sure what error_index_generator is or does.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor of removing this from the blog post entirely -- it's tiny, and AFAIK only affects distros.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, let's replace "Make the error index generator work again" with "Fix Rust's ability to build itself on certain Linux distros". I agree with @steveklabnik that there's not much of a good reason to leave out one of only four changes, but linking to the issue is sufficient for anyone interested to see what broke and how it was fixed.


### Cargo will warn on Windows 7 if an update is needed.

TL;DR: Cargo couldn't fetch the index from crates.io if you were using an older
Windows without having applied security fixes. If you are using a newer
Windows, or a patched Windows, you are not affected by this bug.

In February of 2017, [GitHub announced that they were dropping support for
weak cryptographic
standards](https://githubengineering.com/crypto-deprecation-notice/). One
Expand Down Expand Up @@ -248,9 +258,10 @@ that accessing GitHub would start to fail.
`libgit2` [created a fix](https://github.com/libgit2/libgit2/pull/4550), using the `WinHTTP` API
to request TLS 1.2. On master, we've [updated to fix this](https://github.com/rust-lang/cargo/pull/5091),
but for 1.24.1 stable, we're [issuing a warning](https://github.com/rust-lang/cargo/pull/5069),
suggesting that they upgrade their Windows version. There's a balance here: we could have backported
the `libgit2` fixes, but that'd require updating a lot of code, which feels incorrect for a point
release, especially since the issue does not affect patched systems.
suggesting that they upgrade their Windows version. Although the `libgit2` fix
could have been backported, we felt that the code change footprint was too
large for the point release, especially since the issue does not affect patched
systems.

## Contributors to 1.24.1

Expand Down