randomPoison / cs-bindgen

Experiment in using Rust to build a library that can be loaded by Unity across multiple platforms
4 stars 0 forks source link

Soundness of trusting Describe impls #28

Closed randomPoison closed 4 years ago

randomPoison commented 4 years ago

A potential source of unsoundness right now is the way the C# code generation logic needs to trust that the schema generated by a type's Describe impl both:

Right now, the unsafe bindings on the Rust and C# sides of the FFI boundary are both assuming that they work off the same information, with the schema generated by Describe being used to communicate that shared information to the C# code generator. However, this effectively means that the unsafe code generated by the bindings is relying on safe code in the Describe impl to be correct, which is not a sound thing to do because it means that a mistake in safe code can result in memory unsafety when the C# bindings don't line up with the Rust bindings.

For now this is mostly a theoretical issue, since we don't support manual impls for Describe or From/IntoAbi at this point (#[cs_bindgen] will generate impls for those traits, which would conflict with a user-provided impl), but we'll likely want to support that at some point.

The simplest solution here is to require that some unsafe marker trait be implemented for any type implementing Describe and From/IntoAbi, which would adds a "safety contract" to any manual implementation. We could also make From/IntoAbi unsafe traits, since there's some inherent safety contract to how they are implemented.

randomPoison commented 4 years ago

Thinking about it a bit more, the main thing we want to prevent is using #[cs_bindgen] to automatically generate bindings for a type that has a custom Describe impl. It should be possible to check the #[derive()] attribute(s) on the type from the #[cs_bindgen] proc macro and reject using #[cs_bidngen] if the type isn't also doing #[derive(Describe)].

randomPoison commented 4 years ago

This is effectively superseded by #59, since removing the dependency on schematic means we no longer need to account for manual Describe impls. There's still an open question around how to allow users to manually implement conversion and codegen for their custom typtes, but that's a much larger question that's outside the scope of this issue.