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

Is Box FFI-safe? #157

Closed
stephaneyfx opened this issue Jul 6, 2019 · 21 comments
Closed

Is Box FFI-safe? #157

stephaneyfx opened this issue Jul 6, 2019 · 21 comments

Comments

@stephaneyfx
Copy link

The section about the representation of pointers gives an example of C API using Box<T> on the Rust side and mapping it to struct T* on the C side. Box<T> is used both as parameter and return types. If it is sound to use Box<SomethingSized> across FFI boundaries and handle it as struct SomethingSized* or struct SomethingSized const* on the C side, wouldn't it be worth adding a short paragraph stating so to complement the example? It's reassuring to have a clear statement about what is or isn't sound.

If this is already specified somewhere, I apologize. I couldn't find a definitive answer.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 7, 2019

Box<T> has the same layout as *mut T - that's guaranteed in the Box documentation, which says that it is always valid to convert one into the other.

The representation of pointers section guarantees that *mut T has the same layout as a C raw pointer iff T: Sized.

In the example you mentioned, T: Sized, so Box<T> is identical to a T* in C, and can be used in C FFI to interface with that.

@stephaneyfx
Copy link
Author

stephaneyfx commented Jul 7, 2019

@gnzlbg Thank you for your answer.

The Box documentation just says "A pointer type for heap allocation" and links to the module documentation. The boxed module documentation does not state clearly that "Box<SomethingSized> has the same layout as *mut SomethingSized" or that they can be used interchangeably in extern "C" functions. Instead, it specifies that Box::into_raw and Box::from_raw can be used to convert between them: "More precisely, a value: *mut T [...] may be converted into a box using Box::<T>::from_raw(value). Conversely, the memory backing a value: *mut T obtained from Box::<T>::into_raw may be deallocated [...]". And this is what I have been doing in my FFI code, but advanced Rust developers on Discord suggested to avoid juggling with into_raw and from_raw and just use Box<SomethingSized> and *mut SomethingSized/*const SomethingSized interchangeably instead, leading to less unsafe code and fewer chances to leak. They also suggested that opening a documentation issue might be worth it.

If manipulating these 2 types interchangeably in FFI is sound, it would be nice to have a clear statement in the docs rather than just a single code example happening to rely on this fact without further explanations. For example, the nomicon FFI section clearly states that Option<extern "C" fn()> corresponds to a nullable function pointer in C: "So Option<extern "C" fn(c_int) -> c_int> is a correct way to represent a nullable function pointer using the C ABI (corresponding to the C type int (*)(int))."

I can help and open a doc PR.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 7, 2019

The section of the documentation I meant is here:

It is valid to convert both ways between a Box and a raw pointer allocated with the Global allocator

For me that's clear. Looking at the implementation of Box<T>, it is not repr(transparent), but that might just be a bug.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 7, 2019

Given that Unique<T> is repr(transparent) (see here), for Box the omission appears to be a bug.

@CryZe
Copy link

CryZe commented Jul 7, 2019

It is valid to convert both ways between a Box and a raw pointer allocated with the Global allocator

For me that's clear.

That sounds like you are meant to explicitly convert between them though. That doesn't necessarily imply they are layout compatible. I think that's still vague enough that it's worth clarifying either in Box's documentation or somewhere in the UCG reference.

I guess just adding #[transparent] on it might be enough though, if that happens.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 7, 2019

Right now Box<T> isn't transparent, but that's a bug: rust-lang/rust#52976

Layout-wise, it has currently the same layout as a *mut T as long as T: Sized, but that's unspecified. The right place to specify that would be the Box documentation.

@Lokathor
Copy link
Contributor

Lokathor commented Jul 7, 2019

The repr(transparent) RFC states

As a matter of optimisation, eligible #[repr(Rust)] structs behave as if they were #[repr(transparent)] but as an implementation detail that can't be relied upon by users.

So, if the RFC was successfully implemented correctly then you can non-reliably use Box as if it was transparent already.

We should of course get it upgraded to be repr(transparent) with full certainty, but until then code can limp along if absolutely necessary.

@stephaneyfx
Copy link
Author

@gnzlbg Thank you for linking this other issue. I will open a PR for the Box documentation.

@stephaneyfx
Copy link
Author

Closing as rust-lang/rust#62514 was made for the Box documentation as instructed.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

FWIW this PR #164 here would guarantee this for many cases, independently of whether Box documents it or is transparent or not.

@hanna-kruppe
Copy link

Hold up. #164 only talks about layout, and while we've been bikeshedding what exactly "layout" is, it isn't currently officially defined to include call ABI and the discussion being written up (#34) definitely happened back when layout including call ABI wasn't even on the table.

(This is actually part of a larger worry for me: all this bikeshedding about "layout" could cause immense confusion, and if we ever pull the trigger on redefining it we'll have to audit all uses of the term in existing write-ups.)

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 9, 2019

#164 only talks about layout, and while we've been bikeshedding what exactly "layout" is, it isn't currently officially defined to include call ABI and the discussion being written up (#34) definitely happened back when layout including call ABI wasn't even on the table.

The intent of #164 is to include call ABI, if that gets included as part of layout. I suppose we could be more explicit here.

(This is actually part of a larger worry for me: all this bikeshedding about "layout" could cause immense confusion, and if we ever pull the trigger on redefining it we'll have to audit all uses of the term in existing write-ups.)

I think this kind of iteration is inevitable, and some of these terms will get tweaked until the validity discussions conclude. Before preparing an RFC, we need to make sure that the definitions are consistent, and that the text uses them appropriately.

@Lokathor
Copy link
Contributor

Lokathor commented Jul 9, 2019

For clarification, if #164 happens, then that's basically saying "anything not explicitly repr(transparent) implicitly is transparent anyway as long as it qualifies"?

stephaneyfx added a commit to stephaneyfx/rust that referenced this issue Jul 10, 2019
This officializes what was only shown as a code example in [the unsafe code guidelines](https://rust-lang.github.io/unsafe-code-guidelines/layout/function-pointers.html?highlight=box#use) and follows [the discussion](rust-lang/unsafe-code-guidelines#157) in the corresponding repository.

It is also related to [the issue](rust-lang#52976) regarding marking `Box<T>` `#[repr(transparent)]`.
@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 10, 2019

@Lokathor I don't know the exact rules for repr(transparent) on structs, but the only thing repr(transparent) actually does is produce a nice error if the layout of a type would differ from the layout of one of its fields. It has nothing to do with what layout that type has if you omit repr(transparent), and I think it does make sense to guarantee them to be the same, at least for some cases.

Maybe we would need a repr(transparent) sub section in some general layout page about this.

@Lokathor
Copy link
Contributor

Yeah if it's just an error check against possible future changes that's important to know.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 10, 2019

Well, right now is the only thing that's guaranteed to give you that layout. If you omit repr(transparent), you might or might not get the layout - you definitely cannot "just" rely on it, and would need to inspect compiler internals, etc. and would be depending on an implementation detail.

If we were to guarantee #164, you could just rely on it, always. Independently of whether you use repr(transparent) or not.

In general, this wouldn't matter, because if you can stamp #[repr(transparent)], why not just do that ?

But for Box<T, A> one cannot stamp #[repr(transparent)] because A isn't necessarily a ZST. Without #164, you can't, however, rely on it even if A were a ZST because Box isn't transparent.

@hanna-kruppe
Copy link

Also note that even after #164 is merged, it'll be just a proposal not yet approved by the lang team.

@Lokathor
Copy link
Contributor

It'll be a proposal that's also already backed by an approved RFC ;3

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 10, 2019

It'll be a proposal that's also already backed by an approved RFC ;3

There is no RFC backing this. If anything, the repr(transparent) RFC made the opposite case: that these types of guarantees should be very explicit, so that people don't have to know a bunch of rules to know what they can or cannot rely on. If a struct is repr(transparent), then that's clearly and explicitly part of that type's API, and you are statically guaranteed to get an error if the type cannot be repr(transparent). I think this is a fair point.

I imagine that, at some point, once "layout" is 100% set in stone, we could come up with a set of APIs that allow users to query the layout properties of a type, and write code to check, e.g., whether two types are layout compatible, and/or how compatible they are. That would allow writing reliable unsafe code, that is safe even if a third-party library decides to silently remove a repr(transparent) annotation, which is something that can happen today with repr(transparent), but that unsafe code cannot defend itself against.

@Lokathor
Copy link
Contributor

in my comment above, #157 (comment), the quote block

As a matter of optimisation, eligible #[repr(Rust)] structs behave as if they were #[repr(transparent)] but as an implementation detail that can't be relied upon by users.

is a direct quote from the RFC. the only thing that 164 would change would be a promotion from "implementation detail" to "actually we're set on this now".

@RalfJung
Copy link
Member

is a direct quote from the RFC. the only thing that 164 would change would be a promotion from "implementation detail" to "actually we're set on this now".

Indeed, and that change is not backed by any RFC.

Hold up. #164 only talks about layout, and while we've been bikeshedding what exactly "layout" is, it isn't currently officially defined to include call ABI and the discussion being written up (#34) definitely happened back when layout including call ABI wasn't even on the table.

We should really resolve the question "what is layout" ASAP and then review our existing documents to make sure we are stating the things we want to state (and probably link to the glossary definition of layout every time we say things like "the layout of X and Y are equal").

JohnTitor added a commit to JohnTitor/rust that referenced this issue Dec 12, 2019
Clarify `Box<T>` representation and its use in FFI

This officializes what was only shown as a code example in [the unsafe code guidelines](https://rust-lang.github.io/unsafe-code-guidelines/layout/function-pointers.html?highlight=box#use) and follows [the discussion](rust-lang/unsafe-code-guidelines#157) in the corresponding repository.

It is also related to [the issue](rust-lang#52976) regarding marking `Box<T>` `#[repr(transparent)]`.

If the statement this PR adds is incorrect or a more in-depth discussion is warranted, I apologize. Should it be the case, the example in the unsafe code guidelines should be amended and some document should make it clear that it is not sound/supported.
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

No branches or pull requests

6 participants