1332737 - Ensure enums and structs passed across C++ / Rust language boundary are binary compatible

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

After some discussion with Botond (who in turn was discussing with :mystor) it looks like the way we pass enums from c++ to rust across the webrender API is still not safe. See IRC conversation starting around [1].

In summary, because webrender_traits::ImageFormat is defined without repr(C), we shouldn't be blindly mapping WrImageFormat to webrender_traits::ImageFormat, as they may be different sizes. Short of using rust-bindgen to generate the bindings, we should either directly annotate webrender_traits::ImageFormat with repr(C), or add another enum type in webrender_bindings/src/bindings.rs that *does* have repr(C), and knows how to convert itself to webrender_traits::ImageFormat. This is exactly what we do for structs - we have a |extern "C"| version (e.g. WrRect) in webrender_ffi.h, which is binary-compatible with the repr(C) rust WrRect in bindings.rs, and that knows how to convert itself to the "real" rust type (e.g. LayoutRect) via the to_rect() function.

[1] http://logs.glob.uno/?c=mozilla%23gfx&s=20%20Jan%202017&e=20%20Jan%202017#c291394

Comment 1

[Emilio Cobos Álvarez [:emilio]]

FYI, rust-bindgen is now in central and runs automatically for stylo builds (bug 1302028), so it shouldn't be hard to hook it up also in the graphics branch.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

We have structs being passed across the boundary as well that are probably not safe. For example wr_renderer_current_epoch takes a PipelineId in rust (which is a (u32, u32) tuple that doesn't have repr(C) on it). And on the C++ side we're passing in a uint64_t. I'm not sure that's guaranteed to line up exactly. I didn't do a full audit but most other functions that take a pipeline id take it as u64 in rust and then use u64_to_pipeline_id to convert it, which should work. It's not enforced anywhere though.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)
> FYI, rust-bindgen is now in central and runs automatically for stylo builds
> (bug 1302028), so it shouldn't be hard to hook it up also in the graphics
> branch.

I'm not sure if bindgen will work for us, given that it generates rust bindings from a C/C++ header. We need to go the other way (generating C/C++ wrappers around existing rust code). The rusty-cheddar crate does this, but will probably require some work to get it all hooked up and integrated, make sure it works properly, etc. We discussed this briefly this morning and it seems like it might be a good intern project. In the meantime we're just going to clean up our existing bindings manually.

Comment 4

[Nicolas Silva [:nical]]

I am working on a patch that generates a #[repr(C)] version of every ffi type and with conversion methods from and to the rust equivalents using a macro. With that we'll only need to be disciplined so that types match in bindings.rs and webrender_ffi.h, which should be enough in the short term.

Comment 5

[Nicolas Silva [:nical]]

The current situation is: rust c-like enums that are exposed through ffi are all repr(u32) and their C++ equivalent also set to use uint32_t representation (enum class WrFoo: uint32_t). Many of the simple types in webrender's API are marked as repr(C) in order to be able to use them in the ffi instead of creating ffi equivalents when possible.

In bindings.rs there is a macro check_ffi_type which checks that the size of some of the struct and enums from the API which we expose at the ffi boundary have the size we expect. This means that if the size of the structs change in webrender, we'll notice because it is likely to break the build when we do the next webrender update (The fix is then to propagate the change to the C++ types and update the macro invocation in bindings.rs).
The macro could be improved to check not only the size of the struct but also the size of the members.

In any case I think that we are now in a good spot compared to when the bug was filed so I'll close this, let's open new bugs if we want to improve on this.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Nicolas Silva [:nical] from comment #5)
> The current situation is: rust c-like enums that are exposed through ffi are
> all repr(u32) and their C++ equivalent also set to use uint32_t
> representation (enum class WrFoo: uint32_t).

Are we sure that repr(C) is always the same as repr(u32) on the Rust side? Because you say they are all repr(u32) but in the code they are marked as repr(C) - if they are not guaranteed to be identical then we have a problem still.

> Many of the simple types in
> webrender's API are marked as repr(C) in order to be able to use them in the
> ffi instead of creating ffi equivalents when possible.

Is there any reason why some of the enums are using your check_ffi_type macro and others have an extra layer of wrapper enums? For example we use ImageFormat directly but MixBlendMode has an extra wrapper (WrMixBlendMode) in bindings.rs.

> In bindings.rs there is a macro check_ffi_type which checks that the size of
> some of the struct and enums from the API which we expose at the ffi
> boundary have the size we expect. This means that if the size of the structs
> change in webrender, we'll notice because it is likely to break the build
> when we do the next webrender update (The fix is then to propagate the
> change to the C++ types and update the macro invocation in bindings.rs).
> The macro could be improved to check not only the size of the struct but
> also the size of the members.

There's still the problem of ordering of elements. Right now if somebody swaps the order of two enum entries in webrender_traits that will not be detected and stuff will just silently start breaking. If we don't have test coverage for that particular enum it might get missed.

> In any case I think that we are now in a good spot compared to when the bug
> was filed so I'll close this, let's open new bugs if we want to improve on
> this.

I agree that we are in a better state than we were before, but I don't think we call things "safe" yet. :( Since no patches landed as part of this bug I'd rather just keep this open (maybe as a metabug) to track these remaining issues. I'll give it some more thought and see if I can file more concrete dependent bugs with things to do.

Comment 7

[Nicolas Silva [:nical]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> Are we sure that repr(C) is always the same as repr(u32) on the Rust side?
> Because you say they are all repr(u32) but in the code they are marked as
> repr(C) - if they are not guaranteed to be identical then we have a problem
> still.

Structs and enums are different. My understanding is that for enums repr(u32) implies all of the restrictions of repr(C) already (Or that's what I understood from the reviews when I added the annotations in WebRender). It looks like most enums that can be repr(u32) are marked as such, and on the C++ side they are enum class Foo: uint32_t. Structs on the other hand are repr(C), since repr(u32) does not make sense for them as far as I know. repr(C) should mean identical layout as an equivalent struct definition in an extern C block in C++ land.  


> Is there any reason why some of the enums are using your check_ffi_type
> macro and others have an extra layer of wrapper enums?

No reason I can think of (I think that more things were marked repr(C)/repr(u32) around the same time as the macros landed, but now that this has settled we should convert the missing ones). 

> There's still the problem of ordering of elements.

Definitely.

> 
> I agree that we are in a better state than we were before, but I don't think
> we call things "safe" yet. :( 

Yeah, I guess I should say we went from being downright incorrect on some calling conventions, to being correct provided we are being careful about a few things. We should definitely fix get rid of these few things but isn't as much of an emergency as it was before.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

So now that we are generating bindings using cbindgen I think we're in much better shape, as it automatically checks for things like repr(C)/repr(u32), actual values inside the enums, and ensures all the arguments are lined up correctly.