Stream: wg-ffi-unwind

Topic: "fixing the soundness bug"


Kyle Strand (Nov 14 2019 at 23:45, on Zulip):

The triage group asked for an update on a PR intended to fix the soundness hole that enables panics to propagate across a nounwind function boundary without the use of unsafe. This PR is one of many related to nounwind safety with extern functions.

I don't believe it is within this group's purview to make recommendations to the language team before submitting an RFC, but the topic is very closely related to our efforts, so I have left a comment there paraphrasing the three "immediately ready" fixes for the soundness issue and explaining that the project group has no timeline for delivering an RFC that will resolve the concerns that have kept the "immediate" fixes from being adopted.

https://github.com/rust-lang/rust/pull/64315#issuecomment-554135743

Kyle Strand (Nov 14 2019 at 23:46, on Zulip):

@nikomatsakis maybe I should have discussed that with you first... I forgot about it during our chat earlier. Sorry!

gnzlbg (Nov 15 2019 at 13:11, on Zulip):

Thanks for the summary, I think it properly covers all options I'm at least aware of @Kyle Strand .

gnzlbg (Nov 15 2019 at 13:13, on Zulip):

FWIW I don't think my solution is great, it just the most minimal way of fixing the soundness bug I was able to come up with. Having unsafe affect codegen is weird at best, and many argue that it is a bad idea.

Kyle Strand (Nov 15 2019 at 13:54, on Zulip):

@gnzlbg Thanks! I think it's clear from the thread that many people think it's a bad idea, but if you no longer think it's a good idea, I would almost suggest closing the PR. (In fact, RalfJ's PR is already closed, but I still listed it as one of the possibilities for an "immediate fix".)

gnzlbg (Nov 15 2019 at 14:20, on Zulip):

I think its bad, but its the best alternative I've heard.

gnzlbg (Nov 15 2019 at 14:21, on Zulip):

AFAICT the alternatives are "not closing the soundness bug", or waiting for somebody to properly analyze the impact / implications of ralf's PR, which seem less controversial, but haven't made progress either (which is also bad).

gnzlbg (Nov 15 2019 at 14:21, on Zulip):

My solution can be merged right now, and it closes the soundness bug, with minimal changes right now.

gnzlbg (Nov 15 2019 at 14:22, on Zulip):

If at some point Ralf's solution has consensus, it can also be applied, since it doesn't introduce the soundness bug.

gnzlbg (Nov 15 2019 at 14:22, on Zulip):

Like, as long as the soundness bug is not re-introduced, I don't really mind.

Josh Triplett (Nov 15 2019 at 23:46, on Zulip):

I personally would favor applying Ralf's change to stop applying nounwind.

Kyle Strand (Nov 16 2019 at 00:12, on Zulip):

I personally would favor applying Ralf's change to stop applying nounwind.

I think it's entirely up to the lang team, so I shall not give further input until asked for my opinion! :D

Kyle Strand (Nov 16 2019 at 00:13, on Zulip):

(It is a strenuous exercise in self-control.)

Josh Triplett (Nov 16 2019 at 11:09, on Zulip):

So, the last time this came up in a lang team meeting, the general sentiment was very much "let's leave it up to the ffi-unwind team". :)

Kyle Strand (Nov 16 2019 at 12:13, on Zulip):

Ah! I had thought that the decision was to wait for us to provide a "full" solution, i.e., one that would introduce the "opt out" mechanism. But that was back when we were fairly confident that an opt-out mechanism was needed, and that the default abort-on-unwind behavior should be stabilized as soon as an opt-out was stabilized. Now that we are reconsidering the default, and don't see a "quick" path to a full solution, I think the situation is substantially different and does require another lang-team decision (even if the decision is "we'll land whichever of those three solutions you recommend", or "the project-FFI group should choose between these two options").

Josh Triplett (Nov 16 2019 at 18:31, on Zulip):

@Kyle Strand We've talked about the individual RFCs and PRs, and in each case we want to defer both short-term and long-term solutions to the recommendations of the ffi-unwind team.

Josh Triplett (Nov 16 2019 at 18:32, on Zulip):

We do have the same concern about solutions that would make unsafe have semantic significance.

Josh Triplett (Nov 16 2019 at 18:33, on Zulip):

But, for instance, a short-term PR that removes nounwind, or a similar short-term solution, is one we'd be happy to merge if recommended by the ffi-unwind team.

Josh Triplett (Nov 16 2019 at 18:34, on Zulip):

And I think such a recommendation would help us unstick things that have not made forward progress.

Kyle Strand (Nov 16 2019 at 18:49, on Zulip):

Okay. I would recommend RalfJ's PR then.

Kyle Strand (Nov 16 2019 at 19:01, on Zulip):

This does bring to my attention that this group has an extremely loose concept of membership and no mechanism for voting or consensus...

centril (Nov 16 2019 at 23:14, on Zulip):

But, for instance, a short-term PR that removes nounwind, or a similar short-term solution, is one we'd be happy to merge if recommended by the ffi-unwind team.

What I said on the meeting is basically that I would be willing to listen to arguments, but that any decision is not delegated to the WG; also I haven't heard anything new that would make me happy to merge the PR

Kyle Strand (Nov 16 2019 at 23:33, on Zulip):

In this case, the lang team could try a "veto" approach. It sounds like Centril would veto the approach of no longer emitting nounwind by default, and JoshT would veto the approach of changing the codegen based on the presence of unsafe (a veto that I agree with). That leaves the option of re-stabilizing the abort logic. If no one on the lang team feels equally strongly against that option, perhaps it's the best temporary solution.

Josh Triplett (Nov 17 2019 at 01:18, on Zulip):

@centril I'm not suggesting that we're delegating the decision; I'm suggesting that we're seeking a recommendation.

Josh Triplett (Nov 17 2019 at 01:19, on Zulip):

@Kyle Strand Several people on the lang team felt uncomfortable with the abort approach precisely because it breaks existing code. I don't think that has changed.

Kyle Strand (Nov 17 2019 at 01:26, on Zulip):

In its current state (with nounwind), the code is already broken (even without the "it's UB so it's broken by definition" argument). Also, there are at least two projects that previously would have been broken by this change but no longer will be. rlua (and anything else using longjmp) was fixed by Alex Crichton (I can find the PR later, but this is explained in the PR thread for #64315), and Lucet is now on nightly and uses #[unwind(allowed)].

Amanieu (Nov 17 2019 at 08:44, on Zulip):

This one? https://github.com/rust-lang/rust/pull/48572

Kyle Strand (Nov 17 2019 at 12:08, on Zulip):

Yes, thanks!

Kyle Strand (Nov 17 2019 at 22:03, on Zulip):

@centril One reason I think RalfJ's PR is the best one to merge is because it is the one with the least-visible effect, which allows this group to continue working on a permanent solution. We would still consider the behavior undefined, and we would not encourage users to unwind across FFI boundaries. But we would be making user code less risk/error/UB prone in the case that this feature _is_ being used.

Conversely, if we re-stabilize abort-on-unwind, that simply forces users to do one of the following:

Last update: Dec 12 2019 at 00:55UTC