trait_upcasting unsoundness due to reordered super traits

[lrh2000]

The following program will trigger a segmentation fault. (playground link)

#![feature(trait_upcasting)]

trait Pollable {
    #[allow(unused)]
    fn poll(&self) {}
}
trait FileIo: Pollable + Send + Sync {
    fn read(&self) {}
}
trait Terminal: Send + Sync + FileIo {}

struct A;

impl Pollable for A {}
impl FileIo for A {}
impl Terminal for A {}

fn main() {
    let a = A;

    let b = &a as &dyn Terminal;
    let c = b as &dyn FileIo;
 
    c.read();
}

This is because the vtable layout of Send + Sync + FileIo (i.e., the supertrait of Terminal) is different from that of FileIo, even though (i) Send and Sync are auto traits and (ii) FileIo is Send and Sync. However, it seems that the upcast coerion thinks they are the same, so it reuses the vtable, causing segmentation faults.

vtables

.L__unnamed_1:    # `dyn Terminal`'s vtable
	.asciz	"\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\001\000\000\000\000\000\000"
	.quad	playground::FileIo::read    # `Pollable::poll`
	.quad	playground::FileIo::read    # `FileIo::read`

.L__unnamed_2:    # `dyn FileIo`'s vtable
	.asciz	"\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\000\001\000\000\000\000\000\000"
	.quad	playground::FileIo::read    # `Pollable::poll`
	.quad	.L__unnamed_3               # `Send`'s vptr
	.quad	.L__unnamed_3               # `Sync`'s vptr
	.quad	playground::FileIo::read    # `FileIo::read`

I checked how the vtable layout is generated:

rust/compiler/rustc_trait_selection/src/traits/vtable.rs

Lines 157 to 168 in 798fb83

	segment_visitor(VtblSegment::TraitOwnEntries {
	trait_ref: inner_most_trait_ref,
	emit_vptr: emit_vptr && !tcx.sess.opts.unstable_opts.no_trait_vptr,
	})?;
	// If we've emitted (fed to `segment_visitor`) a trait that has methods present in the vtable,
	// we'll need to emit vptrs from now on.
	if !emit_vptr_on_new_entry
	&& has_own_existential_vtable_entries(tcx, inner_most_trait_ref.def_id())
	{
	emit_vptr_on_new_entry = true;
	}

By the current implementation, Send + Sync + FileIo will not have vptrs for Send and Sync because Send and Sync are iterated at the very first during the prepare_vtable_segments_inner method. So emit_vptr_on_new_entry is false when Send and Sync are iterated.

• However, FileIo will have vptrs for Send and Sync because Send and Sync are iterated after Pollable is iterated. This means that prepare_vtable_segments_inner is true when Send and Sync are iterated.

This sounds a little strange to me.

Patch

I tried the following patch, which makes both Send + Sync + FileIo and FileIo not have vptrs for Send and Sync. I confirmed that the reported problem disappears, but I don't know if I'm on the right track since I'm not familiar with the code. (I can open a PR if it sounds right).

patch

diff --git a/compiler/rustc_trait_selection/src/traits/vtable.rs b/compiler/rustc_trait_selection/src/traits/vtable.rs
index 6e6f948a2cd..ed221e2a183 100644
--- a/compiler/rustc_trait_selection/src/traits/vtable.rs
+++ b/compiler/rustc_trait_selection/src/traits/vtable.rs
@@ -154,18 +154,17 @@ fn prepare_vtable_segments_inner<'tcx, T>(
 
         // emit innermost item, move to next sibling and stop there if possible, otherwise jump to outer level.
         while let Some((inner_most_trait_ref, emit_vptr, mut siblings)) = stack.pop() {
+            let has_entries =
+                has_own_existential_vtable_entries(tcx, inner_most_trait_ref.def_id());
+
             segment_visitor(VtblSegment::TraitOwnEntries {
                 trait_ref: inner_most_trait_ref,
-                emit_vptr: emit_vptr && !tcx.sess.opts.unstable_opts.no_trait_vptr,
+                emit_vptr: emit_vptr && has_entries && !tcx.sess.opts.unstable_opts.no_trait_vptr,
             })?;
 
             // If we've emitted (fed to `segment_visitor`) a trait that has methods present in the vtable,
             // we'll need to emit vptrs from now on.
-            if !emit_vptr_on_new_entry
-                && has_own_existential_vtable_entries(tcx, inner_most_trait_ref.def_id())
-            {
-                emit_vptr_on_new_entry = true;
-            }
+            emit_vptr_on_new_entry |= has_entries;
 
             if let Some(next_inner_most_trait_ref) =
                 siblings.find(|&sibling| visited.insert(sibling.upcast(tcx)))

Cc #65991 (tracking issue for trait_upcasting)

@rustbot label +F-trait_upcasting +A-coercions +A-trait-objects +I-unsound +T-lang +T-type

[workingjubilee]

hey rustbot:

@rustbot label: +F-trait_upcasting +A-coercions +A-trait-objects +I-unsound +T-lang +T-types

[lcnr]

I would probably continue emitting vtables for ordinary traits which don't have any entries but instead make sure that we never emit any entries for auto traits. cc @compiler-errors @WaffleLapkin

[lcnr]

slightly minimized

#![feature(trait_upcasting)]

trait Pollable {
    #[allow(unused)]
    fn poll(&self) {}
}
trait FileIo: Pollable + Send {
    fn read(&self) {}
}
trait Terminal: Send + FileIo {}

struct A;

impl Pollable for A {}
impl FileIo for A {}
impl Terminal for A {}

fn main() {
    let a = A;

    let b = &a as &dyn Terminal;
    let c = b as &dyn FileIo;
 
    c.read();
}

[lrh2000]

I would probably continue emitting vtables for ordinary traits which don't have any entries but instead make sure that we never emit any entries for auto traits.

The specific issue has nothing to do with auto traits (although I mentioned them in the title and use it as an example throughout the issue description). Hope this is not misleading.

If you replace the Send trait with another empty trait (say Evil), the segmentation fault is still there, at least. (playground link)

[lcnr]

Okay, that's very good to know 👍 I thought it only impacted auto traits, moved that concern into a separate issue #131813

[WaffleLapkin]

@lrh2000 the patch you provided looks right to me, please do open a PR :)

Interestingly, I opened #114942 a while ago, but didn't know it can cause unsoundness. I do wander if there are other cases where the ordering of super traits messes up our layout algorithm...

[lrh2000]

the patch you provided looks right to me, please do open a PR :)

@WaffleLapkin Thanks for the confirmation! I opened at #131864.