Repository navigation
Pointer casts allow switching trait parameters for trait objects, which doesn’t interact soundly with trait upcasting #120222
Description
Pointer casts allow switching trait parameters for trait objects, which can change the set of supertraits (and thus the vtable layout), ultimately making upcasting of raw pointers quite unsound
This code reproduces a segfault, on upcoming stable Rust, starting in 1.76 (stabilization of trait object upcasting).
Update: Stabilization was reverted, 1.76 stable Rust is good, the below code examples needs nightly Rust and #[feature(trait_upcasting)] now.
#![feature(trait_upcasting)]
pub trait SupSupA {
fn method(&self) {}
}
pub trait SupSupB {}
impl<T> SupSupA for T {}
impl<T> SupSupB for T {}
pub trait Super<T>: SupSupA + SupSupB {}
pub trait Unimplemented {}
pub trait Trait<T1, T2>: Super<T1> + Super<T2> {
fn missing_method(&self)
where
T1: Unimplemented,
{
}
}
impl<S, T> Super<T> for S {}
impl<S, T1, T2> Trait<T1, T2> for S {}
#[inline(never)]
pub fn user1() -> &'static dyn Trait<u8, u8> {
&()
/* VTABLE:
.L__unnamed_2:
.quad core::ptr::drop_in_place<()>
.asciz "\000\000\000\000\000\000\000\000\001\000\000\000\000\000\000"
.quad example::SupSupA::method
.quad .L__unnamed_4 // SupSupB vtable (pointer)
.zero 8 // null pointer for missing_method
*/
}
#[inline(never)]
pub fn user2() -> &'static dyn Trait<u8, u16> {
&()
/* VTABLE:
.L__unnamed_3:
.quad core::ptr::drop_in_place<()>
.asciz "\000\000\000\000\000\000\000\000\001\000\000\000\000\000\000"
.quad example::SupSupA::method
.quad .L__unnamed_4 // SupSupB vtable (pointer)
.quad .L__unnamed_5 // Super<u16> vtable (pointer)
.zero 8 // null pointer for missing_method
*/
}
fn main() {
let p: *const dyn Trait<u8, u8> = &();
let p = p as *const dyn Trait<u8, u16>; // <- this is bad!
let p = p as *const dyn Super<u16>; // <- this upcast accesses improper vtable entry
// accessing from L__unnamed_2 the position for the 'Super<u16> vtable (pointer)',
// thus reading 'null pointer for missing_method'
let p = p as *const dyn SupSupB; // <- this upcast dereferences (null) pointer from that entry
// to read the SupSupB vtable (pointer)
// SEGFAULT
println!("{:?}", p);
}(playground, compiler explorer)
This issue exists next to #120217, but here I’ve found how the issue of overly liberal casting of raw pointers can be made into a concrete soundness issue, using no feature flags, producing an actual segfault.
Whether or now we need (to keep) two separate & somewhat similar GitHub issues can probably become clear eventually. If we don’t, we can always close one or the other.
Miri will already complain with a less sophisticated setup:
#![feature(trait_upcasting)]
trait Trait<T>: Super {}
trait Super {}
impl<S, T> Trait<T> for S {}
impl<S> Super for S {}
fn main() {
let p: *const dyn Trait<u8> = &();
let p = p as *const dyn Trait<u16>;
let _p = p as *const dyn Super; // this is where miri complains already
}error: Undefined Behavior: upcast on a pointer whose vtable does not match its type
--> src/main.rs:12:14
|
12 | let _p = p as *const dyn Super; // this is where miri complains already
| ^ upcast on a pointer whose vtable does not match its type
|
= help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
= help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
= note: BACKTRACE:
= note: inside `main` at src/main.rs:12:14: 12:15
Three possible ways of fixing this would be to
- either make vtable layout even more regular,
- (and to officially allow somewhat “wrong” vtable types, also for miri, and to kill raw pointer[-like] types as
arbitrary_self_typesreceiver…)1
- (and to officially allow somewhat “wrong” vtable types, also for miri, and to kill raw pointer[-like] types as
- or to start working on limiting those pointer casts,
- (even though they’ve been possible since about Rust
1.2, so some potential breakage and/or transition period, …!?)
- (even though they’ve been possible since about Rust
- or to disallow upcasting raw pointers, after all.
- (perhaps also temporarily as a quick fix before pursuing a better approach)
I’m marking as regression in beta as this is newly introduced unsoundness; and an approach like temporarily removing raw pointers again from our upcasting feature (assuming that’s technically possible in the first place) and/or delaying the stabilization are options, in case this soundness issue is perceived as sufficiently relevant not to be “ignored”.
@rustbot label +regression-from-stable-to-beta +F-trait_upcasting +A-coercions +A-trait-objects +I-unsound
Footnotes
-
my thought here is that e.g. it seems like missing methods are replaced with zeroes already, too, instead of skipped, and that pattern could be extended so that in the case of different sets of supertraits (like the
Super<u8>==Super<u8> + Super<u8>vs.Super<u8> + Super<u16>above) could thus perhaps also avoid supertrait pointers appearing in inconsistent places between an applied trait constructor [Trait<Args1…>vsTraits<Args2…>] with different parameters ↩
Activity
[-]Pointer casts allow switching trait parameters for trait objects, which can change the set of supertraits (and thus the vtable layout), ultimately making upcasting of raw pointers quite unsound[/-][+]Pointer casts allow switching trait parameters for trait objects, which doesn’t interact soundly with trait upcasting[/+]steffahn commentedon Jan 22, 2024
Between T-lang, T-compiler, and T-types [all 3 listed on the tracking issue], I’m not sure which (or which ones) to assign.
Noratrieb commentedon Jan 22, 2024
lcnr on Zulip
20 remaining items
VorpalBlade commentedon Jul 1, 2024
I ran into something where trait upcasting would be the perfect fit. As such I went to look for what was blocking in the tracking PR. It seems that this is the only real blocker? If so, what is the current progress on this (if any)?
oli-obk commentedon Jul 1, 2024
#120248 is making progress on this
cc @WaffleLapkin
Rollup merge of rust-lang#120248 - WaffleLapkin:bonk-ptr-object-casts…
Unrolled build for rust-lang#120248
traviscross commentedon Jul 8, 2024
This is believed to be fixed by:
Rollup merge of #120248 - WaffleLapkin:bonk-ptr-object-casts, r=compi…
Rollup merge of #120248 - WaffleLapkin:bonk-ptr-object-casts, r=compi…
Rollup merge of rust-lang#135296 - lukas-code:dyn-leak-check, r=compi…
Unrolled build for rust-lang#135296
Rollup merge of #135296 - lukas-code:dyn-leak-check, r=compiler-errors
Rollup merge of #135296 - lukas-code:dyn-leak-check, r=compiler-errors