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

[WIP] Make a table of trait object type_ids and vtable pointers available to programs #66113

Closed
wants to merge 4 commits into from

Conversation

alecmocatta
Copy link
Contributor

@alecmocatta alecmocatta commented Nov 5, 2019

This PR aims to enable safe and sound serialization and deserialization of trait objects on tier 1 platforms.

Some context: The ability to serialize and deserialize trait objects, or similarly closures, has been requested by various members of the community for some time: rust-lang/rfcs#668 rust-lang/rfcs#1022. The two main use cases are IPC – for example sending trait objects between forks of a process; and distributed computing – for example sending trait objects to processes distributed across a cluster. While some message kinds can be wrapped in an enum instead of a trait object, this can be inconvenient, and isn't viable for un-nameable types like closures. The goal of this PR is to lay the groundwork for safe and sound serialization and deserialization of trait objects in a low-impact manner that allows user crates to safely experiment and iterate on this functionality.

Deserializing trait objects is possible today, but only in a rather hairy manner. The crate serde_traitobject for example works and sees usage, but it has two unsoundness vectors:

  • a malicious MITM (who could forge ostensibly-valid but unsound data); and
  • multiple vtable segments in a single binary that are loaded at inconsistent relative addresses.

The latter requires a very odd linker script and is unlikely to occur in practise; the former is concerning and rules out many use cases. It is however sufficient for other use cases – in fact a really cool one using it was published recently: native_spark.

The approach I've taken in this PR is to create essentially a global array with appending linkage that stores structs comprising the type_id of the trait object and a pointer to the vtable, for all materialized vtables. Unfortunately appending linkage isn't really implemented by LLVM yet besides for a couple of special variables, so I've used the old-school approach of emitting static variables into an orphan section. The start and end of this section can be reliably retreived by user programs with help from the linker.

This array increases the size of libraries by a small amount; for example build/x86_64-apple-darwin/stage2 by ~1.3MiB i.e. ~0.2%. Thanks to --gc-sections on Linux and /OPT:REF on Windows, it's removed entirely from binaries that don't use it on those platforms. macOS's -dead_strip works a little differently, and the best solution I've found adds 16 bytes per used vtable to the resulting binary. This increases the size of binaries by a small amount: hello world grows by 76 bytes i.e. ~0.03%. With a bit more work I think this could probably be made zero-cost similar to Linux and Windows. Android and iOS behave the same as Linux and macOS, and on other platforms this PR is a no-op.

This array allows programs to retrieve a list of all materializable vtable pointers for a given dyn Trait. From this, a candidate concrete type for serialization can be selected and safely invoked. Here's a rough sketch of how serialization and deserialization can work:

use std::raw::TraitObject;
fn vtables() { /* see src/test/ui/vtables/vtable-list.rs */ }

// Process A
let send: Box<T> = send;
let send: Box<dyn Trait> = send;
let type_tag = send.type_tag();
type_tag.serialize();
trait_object.serialize();

// Process B
let trait_type_id = type_id::<dyn Trait>();
let type_tag = deserialize();
let trait_object = vtables().iter().find_map(|record| {
    if record.type_id != trait_type_id {
        return None;
    }
    let trait_object = TraitObject { data: ptr::dangling(), vtable: record.vtable };
    let trait_object = transmute::<TraitObject, *const dyn Trait>();
    (trait_object.type_tag() == type_tag).then(trait_object)
}).unwrap();
let send: Box<dyn Trait> = trait_object.deserialize_erased();

fn type_tag() here could be type_id(), which would work for unnameable types like closures but doesn't work across different builds where the type_id has changed due to potentially unrelated changes. It could be provided explicitly by the program (i.e. like typetag), which would not work for unnameable types but would work across different builds. It could also be a combination of both – explicit tags where provided, falling back to type_id.

I believe this is a step in the right direction to enabling sound trait object serialization and deserialization. It resolves the aforementioned security issue that exists in applications being used today, it enables user crates to safely iterate on the user interface for trait object deserialization, and it can hopefully co-evolve with the unsafe code guidelines and other RFCs like rust-lang/rfcs#2580 to increase the strength of its guarantees.

An upside of this PR is safe and sound serialization and deserialization of closures in conjunction with serde_closure - which is a proc macro that extracts captured variables from the AST and puts them in a struct that implements Serialize, Deserialize, Debug and other convenient traits. This is the major use case for distributed computing frameworks like constellation and native_spark.


I'd like to add a test that checks the array is removed by the linker on Linux and Windows if it's not used, but I'm not familiar with how to do this?

Let me know how best to feature gate it. Or if it perhaps doesn't need it – doing anything useful with the array is already unstable (i.e. transmuting dyn Trait <-> std::raw::TraitObject to get/set the vtable). Per the RFC guidelines as this should be "invisible to users-of-rust" I've gone ahead with this PR – let me know if it does indeed need an RFC. And lastly it's probably worth confirming it is indeed "invisible to users-of-rust" with a crater and/or perf run?

@rust-highfive
Copy link
Collaborator

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2019
@estebank
Copy link
Contributor

estebank commented Nov 6, 2019

@rust-lang/compiler @rust-lang/lang

@estebank estebank added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 6, 2019
@Rufflewind
Copy link
Contributor

Thanks for moving the idea forward! I'm curious about the safety of this approach: What would happen if a binary tries to deserialize a trait object sent by a binary of a different version? (Or built with different flags?) What kind of safeguards are in place?

The semantics of type IDs themselves are a bit underspecified for this. Is it possible to get false positives where one binary shares the same type ID as another binary yet are actually different types?

@alecmocatta
Copy link
Contributor Author

Thanks for your feedback @Rufflewind! I made a correction to the example deserialization in my first post.

As long as a vtable pointer is known to belong to dyn Trait before being cast to *const dyn Trait, then it is safe. The worst that can happen is then the same as trying to deserialize to the wrong type, for example deserializing to a String what is actually a serialized u64: OOM, dodgy values, or an error raised, depending on the implementation.

The intent of embedding a mapping of type_id to vtable pointers is to provide this knowledge to programs; such that they can look up for a given dyn Trait what the valid vtable pointers are. You're right that dyn Trait type_ids can collide within a binary, and that is indeed an issue.

The approach I envisage for the first time a particular dyn Trait is deserialized:

  • take the records that have a matching type_id
  • create *const dyn Trait pointers from their vtables
  • call a trait method with *const Trait receiver on each to retrieve their concrete type's type_ids
  • cache the trait_type_id -> concrete_type_id -> vtable pointer mapping in a hashmap static variable.

If a program is deserializing dyn Trait, and some dyn OtherTrait has the same type_id, then the assumption that the records with matching type_ids have vtable pointers belonging to dyn Trait doesn't hold, as they could belong to dyn OtherTrait. Such a collision would result in mis-typing vtables and is therefore unsound.

The probability of at least one collision, and thus the probability of a vulnerable binary, can be calculated with a generalisation of the birthday problem. m is the number of distinct dyn Traits that the program deserialises; n is the number of distinct dyn OtherTraits, d is 2^64. It seems hard to calculate (i.e. Mathematica's not returning an answer any time soon) but an approximation, with m = 10000, n = 1000000000 gives 5.4... × 10^-7.

Still, this can and should be fixed, and the approaches sounded out in #10389 do fix this.

Regarding inter-binary concrete T type_id collisions, they do not pose a safety/soundness issue. The concrete type_id is what's sent over the wire, so it's important that it's not possible to trigger unsound behaviour by manipulating it. The worst that can happen is attempting to deserialize data as the wrong concrete type.

@estebank estebank added I-needs-decision Issue: In need of a decision. I-nominated labels Nov 6, 2019
@bors
Copy link
Contributor

bors commented Nov 7, 2019

☔ The latest upstream changes (presumably #66175) made this pull request unmergeable. Please resolve the merge conflicts.

@bjorn3
Copy link
Member

bjorn3 commented Nov 7, 2019

This is not feature gated.

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 7, 2019

Some initial thoughts:

I think that this change is too large to land in an informal PR like this. This is specifically creating groundwork that crates will come to rely on -- nightly crates, but still. That means we'll have to support this mechanism in some form going forward, and I wouldn't want to create that dependency without a plan to "complete" the work. I also think that there are a lot of moving parts here, and this design is too complex to move forward without a proper discussion.

Ultimately I think the right venue would be something more like the ffi-unwind project group that we are working on. Basically, get some people together to explore the problem and its complications and propose solutions. But such an effort needs to be shepherded in conjunction with the lang team, I think, and that will require some bandwidth -- I'm not sure if we have it right now or not.

(This is also one of those issues that falls a bit outside the "core focus areas" of most of the lang team, but I think that can be addressed by incorporating people like @alecmocatta or others as the shepherds and key members.)

@alecmocatta
Copy link
Contributor Author

Okay thanks @nikomatsakis that sounds like a good plan of action.

I've gone ahead and created a place to collaborate on an RFC: https://github.com/alecmocatta/trait-object-deserialization. I invite anyone with an interest or any insight to feel free to contribute to it! I'll input to it myself over the coming weeks.

I think the areas that currently need to be explored are:

  • Use cases – define what they are and what their requirements are;
  • The goal – particularly around defining what precisely is deserializable where;
  • Approaches & prior art – what are the alternative ways to realise the goal;
  • Ramifications – what are the broader effects on the language.

Please feel free to nudge me in the right direction if I'm getting the wrong end of the stick!

@pnkfelix
Copy link
Member

pnkfelix commented Nov 14, 2019

(removing nomination label; the discussion required here is too large for the triage meeting.)

((update: oops, sorry, I shouldn't have removed nomination without waiting for lang team meeting first...))

@Rufflewind
Copy link
Contributor

I've gone ahead and created a place to collaborate on an RFC: https://github.com/alecmocatta/trait-object-deserialization. I invite anyone with an interest or any insight to feel free to contribute to it! I'll input to it myself over the coming weeks.

Link is broken?

@alecmocatta
Copy link
Contributor Author

@Rufflewind Thanks, fixed!

@Rufflewind
Copy link
Contributor

This prior art may be of relevance (see Section 5): http://research.microsoft.com/en-us/um/people/simonpj/papers/parallel/remote.pdf

@Dylan-DPC-zz
Copy link

Marking this as blocked on the rfc

@Dylan-DPC-zz Dylan-DPC-zz added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 27, 2019
@Dylan-DPC-zz
Copy link

Since this is blocked on an rfc which will take some time to go through the process, i'm closing this PR. We can re-open it or preferably start a new PR once that's done :) Thanks for contributing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I-needs-decision Issue: In need of a decision. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants