Review all safe code that takes raw pointers in stylo and other places

[nox]

Functions such as style::gecko::media_queries::Device::new take a RawGeckoPresContextOwned that is ultimately just a raw mutable pointer. This means one can write Device::new(0xdeadbeef as *mut _) and the code will happily compile and probably segfault at runtime, that is unsound, and this shouldn't be doable from safe code. There are probably many other occurrences of that problem in stylo in general, so we should review that.

Cc @Manishearth

[nox]

Most Servo_* functions take raw pointers, are public, and are exposed from outside the crate. These should be unsafe.

[Manishearth]

Most Servo_* functions take raw pointers, are public, and are exposed from outside the crate. These should be unsafe.

Sorry, I disagree.

We should not be calling these functions at all from Servo. It would be nice to have a tidy lint enforcing that.

Firstly, most Servo_ functions take borrows now. RawFooBorrowed is an actual borrow with a lifetime, despite being represented as a raw pointer on the C++ side. If the function is safe, it's likely accepting a borrow. That's the whole point of the safety bindings.

The problem with keeping them unsafe like this is that now basically everything is unsafe. We've diluted the utility of unsafe in pinpointing things that can go wrong. There's a larger issue of passing garbage data to the Servo_* functions, but that's a matter of C++s responsibility, if these funcs are only called from C++. We mark these funcs safe when we have various safety wrappers that let them work, and we have a safety contract with C++. This contract is not the same as the Rust safety contract, because C++ is different. We have to trust C++ at some level, ultimately there's no way for Rust to check that something is 100% legit. Marking the glue code as unsafe would help, but that's really a cosmetic change -- we already knew the glue depends on C++. The harm of marking it as unsafe is that we lose out on the distinction between safe and unsafe methods on the safety wrappers. It's nice to scope the unsafe to the actual unsafe operations happening, not just "oh yes this whole function contains unsafe bits I don't know which". This is all fine as long as these aren't called from Rust. They're not supposed to be, so that works out.

This has helped us catch some unsafe issues already, because it's very easy to root around unsafe code touching things related to a segfault you're seeing.

There is the implicit understanding that glue.rs relies on various contracts with C++ that are not expressed in the Rust way, so ultimately it's still known that it's unsafe to some degree, which is enough -- rather than making the explicit cosmetic change that hobbles the safety wrappers, we use the implicit one.

[nox]

It's not a matter of tidy, or whatever unspoken invariant you wished everyone followed. The function exists, the function is exported from the crate, the function does unsafe things, therefore it should be unsafe.

This has helped us catch some unsafe issues already, because it's very easy to root around unsafe code touching things related to a segfault you're seeing.

That's because the functions do too much, instead of doing pointer to reference conversions and such things, and then calling very few safe functions, it does many unrelated things, and that's why you want to reduce the amount of unsafe code.

I didn't know that the glue was even that unsafe, because all of the raw pointers are hidden behind type aliases. This code is dangerous, and this will bite us back in the future.

[eddyb]

Is there no way to export a function to C but not Rust? Or does #[no_mangle] forbid that?

[Manishearth]

the function is exported from the crate

An FFI crate that only contains extern C functions and is obviously to be called from C/++ code.

That's because the functions do too much, instead of doing pointer to reference conversions and such things, and then calling very few safe functions, it does many unrelated things, and that's why you want to reduce the amount of unsafe code.

Yes, this is an improvement I've wanted to do, it's just low priority for me (because of the aforementioned "it's safe as long as you don't call it from Rust"). Go ahead and do it if you want. It enables a lot of improvements in the way we do safety glue, too, reducing dependence on bindgen replace.

I didn't know that the glue was even that unsafe, because all of the raw pointers are hidden behind type aliases.

There's precisely one accessible raw pointer hidden behind an alias, and that's the owned pres context. I'm aware of this, but there were some tricky issues around pres context ownership (this ties into the Device issue) so it's been on the backburner. (If there are more, that should be fixed)

There is a bit of a safety hazard here in that you could accidentally use one of the owned/borrowed types without generating the right wrapper via build_gecko.rs. I have yet to figure out a way to deal with this; though we could somehow enforce that the type is not a pointer (there's already a unit test that enforces that the function types match).

There are a bunch of glue functions which take *mut directly (not a safety wrapper) that should probably be marked unsafe. I didn't realize these were the ones you were talking about; yes, they should be marked unsafe. I thought I'd fixed these, but more sprung up. Probably should lint this in the test-stylo pass. It would be even better if these could take safety wrappers (nsStringBorrowed or whatever) instead.

---

Here are all the safety issues that have been put on the backburner:

• The aforementioned pres context ownership issue. The pres context is one of those C++ things that's kinda-static-but-not-really; C++ code everywhere just assumes it exists without a way to enforce ownership, and the Rust code can pretty reliably assume it won't go away, but this kind of thing is imperfect so can't be expressed in the type system. It's been a while since I looked at this, but this is what I recall the situation to be.
• Atom wrappers. We currently pass *mut nsIAtom through the bindings; we should have a safety wrapper for that so that Rust can't pass in arbitrary pointers.
• Privacy: Most bindings-generated structs have public fields, even the ones which are raw pointers (or the ones which take part in invariants, like tag fields). We have an unwieldy private-field annotation that we use for this, but it would be nice if Rust actually got unsafe fields. It's a bit hard to separate the unsafe fields from the plain-old-data fields, so this is tricky
• Probably should merge structs::RefPtr and sugar::RefPtr now that inline drop flags no longer exist. There are other improvements that can be done.