Skip to content

Commit

Permalink
Auto merge of rust-lang#119037 - RalfJung:repr-c-abi-mismatch, r=scot…
Browse files Browse the repository at this point in the history
…tmcm

do not allow ABI mismatches inside repr(C) types

In rust-lang#115476 we allowed ABI mismatches inside `repr(C)` types. This wasn't really discussed much; I added it because from how I understand calling conventions, this should actually be safe in practice. However I entirely forgot to actually allow this in Miri, and in the mean time I have learned that too much ABI compatibility can be a problem for CFI (it can reject fewer calls so that gives an attacker more room to play with).

So I propose we take back that part about ABI compatibility in `repr(C)`. It is anyway something that C and C++ do not allow, as far as I understand.

In the future we might want to introduce a class of ABI compatibilities where we say "this is a bug and it may lead to aborting the process, but it won't lead to arbitrary misbehavior -- worst case it'll just transmute the arguments from the caller type to the callee type". That would give CFI leeway to reject such calls without introducing the risk of arbitrary UB. (The UB can still happen if the transmute leads to bad results, of course, but it wouldn't be due to ABI weirdness.)

rust-lang#115476 hasn't reached beta yet so if we land this before Dec 22nd we can just pretend this all never happened. ;)  Otherwise we should do a beta backport (of the docs change at least).

Cc `@rust-lang/opsem` `@rust-lang/types`
  • Loading branch information
bors committed Dec 20, 2023
2 parents 92ad4b4 + c7e3b3f commit 5ac4c8a
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 2 deletions.
2 changes: 0 additions & 2 deletions library/core/src/primitive_docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1575,8 +1575,6 @@ mod prim_ref {}
/// Furthermore, ABI compatibility satisfies the following general properties:
///
/// - Every type is ABI-compatible with itself.
/// - If `T1` and `T2` are ABI-compatible, then two `repr(C)` types that only differ because one
/// field type was changed from `T1` to `T2` are ABI-compatible.
/// - If `T1` and `T2` are ABI-compatible and `T2` and `T3` are ABI-compatible, then so are `T1` and
/// `T3` (i.e., ABI-compatibility is transitive).
/// - If `T1` and `T2` are ABI-compatible, then so are `T2` and `T1` (i.e., ABI-compatibility is
Expand Down
16 changes: 16 additions & 0 deletions src/tools/miri/tests/fail/function_pointers/abi_mismatch_repr_C.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
use std::num::*;

#[repr(C)]
struct S1(NonZeroI32);

#[repr(C)]
struct S2(i32);

fn callee(_s: S2) {}

fn main() {
let fnptr: fn(S2) = callee;
let fnptr: fn(S1) = unsafe { std::mem::transmute(fnptr) };
fnptr(S1(NonZeroI32::new(1).unwrap()));
//~^ ERROR: calling a function with argument of type S2 passing data of type S1
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
error: Undefined Behavior: calling a function with argument of type S2 passing data of type S1
--> $DIR/abi_mismatch_repr_C.rs:LL:CC
|
LL | fnptr(S1(NonZeroI32::new(1).unwrap()));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ calling a function with argument of type S2 passing data of type S1
|
= 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
= help: this means these two types are not *guaranteed* to be ABI-compatible across all targets
= help: if you think this code should be accepted anyway, please report an issue
= note: BACKTRACE:
= note: inside `main` at $DIR/abi_mismatch_repr_C.rs:LL:CC

note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace

error: aborting due to 1 previous error

0 comments on commit 5ac4c8a

Please sign in to comment.