Rust: prevent unwinding past FFI boundaries in code generation

[thestinger]

It's undefined to unwind past an FFI boundary such as a pub extern "C" fn. Code generation should automatically insert a landing pad doing an abort. This will eliminate the class of memory safety errors resulting from unwinding into C from Rust. LLVM will be able to optimize it out if it is being caught and handled explicitly, such as to translate into an error code for C.

EDIT: Mentoring instructions can be found here.

[pnkfelix]

Assigning P-high, not 1.0.

[steveklabnik]

Triage: I'm not aware of a code change here, I believe that we are expecting people to handle this themselves with catch_panic or whatever it's called.

/cc @rust-lang/libs @rust-lang/lang is that the only solution we are pursing here, or do we plan on doing it automatically eventually?

[aturon]

@steveklabnik IIRC, there's still work to do to ensure that you correctly get an abort if you don't recover from the panic before hitting the boundary. I believe that's what this issue is about.

[ranma42]

cc @ranma42

[coder543]

Has there been any work done on this? Automatically inserting catch_unwind statements at FFI boundaries that automatically abort on uncaught panics seems like a straightforward way to close this soundness hole. I see where @brson silently adjusted the tags, so maybe the core team reviewed it back in August.

[nikomatsakis]

UPDATE: This comment appears to have been a bit confused! See revised instructions below.

No work has been done on this to my knowledge, but I am in favor of the solution you proposed @coder543, and would be happy to work with you on implementing it. If nothing else, it would help us gain experience with overhead etc.

I think that the strategy we would use is to modify the code that generates MIR (our mid-level intermediate IR) for calls, which is found in this file here. You can see that it generates the code to execute upon unwind right here, on this line -- currently, this is always cleanup code. We could modify it to inspect the type of the value being invoked (and, in particular, its ABI) and generate alternate unwind code that aborts (I'm not sure the best way to encode an abort right now though).

The function being called is stored in the fun variable, which is an Operand<'tcx>. Operands support a ty method (defined here). In principle, I suppose, we could call it like fun.ty(&this.local_decls, this.hir.tcx()) -- that method is only meant to be called after MIR construction is complete, but I think it will actually work just fine. (Otherwise, we could extract the type of the function being called from the HAIR, which is the intermediate representation we are building things from; but that's just a touch more involved.)

The main question then is how to generate MIR that aborts. Right now I think we have no way to encode an abort into the IR. We could probably add an ABORT terminator (i.e., a new variant of the MIR data structure TerminatorKind, found in src/librustc/mir/mod.rs).

So, if you were going to implement this, I think we would do the following steps:

1. Add an ABORT terminator to TerminatorKind and work through the various implications. That could be a PR by itself, although it'd be presently unused.
2. Inspect the types of callees as described above, find those with C ABIs, and generate an ABORT terminator instead.

That might be two PRs, or maybe one. To get started, one might also skip the first step, and just experiment with having the compiler print out a match when it finds a C ABI function, and then worry about how to handle it.

[coder543]

I'm happy to give it a shot!

The main question then is how to generate MIR that aborts. Right now I think we have no way to encode an abort into the IR.

I mean, isn't using this in a function going to generate some MIR that leads to an abort? Would it be possible to just generate either that, or something equivalent to the MIR for ::sys::abort_internal?

To get started, one might also skip the first step, and just experiment with having the compiler print out a match when it finds a C ABI function, and then worry about how to handle it.

That seems like a good way to start this. I'll go ahead and get things set up here to start doing compiler dev. The compiler is doing its inital build now under Ubuntu 17.04.

[coder543]

I have things set up where I can print things to stderr from the relevant block of code, recompile through stage 1 for fast rebuilds, and then use the generated compiler to compile a small Rust source file, which causes those debug print statements to be generated.

So, everything is configured. Now, to "just" figure out how to print out a match when encountering a C ABI function. I'm done for tonight, but I hope to pick it up again tomorrow evening.

[coder543]

I actually ended up staying awake for a little longer, and I now have it only debug print a message when it encounters a function call to a C ABI function.

I'm not sure ExprKind::Call is the right place to do it, since that's only run at function call sites, not function declarations, and we won't know when a non-Rust user calls across the C FFI.

[coder543]

@nikomatsakis I think at this point it's safe to say I'm going to need help figuring out where to go from here. ExprKind::Call seems like a dead-end, sadly, unless I'm missing something.

[SimonSapin]

@coder543 Are you still interested in working on this? If not I might try to during the "impl days" in RustFest Zurich.

@nikomatsakis It sounds like this is blocked on a mentor providing some more help ;)

[arielb1]

I still doesn't understand whether this issue is supposed to apply to Rust code calling an extern "C" function or C code calling an extern "C" function.

[arielb1]

The current situation is that foreign functions are marked as nounwind in LLVM, so adding abort paths won't actually do anything because they'll just get pruned, and there's no point in modifying ExprKind::Call.

[SimonSapin]

I assumed the latter: Rust unwinding into some foreign stack frame is the case where we have some control?

[coder543]

@SimonSapin feel free to take over!

[nikomatsakis]

@coder543 hey, sorry I dropped the ball on those notifications! Missed them somehow. Still digging out of a months-long hole.

Also, re-reading my mentoring instructions, I think I was confused! That is, I also believe that the goal is to catch unwinds in an extern "C" fn foo() { .. } function, but the instructions I wrote seem to be talking about intercepting calls to extern "C" functions, which is sort of irrelevant. Sorry for the confusion.

That said, it's still true that we need an abort terminator, but the job is substantially easier I think. We just need to find when we are generating MIR for extern "C" functions and modify what we do in the final unwind block -- currently we generate (I think) a TerminatorKind::Resume, but we would instead generate TerminatorKind::Abort. (Alternatively, we could just change how to interpret Resume -- i.e., its meaning might depend on the ABI of the enclosing function, with anything non-Rust meaning "ABORT", but somehow that feels a bit hacky to me.)

@arielb1 does that sounds reasonable to you?

[coder543]

@nikomatsakis Hey, it's alright. I would have loved to help on this issue, but I'll look for other rustc issues to help with at some point when I have time again!

[diwic]

Okay, since I've been complaining about this bug I think maybe I should make an attempt to fix it. :-) But it's my first time this far into the compiler. I started off with this advice:

We just need to find when we are generating MIR for extern "C" functions and modify what we do in the final unwind block -- currently we generate (I think) a TerminatorKind::Resume, but we would instead generate TerminatorKind::Abort.

I could not find any specific "final unwind block" that's already generated. And in the case where the C function merely calls another (Rust) function, why should there be one?
So I think we need to make one. Perhaps like this?

edit: The function changed is librustc_mir/build/mod.rs:construct_fn.

    // We cannot unwind into FFI. Therefore generate an extra "Abort" landing pad.
    if abi != Abi::Rust && abi != Abi::RustCall && abi != Abi::RustIntrinsic {
        let abort_block = builder.cfg.start_new_cleanup_block();
        builder.cfg.terminate(abort_block, source_info, TerminatorKind::Abort);
    }

Gist with code in context

• Maybe !tcx.sess.no_landing_pads() should also be a condition for generating this landing pad?
• The ABIs are quite undocumented. I don't know if I picked the right ones here...

Does this seem reasonable, or am I just completely far off so that I better leave it to someone more knowledgeable in order not to consume your time?

(Ping @nikomatsakis or someone else who would like to provide some mentoring here)

[nikomatsakis]

@diwic you're in the right general direction -- but I think what I had in mind was modifying the terminator that we create on this particular line. This is lazilly constructed when there is a need to generate unwinding code.

(Oh, and plausibly yes we could do the same for panic=abort... not sure if it's worth messing with that now.)

PS feel free to reach out on gitter as well.

[diwic]

It's a lot to take in if you have not worked on the compiler's internals before. So I'm working on it, but not very fast...

@nikomatsakis Assume the following code:

fn pan() { panic!("Oh no!"); }
extern "C" fn middle() { pan(); }

Here's the MIR output for middle:

fn middle() -> () {
    let mut _0: ();                      // return pointer
    let mut _1: ();

    bb0: {
        _1 = const pan() -> bb1;         // scope 0 at src/main.rs:4:3: 4:8
    }

    bb1: {
        _0 = ();                         // scope 0 at src/main.rs:3:24: 5:2
        return;                          // scope 0 at src/main.rs:5:2: 5:2
    }
}

there is no resume cleanup block that we can modify. So instead we need to add a new one, I believe?

The ABIs are quite undocumented. I don't know if I picked the right ones here...

This is what I want to do:

tcx.is_foreign_item(tcx.hir.local_def_id(fn_id))

...because that's when we tell LLVM our function is nounwind. But probably I picked the wrong id, because it returns false regardless of abi.

[iainnicol]

Would it be possible to opt out of the abort generation? Perhaps by tagging the function with the existing #[unwind] attribute?

My use case is that I would like to be able to catch Rust panics from the C/C++ side. This is for writing a standard library for a toy language. I realise such unwinding is undefined and hence there are no guarantees. But it would still be useful, at my own risk.

[diwic]

@iainnicol

My use case is that I would like to be able to catch Rust panics from the C/C++ side.

I have two concerns about this approach: 1) does it really work in practice, and if so what kind of exception is a Rust exception in C++ and 2) why can't you use catch_unwind?

[diwic]

@nikomatsakis or anyone who would like to provide mentoring/review/feedback:

So I made some progress today! (Yay!) As can be seen in https://github.com/diwic/rust/commit/729092c3e39ab95157fd58a14b04db70c1514128

It's probably not ready for PR yet, could use some mentoring / feedback:

• Should tests be added and if so how can one test that an abort has happened? As of now I have only manually inspected mir and llvm-ir to see that it looks correct.
• I'm not sure why my is_foreign_item row is not working, I think that would be better if we could use it
• For all places where the compiler complained about a missing TerminatorKind::Abort branch, I basically added one that did the same as TerminatorKind::Resume (except for code generation). This might need to be double checked due to behavioral differences (after all, resume resumes and abort aborts).
• Not sure what to do on the mir inlining pass. But it does not seem to happen in my small tests, maybe this is something experimental which is not enabled by default?

[nikomatsakis]

@diwic

Should tests be added and if so how can one test that an abort has happened? As of now I have only manually inspected mir and llvm-ir to see that it looks correct.

Yes! Tests should definitely be added. One way to do this is to write a run-pass test that invokes itself using Command, passing some arguments and then checking the return code. sigpipe-should-be-ignored.rs is an example of a test that uses that technique.

I'm not sure why my is_foreign_item row is not working, I think that would be better if we could use it

Can you say a bit more, I'm not sure what that means?

For all places where the compiler complained about a missing TerminatorKind::Abort branch

OK, I'll double-check when you open a PR.

Not sure what to do on the mir inlining pass. But it does not seem to happen in my small tests, maybe this is something experimental which is not enabled by default?

It is experimental and does not happen by default. You can enable it with -Zmir-opt-level=2. It would be great if you did test it -- I think you should be able to "link up" the abort block from the inlined function into the abort block in the caller, presumably handling it in a similar fashion to how resume is handled or something...I'll have to go look in more depth.

In general, it'd be great if you wanted to open a PR and we can discuss in more depth there! Feel free to tag it with [WIP] in the subject line, and be sure to put r? @nikomatsakis so that it gets assigned to me.

[diwic]

Second attempt here: https://github.com/rust-lang/rust/pull/46833

[bstrie]

Thank you so much for pushing on this old bug @diwic!

[diwic]

@iainnicol

Would it be possible to opt out of the abort generation? Perhaps by tagging the function with the existing #[unwind] attribute?

JFTR, as implemented, tagging the function with the #[unwind] attribute does opt out of the abort generation.

[steveklabnik]

re-opening as 1.24.1 removed this behavior, even though it's expected to come back soon.

[nox]

rlua should be added to https://github.com/rust-lang/rust/blob/master/src/tools/cargotest/main.rs

[Mark-Simulacrum]

Closing this in favor of https://github.com/rust-lang/rust/issues/52652 (which is more recent and active).

[SimonSapin]

Oops, sorry I didn’t find this when opening #52652 !