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

Use catch_unwind to prevent panicking across FFI. #25

Merged
merged 2 commits into from
Jan 21, 2021
Merged

Conversation

jsha
Copy link
Collaborator

@jsha jsha commented Jan 15, 2021

Panicking across FFI is undefined behavior, so each of our exported
functions must use catch_unwind and return an error (if an error result
is possible) or a default value.

Panicking across FFI is undefined behavior, so each of our exported
functions must use catch_unwind and return an error (if an error result
is possible) or a default value.
@jsha jsha requested a review from tgeoghegan January 15, 2021 23:08
Copy link
Collaborator

@tgeoghegan tgeoghegan left a comment

Choose a reason for hiding this comment

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

LGTM! The only things that occur to me (and I'm not sure how to do either, or whether either is possible) are:

  • is there a way to define perhaps an attribute macro that could be applied to functions that would automatically wrap their bodies in ffi_panic_boundary_*, without requiring the indentation of the whole function body?
  • is there a way to ensure that new functions added to crustls will be appropriately wrapped with a ffi_panic_boundary_* macro?

@jsha
Copy link
Collaborator Author

jsha commented Jan 21, 2021

is there a way to define perhaps an attribute macro that could be applied to functions that would automatically wrap their bodies in ffi_panic_boundary_*, without requiring the indentation of the whole function body?

This is appealing! I would like to avoid the extra indentation. After reading up, it sounds like attribute macros are a type of procedural macro: https://doc.rust-lang.org/reference/procedural-macros.html#attribute-macros

Procedural macros allow you to run code at compile time that operates over Rust syntax, both consuming and producing Rust syntax. You can sort of think of procedural macros as functions from an AST to another AST.

Procedural macros must be defined in a crate with the crate type of proc-macro.

Procedural macros are unhygienic. This means they behave as if the output token stream was simply written inline to the code it's next to. This means that it's affected by external items and also affects external imports.

Whereas what I'm defining here with macro_rules is a "macro by example:" https://doc.rust-lang.org/reference/macros-by-example.html

macro_rules allows users to define syntax extension in a declarative way. We call such extensions "macros by example" or simply "macros".

It sounds like procedural macros are more powerful, and also more complex, than macros. I'd rather stick with the simplicity of what I've got here and pay the (small) price of excess indentation.

is there a way to ensure that new functions added to crustls will be appropriately wrapped with a ffi_panic_boundary_* macro?

I've asked around and heard a few ideas, from static analysis of the code to control flow analysis of a compiled binary. I think for now I'll settle for a script that greps for pub.*fn and ensures the next line has ffi_panic_boundary_. I'll do that as a separate change: #27.

@jsha jsha merged commit e6e7e4e into main Jan 21, 2021
@jsha jsha deleted the panic-boundary branch January 21, 2021 03:24
@jsha
Copy link
Collaborator Author

jsha commented Jan 22, 2021

By the way, @tgeoghegan you had asked in chat why the repetition operator * in $(tt:tt)* is needed, given that tt matches a token tree. I played around with it, and found that the below, with no repetition operator, does work:

#[macro_export]
macro_rules! boundary {
    ($tokentree:tt) => {
        $tokentree
    };
}
fn main() {
    boundary! {{
        let mut x = 1;
        x += 2;
        println!("{}", x);
    }}
}

But note that I needed to surround the affected code with an extra layer of braces ({{). As I understand it, the macro operates on everything within the pair of braces (or parens, or brackets) it is parameterized by. So if I do:

boundary! {
  let mut x = 1;
}

That's not a single tokenTree, it's 6 tokens (each of which can be its own tokenTree). But this:

boundary! {
  {
    let mut x = 1;
  }
}

Is a single tokenTree that contains 6 tokens.

@tgeoghegan
Copy link
Collaborator

That's pretty trippy, but it makes sense. Thanks for doing the research. And FWIW I agree with your other conclusions about avoiding procedural macros and static analysis.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants