Skip to content

Commit 29fe618

Browse files
borsbors
committed
Auto merge of #123052 - maurer:addr-taken, r=compiler-errors
CFI: Support function pointers for trait methods Adds support for both CFI and KCFI for function pointers to trait methods by attaching both concrete and abstract types to functions. KCFI does this through generation of a `ReifyShim` on any function pointer for a method that could go into a vtable, and keeping this separate from `ReifyShim`s that are *intended* for vtable us by setting a `ReifyReason` on them. CFI does this by setting both the concrete and abstract type on every instance. This should land after #123024 or a similar PR, as it diverges the implementation of CFI vs KCFI. r? `@compiler-errors`
2 parents 43f4f2a + 473a70d commit 29fe618

File tree

15 files changed

+133
-37
lines changed

15 files changed

+133
-37
lines changed

‎compiler/rustc_codegen_llvm/src/declare.rs

+4-2
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
147147
for options in [
148148
TypeIdOptions::GENERALIZE_POINTERS,
149149
TypeIdOptions::NORMALIZE_INTEGERS,
150-
TypeIdOptions::NO_SELF_TYPE_ERASURE,
150+
TypeIdOptions::ERASE_SELF_TYPE,
151151
]
152152
.into_iter()
153153
.powerset()
@@ -173,7 +173,9 @@ impl<'ll, 'tcx> CodegenCx<'ll, 'tcx> {
173173

174174
if self.tcx.sess.is_sanitizer_kcfi_enabled() {
175175
// LLVM KCFI does not support multiple !kcfi_type attachments
176-
let mut options = TypeIdOptions::empty();
176+
// Default to erasing the self type. If we need the concrete type, there will be a
177+
// hint in the instance.
178+
let mut options = TypeIdOptions::ERASE_SELF_TYPE;
177179
if self.tcx.sess.is_sanitizer_cfi_generalize_pointers_enabled() {
178180
options.insert(TypeIdOptions::GENERALIZE_POINTERS);
179181
}

‎compiler/rustc_middle/src/mir/visit.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,7 @@ macro_rules! make_mir_visitor {
341341

342342
ty::InstanceDef::Intrinsic(_def_id) |
343343
ty::InstanceDef::VTableShim(_def_id) |
344-
ty::InstanceDef::ReifyShim(_def_id) |
344+
ty::InstanceDef::ReifyShim(_def_id, _) |
345345
ty::InstanceDef::Virtual(_def_id, _) |
346346
ty::InstanceDef::ThreadLocalShim(_def_id) |
347347
ty::InstanceDef::ClosureOnceShim { call_once: _def_id, track_caller: _ } |

‎compiler/rustc_middle/src/ty/instance.rs

+58-8
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,28 @@ pub struct Instance<'tcx> {
3131
pub args: GenericArgsRef<'tcx>,
3232
}
3333

34+
/// Describes why a `ReifyShim` was created. This is needed to distingish a ReifyShim created to
35+
/// adjust for things like `#[track_caller]` in a vtable from a `ReifyShim` created to produce a
36+
/// function pointer from a vtable entry.
37+
/// Currently, this is only used when KCFI is enabled, as only KCFI needs to treat those two
38+
/// `ReifyShim`s differently.
39+
#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
40+
#[derive(TyEncodable, TyDecodable, HashStable)]
41+
pub enum ReifyReason {
42+
/// The `ReifyShim` was created to produce a function pointer. This happens when:
43+
/// * A vtable entry is directly converted to a function call (e.g. creating a fn ptr from a
44+
/// method on a `dyn` object).
45+
/// * A function with `#[track_caller]` is converted to a function pointer
46+
/// * If KCFI is enabled, creating a function pointer from a method on an object-safe trait.
47+
/// This includes the case of converting `::call`-like methods on closure-likes to function
48+
/// pointers.
49+
FnPtr,
50+
/// This `ReifyShim` was created to populate a vtable. Currently, this happens when a
51+
/// `#[track_caller]` mismatch occurs between the implementation of a method and the method.
52+
/// This includes the case of `::call`-like methods in closure-likes' vtables.
53+
Vtable,
54+
}
55+
3456
#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug)]
3557
#[derive(TyEncodable, TyDecodable, HashStable, TypeFoldable, TypeVisitable, Lift)]
3658
pub enum InstanceDef<'tcx> {
@@ -67,7 +89,13 @@ pub enum InstanceDef<'tcx> {
6789
/// Because this is a required part of the function's ABI but can't be tracked
6890
/// as a property of the function pointer, we use a single "caller location"
6991
/// (the definition of the function itself).
70-
ReifyShim(DefId),
92+
///
93+
/// The second field encodes *why* this shim was created. This allows distinguishing between
94+
/// a `ReifyShim` that appears in a vtable vs one that appears as a function pointer.
95+
///
96+
/// This field will only be populated if we are compiling in a mode that needs these shims
97+
/// to be separable, currently only when KCFI is enabled.
98+
ReifyShim(DefId, Option<ReifyReason>),
7199

72100
/// `<fn() as FnTrait>::call_*` (generated `FnTrait` implementation for `fn()` pointers).
73101
///
@@ -194,7 +222,7 @@ impl<'tcx> InstanceDef<'tcx> {
194222
match self {
195223
InstanceDef::Item(def_id)
196224
| InstanceDef::VTableShim(def_id)
197-
| InstanceDef::ReifyShim(def_id)
225+
| InstanceDef::ReifyShim(def_id, _)
198226
| InstanceDef::FnPtrShim(def_id, _)
199227
| InstanceDef::Virtual(def_id, _)
200228
| InstanceDef::Intrinsic(def_id)
@@ -354,7 +382,9 @@ fn fmt_instance(
354382
match instance.def {
355383
InstanceDef::Item(_) => Ok(()),
356384
InstanceDef::VTableShim(_) => write!(f, " - shim(vtable)"),
357-
InstanceDef::ReifyShim(_) => write!(f, " - shim(reify)"),
385+
InstanceDef::ReifyShim(_, None) => write!(f, " - shim(reify)"),
386+
InstanceDef::ReifyShim(_, Some(ReifyReason::FnPtr)) => write!(f, " - shim(reify-fnptr)"),
387+
InstanceDef::ReifyShim(_, Some(ReifyReason::Vtable)) => write!(f, " - shim(reify-vtable)"),
358388
InstanceDef::ThreadLocalShim(_) => write!(f, " - shim(tls)"),
359389
InstanceDef::Intrinsic(_) => write!(f, " - intrinsic"),
360390
InstanceDef::Virtual(_, num) => write!(f, " - virtual#{num}"),
@@ -476,15 +506,34 @@ impl<'tcx> Instance<'tcx> {
476506
debug!("resolve(def_id={:?}, args={:?})", def_id, args);
477507
// Use either `resolve_closure` or `resolve_for_vtable`
478508
assert!(!tcx.is_closure_like(def_id), "Called `resolve_for_fn_ptr` on closure: {def_id:?}");
509+
let reason = tcx.sess.is_sanitizer_kcfi_enabled().then_some(ReifyReason::FnPtr);
479510
Instance::resolve(tcx, param_env, def_id, args).ok().flatten().map(|mut resolved| {
480511
match resolved.def {
481512
InstanceDef::Item(def) if resolved.def.requires_caller_location(tcx) => {
482513
debug!(" => fn pointer created for function with #[track_caller]");
483-
resolved.def = InstanceDef::ReifyShim(def);
514+
resolved.def = InstanceDef::ReifyShim(def, reason);
484515
}
485516
InstanceDef::Virtual(def_id, _) => {
486517
debug!(" => fn pointer created for virtual call");
487-
resolved.def = InstanceDef::ReifyShim(def_id);
518+
resolved.def = InstanceDef::ReifyShim(def_id, reason);
519+
}
520+
// Reify `Trait::method` implementations if KCFI is enabled
521+
// FIXME(maurer) only reify it if it is a vtable-safe function
522+
_ if tcx.sess.is_sanitizer_kcfi_enabled()
523+
&& tcx.associated_item(def_id).trait_item_def_id.is_some() =>
524+
{
525+
// If this function could also go in a vtable, we need to `ReifyShim` it with
526+
// KCFI because it can only attach one type per function.
527+
resolved.def = InstanceDef::ReifyShim(resolved.def_id(), reason)
528+
}
529+
// Reify `::call`-like method implementations if KCFI is enabled
530+
_ if tcx.sess.is_sanitizer_kcfi_enabled()
531+
&& tcx.is_closure_like(resolved.def_id()) =>
532+
{
533+
// Reroute through a reify via the *unresolved* instance. The resolved one can't
534+
// be directly reified because it's closure-like. The reify can handle the
535+
// unresolved instance.
536+
resolved = Instance { def: InstanceDef::ReifyShim(def_id, reason), args }
488537
}
489538
_ => {}
490539
}
@@ -508,6 +557,7 @@ impl<'tcx> Instance<'tcx> {
508557
debug!(" => associated item with unsizeable self: Self");
509558
Some(Instance { def: InstanceDef::VTableShim(def_id), args })
510559
} else {
560+
let reason = tcx.sess.is_sanitizer_kcfi_enabled().then_some(ReifyReason::Vtable);
511561
Instance::resolve(tcx, param_env, def_id, args).ok().flatten().map(|mut resolved| {
512562
match resolved.def {
513563
InstanceDef::Item(def) => {
@@ -544,18 +594,18 @@ impl<'tcx> Instance<'tcx> {
544594
// Create a shim for the `FnOnce/FnMut/Fn` method we are calling
545595
// - unlike functions, invoking a closure always goes through a
546596
// trait.
547-
resolved = Instance { def: InstanceDef::ReifyShim(def_id), args };
597+
resolved = Instance { def: InstanceDef::ReifyShim(def_id, reason), args };
548598
} else {
549599
debug!(
550600
" => vtable fn pointer created for function with #[track_caller]: {:?}", def
551601
);
552-
resolved.def = InstanceDef::ReifyShim(def);
602+
resolved.def = InstanceDef::ReifyShim(def, reason);
553603
}
554604
}
555605
}
556606
InstanceDef::Virtual(def_id, _) => {
557607
debug!(" => vtable fn pointer created for virtual call");
558-
resolved.def = InstanceDef::ReifyShim(def_id);
608+
resolved.def = InstanceDef::ReifyShim(def_id, reason)
559609
}
560610
_ => {}
561611
}

‎compiler/rustc_middle/src/ty/mod.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ pub use self::context::{
8888
tls, CtxtInterners, CurrentGcx, DeducedParamAttrs, Feed, FreeRegionInfo, GlobalCtxt, Lift,
8989
TyCtxt, TyCtxtFeed,
9090
};
91-
pub use self::instance::{Instance, InstanceDef, ShortInstance, UnusedGenericParams};
91+
pub use self::instance::{Instance, InstanceDef, ReifyReason, ShortInstance, UnusedGenericParams};
9292
pub use self::list::List;
9393
pub use self::parameterized::ParameterizedOverTcx;
9494
pub use self::predicate::{

‎compiler/rustc_middle/src/ty/structural_impls.rs

+1
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,7 @@ TrivialTypeTraversalAndLiftImpls! {
449449
crate::ty::ClosureKind,
450450
crate::ty::ParamConst,
451451
crate::ty::ParamTy,
452+
crate::ty::instance::ReifyReason,
452453
interpret::AllocId,
453454
interpret::CtfeProvenance,
454455
interpret::Scalar,

‎compiler/rustc_mir_transform/src/inline.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,7 @@ impl<'tcx> Inliner<'tcx> {
324324
// do not need to catch this here, we can wait until the inliner decides to continue
325325
// inlining a second time.
326326
InstanceDef::VTableShim(_)
327-
| InstanceDef::ReifyShim(_)
327+
| InstanceDef::ReifyShim(..)
328328
| InstanceDef::FnPtrShim(..)
329329
| InstanceDef::ClosureOnceShim { .. }
330330
| InstanceDef::ConstructCoroutineInClosureShim { .. }

‎compiler/rustc_mir_transform/src/inline/cycle.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ pub(crate) fn mir_callgraph_reachable<'tcx>(
8484
// again, a function item can end up getting inlined. Thus we'll be able to cause
8585
// a cycle that way
8686
InstanceDef::VTableShim(_)
87-
| InstanceDef::ReifyShim(_)
87+
| InstanceDef::ReifyShim(..)
8888
| InstanceDef::FnPtrShim(..)
8989
| InstanceDef::ClosureOnceShim { .. }
9090
| InstanceDef::ConstructCoroutineInClosureShim { .. }

‎compiler/rustc_mir_transform/src/shim.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ fn make_shim<'tcx>(tcx: TyCtxt<'tcx>, instance: ty::InstanceDef<'tcx>) -> Body<'
5555
// a virtual call, or a direct call to a function for which
5656
// indirect calls must be codegen'd differently than direct ones
5757
// (such as `#[track_caller]`).
58-
ty::InstanceDef::ReifyShim(def_id) => {
58+
ty::InstanceDef::ReifyShim(def_id, _) => {
5959
build_call_shim(tcx, instance, None, CallKind::Direct(def_id))
6060
}
6161
ty::InstanceDef::ClosureOnceShim { call_once: _, track_caller: _ } => {

‎compiler/rustc_symbol_mangling/src/legacy.rs

+9-3
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ use rustc_data_structures::stable_hasher::{Hash64, HashStable, StableHasher};
22
use rustc_hir::def_id::CrateNum;
33
use rustc_hir::definitions::{DefPathData, DisambiguatedDefPathData};
44
use rustc_middle::ty::print::{PrettyPrinter, Print, PrintError, Printer};
5-
use rustc_middle::ty::{self, Instance, Ty, TyCtxt, TypeVisitableExt};
5+
use rustc_middle::ty::{self, Instance, ReifyReason, Ty, TyCtxt, TypeVisitableExt};
66
use rustc_middle::ty::{GenericArg, GenericArgKind};
77

88
use std::fmt::{self, Write};
@@ -71,8 +71,14 @@ pub(super) fn mangle<'tcx>(
7171
ty::InstanceDef::VTableShim(..) => {
7272
printer.write_str("{{vtable-shim}}").unwrap();
7373
}
74-
ty::InstanceDef::ReifyShim(..) => {
75-
printer.write_str("{{reify-shim}}").unwrap();
74+
ty::InstanceDef::ReifyShim(_, reason) => {
75+
printer.write_str("{{reify-shim").unwrap();
76+
match reason {
77+
Some(ReifyReason::FnPtr) => printer.write_str("-fnptr").unwrap(),
78+
Some(ReifyReason::Vtable) => printer.write_str("-vtable").unwrap(),
79+
None => (),
80+
}
81+
printer.write_str("}}").unwrap();
7682
}
7783
// FIXME(async_closures): This shouldn't be needed when we fix
7884
// `Instance::ty`/`Instance::def_id`.

‎compiler/rustc_symbol_mangling/src/typeid.rs

+10-5
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
/// For more information about LLVM CFI and cross-language LLVM CFI support for the Rust compiler,
55
/// see design document in the tracking issue #89653.
66
use bitflags::bitflags;
7-
use rustc_middle::ty::{Instance, Ty, TyCtxt};
7+
use rustc_middle::ty::{Instance, InstanceDef, ReifyReason, Ty, TyCtxt};
88
use rustc_target::abi::call::FnAbi;
99
use std::hash::Hasher;
1010
use twox_hash::XxHash64;
@@ -24,9 +24,9 @@ bitflags! {
2424
/// `-fsanitize-cfi-icall-experimental-normalize-integers` option for cross-language LLVM
2525
/// CFI and KCFI support.
2626
const NORMALIZE_INTEGERS = 4;
27-
/// Do not perform self type erasure for attaching a secondary type id to methods with their
28-
/// concrete self so they can be used as function pointers.
29-
const NO_SELF_TYPE_ERASURE = 8;
27+
/// Generalize the instance by erasing the concrete `Self` type where possible.
28+
/// Only has an effect on `{kcfi_,}typeid_for_instance`.
29+
const ERASE_SELF_TYPE = 8;
3030
}
3131
}
3232

@@ -67,8 +67,13 @@ pub fn kcfi_typeid_for_fnabi<'tcx>(
6767
pub fn kcfi_typeid_for_instance<'tcx>(
6868
tcx: TyCtxt<'tcx>,
6969
instance: Instance<'tcx>,
70-
options: TypeIdOptions,
70+
mut options: TypeIdOptions,
7171
) -> u32 {
72+
// If we receive a `ReifyShim` intended to produce a function pointer, we need to remain
73+
// concrete - abstraction is for vtables.
74+
if matches!(instance.def, InstanceDef::ReifyShim(_, Some(ReifyReason::FnPtr))) {
75+
options.remove(TypeIdOptions::ERASE_SELF_TYPE);
76+
}
7277
// A KCFI type metadata identifier is a 32-bit constant produced by taking the lower half of the
7378
// xxHash64 of the type metadata identifier. (See llvm/llvm-project@cff5bef.)
7479
let mut hash: XxHash64 = Default::default();

‎compiler/rustc_symbol_mangling/src/typeid/typeid_itanium_cxx_abi.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1172,7 +1172,7 @@ pub fn typeid_for_instance<'tcx>(
11721172
instance.args = tcx.mk_args_trait(invoke_ty, trait_ref.args.into_iter().skip(1));
11731173
}
11741174

1175-
if !options.contains(EncodeTyOptions::NO_SELF_TYPE_ERASURE) {
1175+
if options.contains(EncodeTyOptions::ERASE_SELF_TYPE) {
11761176
if let Some(impl_id) = tcx.impl_of_method(instance.def_id())
11771177
&& let Some(trait_ref) = tcx.impl_trait_ref(impl_id)
11781178
{

‎compiler/rustc_symbol_mangling/src/v0.rs

+5-3
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ use rustc_hir::definitions::{DefPathData, DisambiguatedDefPathData};
88
use rustc_middle::ty::layout::IntegerExt;
99
use rustc_middle::ty::print::{Print, PrintError, Printer};
1010
use rustc_middle::ty::{
11-
self, EarlyBinder, FloatTy, Instance, IntTy, Ty, TyCtxt, TypeVisitable, TypeVisitableExt,
12-
UintTy,
11+
self, EarlyBinder, FloatTy, Instance, IntTy, ReifyReason, Ty, TyCtxt, TypeVisitable,
12+
TypeVisitableExt, UintTy,
1313
};
1414
use rustc_middle::ty::{GenericArg, GenericArgKind};
1515
use rustc_span::symbol::kw;
@@ -44,7 +44,9 @@ pub(super) fn mangle<'tcx>(
4444
let shim_kind = match instance.def {
4545
ty::InstanceDef::ThreadLocalShim(_) => Some("tls"),
4646
ty::InstanceDef::VTableShim(_) => Some("vtable"),
47-
ty::InstanceDef::ReifyShim(_) => Some("reify"),
47+
ty::InstanceDef::ReifyShim(_, None) => Some("reify"),
48+
ty::InstanceDef::ReifyShim(_, Some(ReifyReason::FnPtr)) => Some("reify-fnptr"),
49+
ty::InstanceDef::ReifyShim(_, Some(ReifyReason::Vtable)) => Some("reify-vtable"),
4850

4951
ty::InstanceDef::ConstructCoroutineInClosureShim { .. }
5052
| ty::InstanceDef::CoroutineKindShim { .. } => Some("fn_once"),

‎tests/codegen/sanitizer/cfi/emit-type-metadata-id-itanium-cxx-abi-method-secondary-typeid.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -18,5 +18,5 @@ impl Trait1 for Type1 {
1818
}
1919

2020

21-
// CHECK: ![[TYPE1]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtC{{[[:print:]]+}}_{{[[:print:]]+}}6Trait1u6regionEEE"}
22-
// CHECK: ![[TYPE2]] = !{i64 0, !"_ZTSFvu3refIu{{[0-9]+}}NtC{{[[:print:]]+}}_{{[[:print:]]+}}5Type1EE"}
21+
// CHECK: ![[TYPE1]] = !{i64 0, !"_ZTSFvu3refIu{{[0-9]+}}NtC{{[[:print:]]+}}_{{[[:print:]]+}}5Type1EE"}
22+
// CHECK: ![[TYPE2]] = !{i64 0, !"_ZTSFvu3refIu3dynIu{{[0-9]+}}NtC{{[[:print:]]+}}_{{[[:print:]]+}}6Trait1u6regionEEE"}

‎tests/ui/sanitizer/cfi-closures.rs

-4
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515

1616
#![feature(fn_traits)]
1717
#![feature(unboxed_closures)]
18-
#![feature(cfg_sanitize)]
1918

2019
fn foo<'a, T>() -> Box<dyn Fn(&'a T) -> &'a T> {
2120
Box::new(|x| x)
@@ -72,9 +71,6 @@ fn use_closure<C>(call: extern "rust-call" fn(&C, ()) -> i32, f: &C) -> i32 {
7271
}
7372

7473
#[test]
75-
// FIXME after KCFI reify support is added, remove this
76-
// It will appear to work if you test locally, set -C opt-level=0 to see it fail.
77-
#[cfg_attr(sanitize = "kcfi", ignore)]
7874
fn closure_addr_taken() {
7975
let x = 3i32;
8076
let f = || x;

‎tests/ui/sanitizer/cfi-method-fn-ptr-cast.rs

+38-4
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,41 @@
11
// Verifies that casting a method to a function pointer works.
2-
//
3-
// FIXME(#122848): Remove only-linux when fixed.
2+
3+
//@ revisions: cfi kcfi
4+
// FIXME(#122848) Remove only-linux once OSX CFI binaries work
45
//@ only-linux
5-
//@ needs-sanitizer-cfi
6-
//@ compile-flags: -Clto -Copt-level=0 -Cprefer-dynamic=off -Ctarget-feature=-crt-static -Zsanitizer=cfi
6+
//@ [cfi] needs-sanitizer-cfi
7+
//@ [kcfi] needs-sanitizer-kcfi
8+
//@ compile-flags: -C target-feature=-crt-static
9+
//@ [cfi] compile-flags: -C opt-level=0 -C codegen-units=1 -C lto
10+
//@ [cfi] compile-flags: -C prefer-dynamic=off
11+
//@ [cfi] compile-flags: -Z sanitizer=cfi
12+
//@ [kcfi] compile-flags: -Z sanitizer=kcfi
13+
//@ [kcfi] compile-flags: -C panic=abort -C prefer-dynamic=off
714
//@ run-pass
815

16+
trait Foo {
17+
fn foo(&self);
18+
fn bar(&self);
19+
}
20+
21+
struct S;
22+
23+
impl Foo for S {
24+
fn foo(&self) {}
25+
#[track_caller]
26+
fn bar(&self) {}
27+
}
28+
29+
struct S2 {
30+
f: fn(&S)
31+
}
32+
33+
impl S2 {
34+
fn foo(&self, s: &S) {
35+
(self.f)(s)
36+
}
37+
}
38+
939
trait Trait1 {
1040
fn foo(&self);
1141
}
@@ -20,4 +50,8 @@ fn main() {
2050
let type1 = Type1 {};
2151
let f = <Type1 as Trait1>::foo;
2252
f(&type1);
53+
// Check again with different optimization barriers
54+
S2 { f: <S as Foo>::foo }.foo(&S);
55+
// Check mismatched #[track_caller]
56+
S2 { f: <S as Foo>::bar }.foo(&S)
2357
}

0 commit comments

Comments
 (0)